fix(#371): skipped role is not 'allInstalled', test the reason->action branch (review round 1)
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) <noreply@anthropic.com>
This commit is contained in:
+18
-13
@@ -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,
|
||||
|
||||
Reference in New Issue
Block a user