From b8cce4f814fe7c79b22c92aa234ce9de9f07757c Mon Sep 17 00:00:00 2001 From: agent_coder Date: Sun, 5 Jul 2026 06:09:58 +0300 Subject: [PATCH] fix(#371): skipped role is not 'allInstalled', test the reason->action branch (review round 1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit F1 [WARNING] bundlePhase returned 'allInstalled' when a bundle's only non-installed role was skipped (0 installed for it), so the collapsed green 'All installed · up to date' header contradicted the open 'Installed 0 · 1 skipped' plaque. It now returns 'mixed' whenever a skipped role is present. Fixed the test that encoded the wrong behavior. F2 [WARNING] The reason->action branch (name-conflict -> transient overlay + 'Rename & install'; already-installed -> informational, no button) lived only in the component, untested. Extracted the two decisions into pure, unit-tested helpers nameConflictSlugs() and partialOffersRename() and wired them into the modal; both reason values are now covered. F3 [low] Removed the unused useRef import (client eslint no-unused-vars is off, so it shipped silently). F4 [low] Extracted bundleCounts() as the single tally pass; bundlePhase and the panel both derive from it instead of rescanning the roles array ~5x per render (the same model<->component consolidation this PR is about). Co-Authored-By: Claude Opus 4.8 (1M context) --- .../utils/catalog-bundle-model.test.ts | 49 +++++++++++- .../ai-chat/utils/catalog-bundle-model.ts | 74 ++++++++++++++++--- .../ai-agent-roles-catalog-modal.tsx | 31 ++++---- 3 files changed, 130 insertions(+), 24 deletions(-) diff --git a/apps/client/src/features/ai-chat/utils/catalog-bundle-model.test.ts b/apps/client/src/features/ai-chat/utils/catalog-bundle-model.test.ts index 23806750..b7ff39a3 100644 --- a/apps/client/src/features/ai-chat/utils/catalog-bundle-model.test.ts +++ b/apps/client/src/features/ai-chat/utils/catalog-bundle-model.test.ts @@ -1,9 +1,12 @@ import { describe, it, expect } from "vitest"; import { + bundleCounts, bundlePhase, installedLangForRole, mapBundleRolesToView, mapCatalogRoleToView, + nameConflictSlugs, + partialOffersRename, type CatalogViewRole, } from "./catalog-bundle-model.ts"; import type { @@ -82,8 +85,50 @@ describe("bundlePhase", () => { expect(bundlePhase([viewRole("import"), viewRole("update")])).toBe("mixed"); }); - it("transient skipped roles are ignored (counted as neither) -> allInstalled", () => { - expect(bundlePhase([viewRole("skipped")])).toBe("allInstalled"); + it("a skipped role with nothing installed -> mixed (NOT allInstalled)", () => { + // F1: a bundle whose only non-installed role was skipped has 0 installed for + // it, so the collapsed 'All installed · up to date' header would contradict + // the open 'Installed 0 · 1 skipped' plaque. It must be mixed until resolved. + expect(bundlePhase([viewRole("skipped")])).toBe("mixed"); + }); + + it("installed + a skipped role -> mixed (partial success is not allInstalled)", () => { + expect(bundlePhase([viewRole("installed"), viewRole("skipped")])).toBe( + "mixed", + ); + }); +}); + +describe("bundleCounts", () => { + it("tallies each status once", () => { + expect( + bundleCounts([ + viewRole("import"), + viewRole("import"), + viewRole("installed"), + viewRole("update"), + viewRole("skipped"), + ]), + ).toEqual({ importable: 2, installed: 1, update: 1, skipped: 1 }); + }); +}); + +describe("nameConflictSlugs / partialOffersRename (reason -> action)", () => { + it("only name-conflict skips become the transient overlay / offer rename", () => { + const skipped = [ + { slug: "writer", name: "Writer", reason: "name-conflict" as const }, + { slug: "editor", name: "Editor", reason: "already-installed" as const }, + ]; + expect(nameConflictSlugs(skipped)).toEqual(["writer"]); + expect(partialOffersRename(skipped)).toBe(true); + }); + + it("an already-installed-only skip is informational: no overlay, no rename", () => { + const skipped = [ + { slug: "editor", name: "Editor", reason: "already-installed" as const }, + ]; + expect(nameConflictSlugs(skipped)).toEqual([]); + expect(partialOffersRename(skipped)).toBe(false); }); }); diff --git a/apps/client/src/features/ai-chat/utils/catalog-bundle-model.ts b/apps/client/src/features/ai-chat/utils/catalog-bundle-model.ts index c7102379..6197a638 100644 --- a/apps/client/src/features/ai-chat/utils/catalog-bundle-model.ts +++ b/apps/client/src/features/ai-chat/utils/catalog-bundle-model.ts @@ -52,11 +52,9 @@ export interface CatalogViewRole { * the collapsed-header summary and the bundle's single primary action. * - `empty` — the bundle has no roles. * - `allNew` — everything is importable, nothing installed. - * - `allInstalled` — nothing to import and nothing to update. + * - `allInstalled` — everything installed & up to date; nothing else pending. * - `updates` — updates available and nothing left to import. * - `mixed` — any other combination. - * Transient `skipped` roles are ignored (they count as neither import, installed - * nor update), so a post-import conflict does not distort the header summary. */ export type BundlePhase = | "empty" @@ -65,17 +63,75 @@ export type BundlePhase = | "updates" | "mixed"; +/** Per-status tallies for a bundle's roles (the single source of truth). */ +export interface BundleCounts { + importable: number; + installed: number; + update: number; + skipped: number; +} + +/** + * Count a bundle's roles by status ONCE. Both `bundlePhase` and the panel derive + * from this, so the tally logic lives in exactly one place (no rescans / drift). + */ +export function bundleCounts(roles: CatalogViewRole[]): BundleCounts { + const counts: BundleCounts = { + importable: 0, + installed: 0, + update: 0, + skipped: 0, + }; + for (const r of roles) { + if (r.status === "import") counts.importable += 1; + else if (r.status === "installed") counts.installed += 1; + else if (r.status === "update") counts.update += 1; + else if (r.status === "skipped") counts.skipped += 1; + } + return counts; +} + export function bundlePhase(roles: CatalogViewRole[]): BundlePhase { if (roles.length === 0) return "empty"; - const imp = roles.filter((r) => r.status === "import").length; - const ups = roles.filter((r) => r.status === "update").length; - const installed = roles.filter((r) => r.status === "installed").length; - if (imp === 0 && ups === 0) return "allInstalled"; - if (ups > 0 && imp === 0) return "updates"; - if (imp > 0 && installed === 0 && ups === 0) return "allNew"; + const { importable, installed, update, skipped } = bundleCounts(roles); + // A `skipped` role is a pending post-import conflict (0 installed for it), so a + // bundle that has ANY skipped role is NOT "all installed & up to date" — that + // would make the collapsed green "up to date" header contradict the open + // panel's "Installed 0 · 1 skipped" plaque. It is `mixed` until resolved. + if (importable === 0 && update === 0 && skipped === 0) return "allInstalled"; + if (update > 0 && importable === 0 && skipped === 0) return "updates"; + if (importable > 0 && installed === 0 && update === 0 && skipped === 0) + return "allNew"; return "mixed"; } +/** + * The subset of a skip result that should be shown as a TRANSIENT `skipped` + * overlay in the bundle (so the row offers a re-import path). Only NAME-CONFLICT + * skips qualify: an `already-installed` skip (a concurrent-import race) has + * nothing to act on — re-importing the same slug would just skip again — so it + * must NOT be overlaid (else the row shows a misleading "Rename & install" that + * self-heals into a false "installed"). Pure so both reason branches are tested. + */ +export function nameConflictSlugs( + skipped: { slug: string; reason: "name-conflict" | "already-installed" }[], +): string[] { + return skipped + .filter((s) => s.reason === "name-conflict") + .map((s) => s.slug); +} + +/** + * Whether a partial-import result should offer the "Rename & install" action: + * only when at least one skip is a name conflict (renameable). An + * `already-installed`-only partial is informational. + */ +export function partialOffersRename( + skipped: { reason: "name-conflict" | "already-installed" }[], +): boolean { + return skipped.some((s) => s.reason === "name-conflict"); +} + /** * For a role NOT installed in the current `language`, find a workspace role with * the same catalog `slug` installed under a DIFFERENT language, and return that diff --git a/apps/client/src/features/workspace/components/settings/components/ai-agent-roles-catalog-modal.tsx b/apps/client/src/features/workspace/components/settings/components/ai-agent-roles-catalog-modal.tsx index 48bc4249..e4547aa4 100644 --- a/apps/client/src/features/workspace/components/settings/components/ai-agent-roles-catalog-modal.tsx +++ b/apps/client/src/features/workspace/components/settings/components/ai-agent-roles-catalog-modal.tsx @@ -1,4 +1,4 @@ -import { useEffect, useMemo, useRef, useState } from "react"; +import { useEffect, useMemo, useState } from "react"; import { Accordion, Alert, @@ -33,9 +33,12 @@ import { } from "@/features/ai-chat/queries/ai-chat-query.ts"; import { IAiRole } from "@/features/ai-chat/types/ai-chat.types.ts"; import { + bundleCounts, bundlePhase, CatalogViewRole, mapBundleRolesToView, + nameConflictSlugs, + partialOffersRename, RoleStatus, } from "@/features/ai-chat/utils/catalog-bundle-model.ts"; @@ -290,15 +293,14 @@ export default function AiAgentRolesCatalogModal({ slugs, conflict: "skip", }); - // Only name conflicts get a "Rename & install"; already-installed skips - // are just informational (they can happen on a concurrent import race). - const nameConflicts = res.skippedRoles.filter( - (s) => s.reason === "name-conflict", - ); - if (nameConflicts.length > 0) { + // Only name conflicts become a transient `skipped` overlay (renameable); + // an already-installed race has nothing to act on. Decision lives in the + // pure, unit-tested nameConflictSlugs helper. + const conflictSlugs = nameConflictSlugs(res.skippedRoles); + if (conflictSlugs.length > 0) { setSkipped((prev) => { const set = new Set(prev[b.id] ?? []); - nameConflicts.forEach((s) => set.add(s.slug)); + conflictSlugs.forEach((slug) => set.add(slug)); return { ...prev, [b.id]: set }; }); } @@ -610,11 +612,11 @@ function BundlePanel({ }: BundlePanelProps) { const { t } = useTranslation(); + // Single tally pass shared by the summary and the primary action (F4). + const counts = bundleCounts(b.roles); const impCount = importableCount; - const upCount = b.roles.filter((r) => r.status === "update").length; - const installedCount = b.roles.filter( - (r) => r.status === "installed", - ).length; + const upCount = counts.update; + const installedCount = counts.installed; const busy = busyBundle === b.id || busyBundle === GLOBAL_SCOPE; const phase = bundlePhase(b.roles); @@ -952,7 +954,10 @@ function ResultBanner({ // name-conflict skip; an already-installed race is informational (re-importing // the same slug+language would just skip again, so no button — otherwise the // click self-heals into a false "installed" with nothing actually installed). - const nameConflict = result.skipped.find((s) => s.reason === "name-conflict"); + const offersRename = partialOffersRename(result.skipped); + const nameConflict = offersRename + ? result.skipped.find((s) => s.reason === "name-conflict") + : undefined; const detail = nameConflict ? t('A role named "{{name}}" already exists in this workspace.', { name: nameConflict.name,