From 163dac48b080678cd1b0e0c164349d95145d10c1 Mon Sep 17 00:00:00 2001 From: TyperBody Date: Mon, 21 Sep 2026 00:40:23 +0800 Subject: [PATCH] fix(plugins): correct install progress bounds, skill identity, and stage accuracy Addresses review feedback on the marketplace installed-state change. - Progress: byte-derived progress no longer has time-based drift layered on top, and fallback drift is clamped to the current stage range, so the bar cannot exceed the download band. 90/100 bytes at 40s elapsed used to report 61% against a declared 5-45% band; it now reports 41%. Drift is measured from when the stage was entered rather than from task start. - Skills: a marketplace skill is no longer reported installed from a bare skill name. The backend names skills from their own SKILL.md and records no publisher, so alice/review and bob/review install identically; matching the bare name marked every publisher's skill as installed. Skill cards now resolve to not-installed until the installed skill carries a publisher. - Stages: "installing plugin dependencies" and "launching plugin" described work this task context cannot observe (installation persistence ran under the former; the runtime installs dependencies and starts the plugin inside apply_plugin_installation under the latter). They become "persisting the installation" and "installing or starting plugin", and the frontend maps that combined step to the dependency stage rather than the launch stage. - Removed the per-dependency progress fields: the backend never populated them, so the UI could never have displayed them. The stage mapping and progress maths move to install-progress.ts, and the installed-state matching to a React-free marketplace-installed.ts, so both are covered by executable tests (+9). Verified: ruff, tsc --noEmit, prettier --check, eslint (0 errors), 98/98 unit tests. --- src/langbot/pkg/plugin/connector.py | 9 +- .../PluginInstallProgressDialog.tsx | 50 +---- .../PluginInstallTaskContext.tsx | 173 ++++-------------- .../plugin-install-task/install-progress.ts | 135 ++++++++++++++ .../PluginInstalledComponent.tsx | 3 +- .../plugin-market/PluginMarketComponent.tsx | 6 +- .../plugin-market/RecommendationLists.tsx | 2 +- .../plugin-market/marketplace-installed.ts | 87 ++++----- .../useMarketplaceInstalledIndex.ts | 24 +++ web/tests/unit/install-progress.test.mjs | 133 ++++++++++++++ .../unit/marketplace-installed-state.test.mjs | 121 ++++++++++++ 11 files changed, 494 insertions(+), 249 deletions(-) create mode 100644 web/src/app/home/plugins/components/plugin-install-task/install-progress.ts create mode 100644 web/src/app/home/plugins/components/plugin-market/useMarketplaceInstalledIndex.ts create mode 100644 web/tests/unit/install-progress.test.mjs create mode 100644 web/tests/unit/marketplace-installed-state.test.mjs diff --git a/src/langbot/pkg/plugin/connector.py b/src/langbot/pkg/plugin/connector.py index cf3f4b4aa..223928058 100644 --- a/src/langbot/pkg/plugin/connector.py +++ b/src/langbot/pkg/plugin/connector.py @@ -1770,7 +1770,7 @@ class PluginRuntimeConnector(ManagedRuntimeConnector): artifact_digest = hashlib.sha256(file_bytes).hexdigest() await self._store_artifact_package(execution_context, artifact_digest, file_bytes) if task_context is not None: - task_context.set_current_action('installing plugin dependencies') + task_context.set_current_action('persisting the installation') try: binding, previous_digest, previous_was_durable = await self._persist_installation_package( execution_context, @@ -1789,7 +1789,12 @@ class PluginRuntimeConnector(ManagedRuntimeConnector): plugin_name=plugin_name, ) if task_context is not None: - task_context.set_current_action('launching plugin') + # The runtime installs the plugin's dependencies and starts it + # inside apply_plugin_installation. It does not stream + # per-dependency progress back to this task context, so this stage + # deliberately stays coarse instead of claiming a separate, + # unobservable "installing dependencies" step. + task_context.set_current_action('installing or starting plugin') await self._apply_desired_state( PluginInstallationDesiredState(binding=binding, enabled=True), artifact_package=file_bytes, diff --git a/web/src/app/home/plugins/components/plugin-install-task/PluginInstallProgressDialog.tsx b/web/src/app/home/plugins/components/plugin-install-task/PluginInstallProgressDialog.tsx index f4f7ede31..def01d4bf 100644 --- a/web/src/app/home/plugins/components/plugin-install-task/PluginInstallProgressDialog.tsx +++ b/web/src/app/home/plugins/components/plugin-install-task/PluginInstallProgressDialog.tsx @@ -225,53 +225,9 @@ function TaskProgressContent({ task }: { task: PluginInstallTask }) { } if (stageKey === InstallStage.INSTALLING_DEPS) { - const total = task.depsTotal; - const installed = task.depsInstalled; - const remaining = task.depsRemaining; - const currentDep = task.currentDep; - const dlSize = task.depsDownloadedSize; - const speed = task.depsSpeed; - - if (isCompletedView && total != null) { - const parts: string[] = []; - parts.push(t('plugins.installProgress.depsInfo', { count: total })); - if (dlSize && dlSize > 0) { - parts.push(formatFileSize(dlSize)); - } - return parts.join(' · '); - } - - if (total != null && installed != null) { - const parts: string[] = []; - parts.push( - t('plugins.installProgress.depsProgress', { - installed, - total, - remaining: remaining ?? total - installed, - }), - ); - if (dlSize && dlSize > 0) { - parts.push(formatFileSize(dlSize)); - } - if (speed && speed > 0) { - parts.push(formatSpeed(speed)); - } - if (currentDep) { - return ( - <> - {parts.join(' · ')} -
- {currentDep} - - ); - } - return parts.join(' · '); - } - - if (total != null) { - return t('plugins.installProgress.depsInfo', { count: total }); - } - + // The runtime installs dependencies and starts the plugin in one step + // and does not report per-dependency progress, so this stage has no + // detail to show beyond its label. return undefined; } diff --git a/web/src/app/home/plugins/components/plugin-install-task/PluginInstallTaskContext.tsx b/web/src/app/home/plugins/components/plugin-install-task/PluginInstallTaskContext.tsx index 92878dbb3..247b6d88a 100644 --- a/web/src/app/home/plugins/components/plugin-install-task/PluginInstallTaskContext.tsx +++ b/web/src/app/home/plugins/components/plugin-install-task/PluginInstallTaskContext.tsx @@ -8,18 +8,14 @@ import React, { } from 'react'; import { httpClient } from '@/app/infra/http/HttpClient'; import { AsyncTask } from '@/app/infra/entities/api'; +import { + InstallStage, + INSTALL_PROGRESS_CAP, + computeStageProgress, + mapActionToStage, +} from './install-progress'; -/** - * Installation stages mapped from backend current_action strings. - */ -export enum InstallStage { - DOWNLOADING = 'downloading', - INSTALLING_DEPS = 'installing_deps', - INITIALIZING = 'initializing', - LAUNCHING = 'launching', - DONE = 'done', - ERROR = 'error', -} +export { InstallStage } from './install-progress'; export interface PluginInstallTask { id: string; // unique key: `${source}-${taskId}` @@ -34,15 +30,10 @@ export interface PluginInstallTask { downloadCurrent?: number; // bytes downloaded so far downloadTotal?: number; // total bytes to download downloadSpeed?: number; // bytes per second - // Dependency progress - depsTotal?: number; // total dependency count - depsInstalled?: number; // deps installed so far - depsRemaining?: number; // remaining - currentDep?: string; // currently installing dep name - depsDownloadedSize?: number; // total bytes of downloaded deps - depsSpeed?: number; // deps download speed bytes/s error?: string; startedAt: number; // timestamp + /** When the current stage began, used to bound in-stage drift. */ + stageStartedAt: number; currentAction: string; // raw backend action string } @@ -83,87 +74,6 @@ export function usePluginInstallTasks() { return ctx; } -/** - * Map the backend `current_action` string to an InstallStage. - * - * The runtime connector emits human-readable stage strings; each branch here - * matches the wording produced by the connector so newly added stages show up - * in the UI without a protocol change. - */ -function mapActionToStage(action: string): InstallStage { - const lower = (action || '').toLowerCase(); - - if ( - lower.includes('installed') || - lower.includes('complete') || - lower.includes('ready') - ) { - // "waiting for plugin to become ready" is still an active stage. - if (lower.includes('waiting')) return InstallStage.LAUNCHING; - return InstallStage.DONE; - } - if (lower.includes('launch') || lower.includes('start')) { - return InstallStage.LAUNCHING; - } - // Check the pre-download stages before the generic "install" match below, - // because "preparing plugin install" also contains "install". - if (lower.includes('prepar')) return InstallStage.DOWNLOADING; - if (lower.includes('download')) return InstallStage.DOWNLOADING; - - if ( - lower.includes('dependenc') || - lower.includes('requirements') || - lower.includes('install') - ) { - return InstallStage.INSTALLING_DEPS; - } - if (lower.includes('initializ') || lower.includes('configur')) { - return InstallStage.INITIALIZING; - } - if (lower.includes('inspect') || lower.includes('storing')) { - return InstallStage.INSTALLING_DEPS; - } - - return InstallStage.DOWNLOADING; -} - -/** - * Progress range (start → end) attributed to each stage, used to build a - * smooth determinate bar that never goes backwards. - */ -const STAGE_PROGRESS_RANGE: Record = { - [InstallStage.DOWNLOADING]: [5, 45], - [InstallStage.INSTALLING_DEPS]: [45, 85], - [InstallStage.INITIALIZING]: [85, 88], - [InstallStage.LAUNCHING]: [88, 97], - [InstallStage.DONE]: [100, 100], - [InstallStage.ERROR]: [0, 0], -}; - -/** - * Compute overall progress, preferring real byte counts over the stage range - * when the backend has reported a download size. - */ -function computeOverallProgress(task: { - stage: InstallStage; - downloadCurrent?: number; - downloadTotal?: number; -}): number { - const [start, end] = STAGE_PROGRESS_RANGE[task.stage] ?? [0, 0]; - - if ( - task.stage === InstallStage.DOWNLOADING && - task.downloadTotal && - task.downloadTotal > 0 && - task.downloadCurrent != null - ) { - const ratio = Math.min(1, task.downloadCurrent / task.downloadTotal); - return Math.round(start + (end - start) * ratio); - } - - return start; -} - /** * Extract install source from backend task name. */ @@ -215,11 +125,12 @@ function asyncTaskToPluginInstallTask(task: AsyncTask): PluginInstallTask { } else { stage = mapActionToStage(action); overallProgress = Math.min( - 99, - computeOverallProgress({ + INSTALL_PROGRESS_CAP, + computeStageProgress({ stage, downloadCurrent: num(md.download_current), downloadTotal: num(md.download_total), + stageElapsedSeconds: 0, }), ); } @@ -244,14 +155,9 @@ function asyncTaskToPluginInstallTask(task: AsyncTask): PluginInstallTask { downloadCurrent: num(md.download_current), downloadTotal: num(md.download_total), downloadSpeed: num(md.download_speed), - depsTotal: num(md.deps_total), - depsInstalled: num(md.deps_installed), - depsRemaining: num(md.deps_remaining), - currentDep: str(md.current_dep), - depsDownloadedSize: num(md.deps_downloaded_size), - depsSpeed: num(md.deps_speed), error, startedAt: Date.now(), + stageStartedAt: Date.now(), currentAction: action, }; } @@ -320,19 +226,13 @@ export function PluginInstallTaskProvider({ unknown >; - // Extract progress fields from metadata + // Download byte counts are the only measurable install progress + // the backend reports for this task. const num = (v: unknown) => (typeof v === 'number' ? v : undefined); - const str = (v: unknown) => (typeof v === 'string' ? v : undefined); const downloadCurrent = num(md.download_current); const downloadTotal = num(md.download_total); const downloadSpeed = num(md.download_speed); - const depsTotal = num(md.deps_total); - const depsInstalled = num(md.deps_installed); - const depsRemaining = num(md.deps_remaining); - const currentDep = str(md.current_dep); - const depsDownloadedSize = num(md.deps_downloaded_size); - const depsSpeed = num(md.deps_speed); setTasks((prev) => prev.map((t) => { @@ -342,13 +242,6 @@ export function PluginInstallTaskProvider({ downloadCurrent: downloadCurrent ?? t.downloadCurrent, downloadTotal: downloadTotal ?? t.downloadTotal, downloadSpeed: downloadSpeed ?? t.downloadSpeed, - depsTotal: depsTotal ?? t.depsTotal, - depsInstalled: depsInstalled ?? t.depsInstalled, - depsRemaining: depsRemaining ?? t.depsRemaining, - currentDep: currentDep ?? t.currentDep, - depsDownloadedSize: - depsDownloadedSize ?? t.depsDownloadedSize, - depsSpeed: depsSpeed ?? t.depsSpeed, }; if (done) { @@ -382,31 +275,28 @@ export function PluginInstallTaskProvider({ } const stage = mapActionToStage(action); - const [rangeStart, rangeEnd] = STAGE_PROGRESS_RANGE[stage] ?? [ - 0, 0, - ]; - // Prefer real byte counts where available; otherwise drift - // slowly inside the current stage so the bar still moves. - const elapsed = (Date.now() - t.startedAt) / 1000; - const drift = Math.min( - Math.max(0, rangeEnd - rangeStart - 1), - Math.floor(elapsed / 2), - ); + // Reset the in-stage clock whenever the reported stage moves + // so drift reflects time spent in this stage, not the whole + // installation. + const stageChanged = stage !== t.stage; + const stageStartedAt = stageChanged + ? Date.now() + : t.stageStartedAt; + const stageProgress = computeStageProgress({ + stage, + downloadCurrent, + downloadTotal, + stageElapsedSeconds: (Date.now() - stageStartedAt) / 1000, + }); const progress = Math.min( - 99, - Math.max( - t.overallProgress, - computeOverallProgress({ - stage, - downloadCurrent, - downloadTotal, - }) + drift, - ), + INSTALL_PROGRESS_CAP, + Math.max(t.overallProgress, stageProgress), ); return { ...t, stage, + stageStartedAt, overallProgress: progress, currentAction: action, ...progressFields, @@ -533,6 +423,7 @@ export function PluginInstallTaskProvider({ overallProgress: 5, fileSize: params.fileSize, startedAt: Date.now(), + stageStartedAt: Date.now(), currentAction: '', }; diff --git a/web/src/app/home/plugins/components/plugin-install-task/install-progress.ts b/web/src/app/home/plugins/components/plugin-install-task/install-progress.ts new file mode 100644 index 000000000..42fdb95d9 --- /dev/null +++ b/web/src/app/home/plugins/components/plugin-install-task/install-progress.ts @@ -0,0 +1,135 @@ +/** + * Pure install-stage model shared by the install-task UI. + * + * Kept free of React imports so the mapping and progress maths can be + * exercised directly in unit tests. + */ + +/** + * Installation stages mapped from backend current_action strings. + */ +export enum InstallStage { + DOWNLOADING = 'downloading', + INSTALLING_DEPS = 'installing_deps', + INITIALIZING = 'initializing', + LAUNCHING = 'launching', + DONE = 'done', + ERROR = 'error', +} + +/** + * Map the backend `current_action` string to an InstallStage. + * + * The runtime connector emits human-readable stage strings; each branch here + * matches the wording produced by the connector so newly added stages show up + * in the UI without a protocol change. + */ +export function mapActionToStage(action: string): InstallStage { + const lower = (action || '').toLowerCase(); + + // Terminal wording first: "installed" would otherwise also match the + // in-progress "installing" branch below. + if (lower.includes('installed') || lower.includes('complete')) { + return InstallStage.DONE; + } + // "waiting for plugin to become ready" is the post-install readiness wait, + // checked before the stage branches so "ready" is not read as "done". + if (lower.includes('waiting') || lower.includes('ready')) { + return InstallStage.LAUNCHING; + } + // Pre-download wording, checked before the "install" branches because + // "preparing plugin install" also contains "install". + if (lower.includes('prepar') || lower.includes('checking')) { + return InstallStage.DOWNLOADING; + } + if (lower.includes('download')) return InstallStage.DOWNLOADING; + + // The runtime installs the plugin's dependencies and starts it in a single + // step ("installing or starting plugin"), and persisting the installation + // precedes it. None of these stream finer-grained progress, so they all map + // to one honest stage rather than pretending to be a separate dependency + // step. This is checked before the generic "launch"/"start" branch, which + // would otherwise catch the "...or starting..." wording. + if ( + lower.includes('installing') || + lower.includes('starting') || + lower.includes('persisting') || + lower.includes('storing') || + lower.includes('inspect') + ) { + return InstallStage.INSTALLING_DEPS; + } + if (lower.includes('launch')) return InstallStage.LAUNCHING; + if (lower.includes('initializ') || lower.includes('configur')) { + return InstallStage.INITIALIZING; + } + + return InstallStage.DOWNLOADING; +} + +/** + * Progress range (start → end) attributed to each stage, used to build a + * smooth determinate bar that never goes backwards. The ranges are contiguous + * and non-overlapping, so progress never has to move backwards when the stage + * advances. + */ +export const STAGE_PROGRESS_RANGE: Record = { + [InstallStage.DOWNLOADING]: [5, 45], + [InstallStage.INSTALLING_DEPS]: [45, 85], + [InstallStage.INITIALIZING]: [85, 88], + [InstallStage.LAUNCHING]: [88, 97], + [InstallStage.DONE]: [100, 100], + [InstallStage.ERROR]: [0, 0], +}; + +/** Progress never reaches 100 until the backend reports the task as done. */ +export const INSTALL_PROGRESS_CAP = 99; + +function clampToRange(value: number, start: number, end: number): number { + return Math.min(end, Math.max(start, value)); +} + +export interface StageProgressInput { + stage: InstallStage; + downloadCurrent?: number; + downloadTotal?: number; + /** Seconds spent in the current stage, used to bound fallback drift. */ + stageElapsedSeconds: number; +} + +/** + * Progress contributed by a single stage, always inside that stage's range. + * + * Real byte counts take priority: when the backend has reported a download + * size, the measured ratio is authoritative and no time-based drift is added + * on top of it. Drift is only a fallback for stages that report no measurable + * progress, and it is clamped to the current stage so it can never spill into + * a later stage's range. + */ +export function computeStageProgress(input: StageProgressInput): number { + const [start, end] = STAGE_PROGRESS_RANGE[input.stage] ?? [0, 0]; + + const hasMeasuredBytes = + input.stage === InstallStage.DOWNLOADING && + input.downloadTotal != null && + input.downloadTotal > 0 && + input.downloadCurrent != null; + + if (hasMeasuredBytes) { + const ratio = Math.min( + 1, + (input.downloadCurrent as number) / (input.downloadTotal as number), + ); + return clampToRange(Math.round(start + (end - start) * ratio), start, end); + } + + // Nothing measurable to show yet: drift slowly, but never past this stage's + // own ceiling (hence `end - start - 1`, leaving the final point to the real + // stage transition). + const maxDrift = Math.max(0, end - start - 1); + const drift = Math.min( + maxDrift, + Math.floor(Math.max(0, input.stageElapsedSeconds) / 2), + ); + return clampToRange(start + drift, start, end); +} diff --git a/web/src/app/home/plugins/components/plugin-installed/PluginInstalledComponent.tsx b/web/src/app/home/plugins/components/plugin-installed/PluginInstalledComponent.tsx index fb429b0a4..95867c50b 100644 --- a/web/src/app/home/plugins/components/plugin-installed/PluginInstalledComponent.tsx +++ b/web/src/app/home/plugins/components/plugin-installed/PluginInstalledComponent.tsx @@ -21,8 +21,7 @@ import { extractI18nObject } from '@/i18n/I18nProvider'; import { toast } from 'sonner'; import { useAsyncTask, AsyncTaskStatus } from '@/hooks/useAsyncTask'; import { useSidebarData } from '@/app/home/components/home-sidebar/SidebarDataContext'; -import { Input } from '@/components/ui/input'; -import { Loader2, Puzzle, Search, Server, Sparkles, X } from 'lucide-react'; +import { Loader2, Puzzle, Search, Server, Sparkles } from 'lucide-react'; export interface PluginInstalledComponentRef { refreshPluginList: () => void; diff --git a/web/src/app/home/plugins/components/plugin-market/PluginMarketComponent.tsx b/web/src/app/home/plugins/components/plugin-market/PluginMarketComponent.tsx index bcfcbb455..eca52de98 100644 --- a/web/src/app/home/plugins/components/plugin-market/PluginMarketComponent.tsx +++ b/web/src/app/home/plugins/components/plugin-market/PluginMarketComponent.tsx @@ -44,10 +44,8 @@ import { } from '@/components/ui/tooltip'; import PluginMarketCardComponent from './plugin-market-card/PluginMarketCardComponent'; import { PluginMarketCardVO } from './plugin-market-card/PluginMarketCardVO'; -import { - resolveInstalledState, - useMarketplaceInstalledIndex, -} from './marketplace-installed'; +import { resolveInstalledState } from './marketplace-installed'; +import { useMarketplaceInstalledIndex } from './useMarketplaceInstalledIndex'; import { RecommendationLists } from './RecommendationLists'; import type { RecommendationList } from './RecommendationLists'; import { diff --git a/web/src/app/home/plugins/components/plugin-market/RecommendationLists.tsx b/web/src/app/home/plugins/components/plugin-market/RecommendationLists.tsx index cbc9804b5..6f8dbfe11 100644 --- a/web/src/app/home/plugins/components/plugin-market/RecommendationLists.tsx +++ b/web/src/app/home/plugins/components/plugin-market/RecommendationLists.tsx @@ -9,8 +9,8 @@ import { extractI18nObject } from '@/i18n/I18nProvider'; import { getCloudServiceClientSync } from '@/app/infra/http'; import { useTranslation } from 'react-i18next'; import { - InstalledExtensionEntry, resolveInstalledState, + type InstalledExtensionEntry, } from './marketplace-installed'; export interface RecommendationList { diff --git a/web/src/app/home/plugins/components/plugin-market/marketplace-installed.ts b/web/src/app/home/plugins/components/plugin-market/marketplace-installed.ts index 59c01ef29..27d25da46 100644 --- a/web/src/app/home/plugins/components/plugin-market/marketplace-installed.ts +++ b/web/src/app/home/plugins/components/plugin-market/marketplace-installed.ts @@ -1,16 +1,25 @@ -import { useMemo } from 'react'; -import { useSidebarData } from '@/app/home/components/home-sidebar/SidebarDataContext'; - /** - * Marketplace extensions are addressed as `author/name`, while installed - * extensions in the sidebar use slightly different identities per kind: + * Marketplace extensions are addressed as `author/name`. Installed extensions + * only carry a publisher-scoped identity for plugins and MCP servers: * - plugins: `author/name` * - MCP servers: `author__name` (double underscore) - * - skills: the bare skill name + * - skills: the bare skill name, with no publisher recorded * - * The index below normalises all of them to a single `type:author/name` shape - * so a marketplace card can be matched with one lookup. + * The index below normalises those to a single `type:author/name` shape so a + * marketplace card can be matched with one lookup. + * + * Skills are deliberately *not* indexable: the backend derives a skill's name + * from the `name` field in its own SKILL.md (falling back to the package + * directory name), so a skill published by `alice/review` and one published by + * `bob/review` both install as the plain name `review`. Matching on that bare + * name would mark every publisher's `review` as installed once any single one + * of them is. Until the installed skill carries its publisher, a skill card + * cannot be resolved authoritatively, and so is reported as not installed. + * + * This module is intentionally free of React imports so it can be unit tested + * directly; the reactive hook lives in `useMarketplaceInstalledIndex.ts`. */ + export interface InstalledExtensionEntry { /** An installed extension of the same identity has a newer remote version. */ hasUpdate: boolean; @@ -30,11 +39,18 @@ export function installedExtensionKey( return `${type || 'plugin'}:${author}/${name}`; } +/** Split an `author/name` identity, tolerating a missing author. */ +function splitIdentity(identity: string): [string, string] { + const slash = identity.indexOf('/'); + if (slash < 0) return ['', identity]; + return [identity.slice(0, slash), identity.slice(slash + 1)]; +} + /** * Build the installed-extension lookup from the sidebar entity lists. * - * Skills are indexed under both the bare name and the `author/name` form so a - * marketplace skill card resolves regardless of how it was published. + * Skills are intentionally omitted — see the module header for why a bare + * skill name cannot be attributed to a publisher. */ export function buildInstalledIndex( plugins: { id: string; hasUpdate?: boolean }[], @@ -60,28 +76,18 @@ export function buildInstalledIndex( ); } - for (const skill of skills) { - const entry: InstalledExtensionEntry = { hasUpdate: false }; - const identity = splitIdentity(skill.id); - index.set(installedExtensionKey('skill', ...identity), entry); - // Skills are stored under their bare name but marketplace cards always - // carry `author/name`, so also index the name-only form. - index.set(`skill:${skill.id}`, entry); - } + // `skills` is accepted for call-site symmetry (and so the surrounding + // useMemo still re-runs when the list changes) but contributes nothing. + void skills; return index; } -/** Split an `author/name` identity, tolerating a missing author. */ -function splitIdentity(identity: string): [string, string] { - const slash = identity.indexOf('/'); - if (slash < 0) return ['', identity]; - return [identity.slice(0, slash), identity.slice(slash + 1)]; -} - /** * Resolve whether a marketplace extension is already installed. * + * Matching requires the full `type:author/name` identity, so a card is only + * marked installed when the installed extension carries the same publisher. * Unknown types fall back to `plugin`, matching the marketplace defaults. */ export function resolveInstalledState( @@ -89,36 +95,13 @@ export function resolveInstalledState( extension: { type?: string; author: string; pluginName: string }, ): MarketplaceInstalledState { const type = extension.type || 'plugin'; - const candidates = [ + const entry = index.get( `${type}:${extension.author}/${extension.pluginName}`, - // Skills may be indexed under their bare name. - `${type}:${extension.pluginName}`, - ]; + ); - for (const key of candidates) { - const entry = index.get(key); - if (entry) { - return { installed: true, hasUpdate: entry.hasUpdate }; - } + if (entry) { + return { installed: true, hasUpdate: entry.hasUpdate }; } return { installed: false, hasUpdate: false }; } - -/** - * Reactive installed-extension index derived from the sidebar data context. - * - * Because the index is memoised on the sidebar lists, a finished install (which - * triggers a sidebar refresh) automatically re-evaluates the marketplace cards. - */ -export function useMarketplaceInstalledIndex(): Map< - string, - InstalledExtensionEntry -> { - const { plugins, mcpServers, skills } = useSidebarData(); - - return useMemo( - () => buildInstalledIndex(plugins, mcpServers, skills), - [plugins, mcpServers, skills], - ); -} diff --git a/web/src/app/home/plugins/components/plugin-market/useMarketplaceInstalledIndex.ts b/web/src/app/home/plugins/components/plugin-market/useMarketplaceInstalledIndex.ts new file mode 100644 index 000000000..8d675a264 --- /dev/null +++ b/web/src/app/home/plugins/components/plugin-market/useMarketplaceInstalledIndex.ts @@ -0,0 +1,24 @@ +import { useMemo } from 'react'; +import { useSidebarData } from '@/app/home/components/home-sidebar/SidebarDataContext'; +import { + buildInstalledIndex, + type InstalledExtensionEntry, +} from './marketplace-installed'; + +/** + * Reactive installed-extension index derived from the sidebar data context. + * + * Because the index is memoised on the sidebar lists, a finished install (which + * triggers a sidebar refresh) automatically re-evaluates the marketplace cards. + */ +export function useMarketplaceInstalledIndex(): Map< + string, + InstalledExtensionEntry +> { + const { plugins, mcpServers, skills } = useSidebarData(); + + return useMemo( + () => buildInstalledIndex(plugins, mcpServers, skills), + [plugins, mcpServers, skills], + ); +} diff --git a/web/tests/unit/install-progress.test.mjs b/web/tests/unit/install-progress.test.mjs new file mode 100644 index 000000000..96fae40c3 --- /dev/null +++ b/web/tests/unit/install-progress.test.mjs @@ -0,0 +1,133 @@ +import assert from 'node:assert/strict'; +import fs from 'node:fs'; +import path from 'node:path'; +import test from 'node:test'; +import { createRequire } from 'node:module'; +import { fileURLToPath } from 'node:url'; +import ts from 'typescript'; + +const currentDirectory = path.dirname(fileURLToPath(import.meta.url)); +const sourcePath = path.resolve( + currentDirectory, + '../../src/app/home/plugins/components/plugin-install-task/install-progress.ts', +); +const source = fs.readFileSync(sourcePath, 'utf8'); +const compiled = ts.transpileModule(source, { + compilerOptions: { module: ts.ModuleKind.CommonJS }, +}).outputText; +const sourceRequire = createRequire(sourcePath); +const loadedModule = { exports: {} }; +new Function('require', 'module', 'exports', compiled)( + sourceRequire, + loadedModule, + loadedModule.exports, +); + +const { + InstallStage, + INSTALL_PROGRESS_CAP, + STAGE_PROGRESS_RANGE, + computeStageProgress, + mapActionToStage, +} = loadedModule.exports; + +test('maps the connector stage strings the runtime actually emits', () => { + assert.equal( + mapActionToStage('preparing plugin install'), + InstallStage.DOWNLOADING, + '"preparing plugin install" must not be read as the dependency stage', + ); + assert.equal( + mapActionToStage('downloading plugin package'), + InstallStage.DOWNLOADING, + ); + assert.equal( + mapActionToStage('inspecting plugin package'), + InstallStage.INSTALLING_DEPS, + ); + assert.equal( + mapActionToStage('storing plugin package'), + InstallStage.INSTALLING_DEPS, + ); + assert.equal( + mapActionToStage('persisting the installation'), + InstallStage.INSTALLING_DEPS, + ); + assert.equal(mapActionToStage('launching plugin'), InstallStage.LAUNCHING); + assert.equal( + mapActionToStage('waiting for plugin to become ready'), + InstallStage.LAUNCHING, + 'the readiness wait is still an active stage, not completion', + ); +}); + +test('the combined install-and-start stage is not reported as launching', () => { + // The runtime installs dependencies and starts the plugin in one step; the + // wording contains "starting" and must not be mapped to the launch stage. + assert.equal( + mapActionToStage('installing or starting plugin'), + InstallStage.INSTALLING_DEPS, + ); +}); + +test('measured byte counts stay inside the download stage range', () => { + const [start, end] = STAGE_PROGRESS_RANGE[InstallStage.DOWNLOADING]; + + // 90 of 100 bytes is 90% of the download range, even after 40s elapsed. + const progress = computeStageProgress({ + stage: InstallStage.DOWNLOADING, + downloadCurrent: 90, + downloadTotal: 100, + stageElapsedSeconds: 40, + }); + + assert.ok( + progress >= start && progress <= end, + `expected progress within [${start}, ${end}], received ${progress}`, + ); + assert.equal( + progress, + 41, + 'drift must not be layered on top of a measured byte ratio', + ); +}); + +test('fallback drift never spills into the next stage range', () => { + for (const stage of [ + InstallStage.DOWNLOADING, + InstallStage.INSTALLING_DEPS, + InstallStage.INITIALIZING, + InstallStage.LAUNCHING, + ]) { + const [start, end] = STAGE_PROGRESS_RANGE[stage]; + const progress = computeStageProgress({ + stage, + stageElapsedSeconds: 100000, + }); + + assert.ok( + progress >= start && progress <= end, + `stage ${stage} produced ${progress}, outside [${start}, ${end}]`, + ); + assert.ok( + progress < INSTALL_PROGRESS_CAP, + `stage ${stage} must not reach the completion cap on drift alone`, + ); + } +}); + +test('a missing or zero download total falls back to bounded drift', () => { + const [start, end] = STAGE_PROGRESS_RANGE[InstallStage.DOWNLOADING]; + + const progress = computeStageProgress({ + stage: InstallStage.DOWNLOADING, + downloadCurrent: 10, + downloadTotal: 0, + stageElapsedSeconds: 100000, + }); + + assert.ok( + progress >= start && progress <= end, + `expected progress within [${start}, ${end}], received ${progress}`, + ); +}); diff --git a/web/tests/unit/marketplace-installed-state.test.mjs b/web/tests/unit/marketplace-installed-state.test.mjs new file mode 100644 index 000000000..cb3695f39 --- /dev/null +++ b/web/tests/unit/marketplace-installed-state.test.mjs @@ -0,0 +1,121 @@ +import assert from 'node:assert/strict'; +import fs from 'node:fs'; +import path from 'node:path'; +import test from 'node:test'; +import { createRequire } from 'node:module'; +import { fileURLToPath } from 'node:url'; +import ts from 'typescript'; + +const currentDirectory = path.dirname(fileURLToPath(import.meta.url)); +const sourcePath = path.resolve( + currentDirectory, + '../../src/app/home/plugins/components/plugin-market/marketplace-installed.ts', +); +const source = fs.readFileSync(sourcePath, 'utf8'); +const compiled = ts.transpileModule(source, { + compilerOptions: { module: ts.ModuleKind.CommonJS }, +}).outputText; +const sourceRequire = createRequire(sourcePath); +const loadedModule = { exports: {} }; +new Function('require', 'module', 'exports', compiled)( + sourceRequire, + loadedModule, + loadedModule.exports, +); + +const { buildInstalledIndex, resolveInstalledState, installedExtensionKey } = + loadedModule.exports; + +test('matches installed plugins by author and name', () => { + const index = buildInstalledIndex( + [{ id: 'alice/review', hasUpdate: true }], + [], + [], + ); + + assert.deepEqual( + resolveInstalledState(index, { + type: 'plugin', + author: 'alice', + pluginName: 'review', + }), + { installed: true, hasUpdate: true }, + ); + assert.deepEqual( + resolveInstalledState(index, { + type: 'plugin', + author: 'bob', + pluginName: 'review', + }), + { installed: false, hasUpdate: false }, + 'a different publisher must not match', + ); +}); + +test('normalises MCP servers from `author__name` to `author/name`', () => { + const index = buildInstalledIndex([], [{ id: 'acme__search' }], []); + + assert.equal( + resolveInstalledState(index, { + type: 'mcp', + author: 'acme', + pluginName: 'search', + }).installed, + true, + ); + assert.equal( + resolveInstalledState(index, { + type: 'mcp', + author: 'other', + pluginName: 'search', + }).installed, + false, + ); +}); + +test('does not mark skills installed from a bare name', () => { + // Two publishers ship a skill that both install as the plain name + // `review`; the sidebar records no publisher for either. + const index = buildInstalledIndex([], [], [{ id: 'review' }]); + + const alice = resolveInstalledState(index, { + type: 'skill', + author: 'alice', + pluginName: 'review', + }); + const bob = resolveInstalledState(index, { + type: 'skill', + author: 'bob', + pluginName: 'review', + }); + + assert.equal( + alice.installed, + false, + 'alice/review must not be reported installed from a bare skill name', + ); + assert.equal( + bob.installed, + false, + 'bob/review must not be reported installed from a bare skill name', + ); +}); + +test('keeps extension kinds separate for identical identities', () => { + const index = buildInstalledIndex([{ id: 'alice/toolkit' }], [], []); + + assert.equal( + resolveInstalledState(index, { + type: 'mcp', + author: 'alice', + pluginName: 'toolkit', + }).installed, + false, + 'a plugin must not mark the same-named MCP server as installed', + ); + assert.equal( + installedExtensionKey(undefined, 'alice', 'toolkit'), + 'plugin:alice/toolkit', + 'a missing type must default to plugin', + ); +});