diff --git a/app/src-tauri/src/marketplace/mod.rs b/app/src-tauri/src/marketplace/mod.rs index dd47a48..010b130 100644 --- a/app/src-tauri/src/marketplace/mod.rs +++ b/app/src-tauri/src/marketplace/mod.rs @@ -23,7 +23,8 @@ use tauri::Emitter; use tokio::sync::oneshot; use crate::models::marketplace::{ - effective_installs, CatalogItem, ItemUpdate, Marketplace, MarketplaceInstall, MarketplaceSnapshot, SyncReport, + effective_installs, CatalogItem, ItemKind, ItemUpdate, Marketplace, MarketplaceInstall, MarketplaceSnapshot, + SyncReport, }; use crate::models::{AppSettings, Project}; use catalog::{item_fingerprint, parse_catalog}; @@ -103,6 +104,18 @@ impl MarketplaceManager { .and_then(|s| s.head_commit.clone()) } + /// Each item's `invalid` reason in the in-memory snapshot, if that + /// snapshot is at `head` — without copying the items' previews. + fn invalid_reasons_at( + &self, + marketplace_id: &str, + head: &str, + ) -> Option>> { + let snapshots = self.snapshots.lock().unwrap(); + let snap = snapshots.get(marketplace_id)?; + (snap.head_commit.as_deref() == Some(head)).then(|| invalid_reasons(&snap.items)) + } + pub fn snapshot(&self, marketplace_id: &str) -> Option { self.snapshots.lock().unwrap().get(marketplace_id).cloned() } @@ -350,12 +363,21 @@ pub async fn remove_marketplace_cache(mgr: &MarketplaceManager, marketplace_id: .await; } +fn invalid_reasons(items: &[CatalogItem]) -> HashMap<(ItemKind, String), Option> { + items + .iter() + .map(|i| ((i.kind, i.key.clone()), i.invalid.clone())) + .collect() +} + /// One marketplace's side of an update check: its head, read once, and its /// cache, opened once, with trees shared across installs (PR review #10). struct UpdateCheck { head: String, repo: Option, trees: HashMap>, + /// Catalog `invalid` per item at head, read once when first needed. + invalid_at_head: Option>>, } impl UpdateCheck { @@ -365,9 +387,40 @@ impl UpdateCheck { head, repo: None, trees: HashMap::new(), + invalid_at_head: None, }) } + /// Why `inst`'s item cannot be installed at head — the rule + /// `update_marketplace_item` applies (round 2) — from the snapshot when + /// it is at head, else from the catalog parsed at head. + fn invalid_reason( + &mut self, + mgr: &MarketplaceManager, + m: &Marketplace, + inst: &MarketplaceInstall, + ) -> Option { + if self.invalid_at_head.is_none() { + let head = self.head.clone(); + let reasons = match mgr.invalid_reasons_at(&m.id, &head) { + Some(r) => r, + None => match self.tree(&git::cache_path(mgr.data_root(), &m.id), &head) { + Ok(tree) => invalid_reasons(&parse_catalog(tree)), + Err(e) => return Some(e), + }, + }; + self.invalid_at_head = Some(reasons); + } + match self + .invalid_at_head + .as_ref() + .and_then(|r| r.get(&(inst.kind, inst.key.clone()))) + { + Some(reason) => reason.clone(), + None => Some(format!("\"{}\" is no longer in \"{}\".", inst.key, m.name)), + } + } + fn tree(&mut self, repo_path: &Path, commit: &str) -> Result<&GitTree, String> { if !self.trees.contains_key(commit) { if self.repo.is_none() { @@ -428,6 +481,7 @@ pub fn compute_updates( item: inst.item_ref(), pinned: inst.commit.clone(), head: check.head.clone(), + invalid_at_head: check.invalid_reason(mgr, m, inst), }), Ok(false) => {} Err(e) => log::debug!("Update check skipped for {}: {}", inst.key, e), @@ -876,6 +930,52 @@ mod tests { assert_eq!(updates[0].head, c2); } + /// Re-review round 2: an update to a head where the item is not + /// installable is listed with the reason, since update_marketplace_item + /// would always refuse it. + #[tokio::test] + async fn an_update_to_an_invalid_version_carries_the_reason() { + let Some(fx) = GitFixture::new() else { return }; + let c1 = fx.with_all_kinds(); + fx.write( + "hooks/notify-on-stop/hook.json", + r#"{"hooks":{"PreFoo":[{"hooks":[{"type":"command","command":"x"}]}]}}"#, + ); + fx.write( + "agents/code-reviewer.md", + "---\nname: code-reviewer\ndescription: Reviews code\n---\nReview harder.\n", + ); + std::fs::remove_file(fx.dir.path().join("commands/example-command.md")).unwrap(); + let c2 = fx.commit("break the hook, tweak the agent, drop the command"); + let data = tempfile::tempdir().unwrap(); + let mut settings = settings_with(&fx.url()); + settings.global_marketplace_installs = vec![ + install(ItemKind::Hook, "notify-on-stop", &c1), + install(ItemKind::Agent, "code-reviewer", &c1), + install(ItemKind::Command, "example-command", &c1), + ]; + { + let mgr = MarketplaceManager::new(data.path().to_path_buf()); + refresh_marketplace(&mgr, &|| settings.clone(), "m1").await; + check_reasons(&compute_updates(&mgr, &settings, &[]), &c2); + } + // Same answer from the cache alone (no snapshot in memory). + let mgr = MarketplaceManager::new(data.path().to_path_buf()); + check_reasons(&compute_updates(&mgr, &settings, &[]), &c2); + } + + fn check_reasons(updates: &[ItemUpdate], head: &str) { + let reason = |key: &str| { + let u = updates.iter().find(|u| u.item.key == key).unwrap(); + assert_eq!(u.head, head); + u.invalid_at_head.clone() + }; + assert_eq!(updates.len(), 3, "{updates:?}"); + assert!(reason("notify-on-stop").unwrap().contains("PreFoo")); + assert_eq!(reason("code-reviewer"), None); + assert!(reason("example-command").unwrap().contains("no longer in")); + } + /// PR review #10: refreshing one marketplace sets only its pins, and so /// never waits on another marketplace's lock. #[tokio::test] diff --git a/app/src-tauri/src/models/marketplace.rs b/app/src-tauri/src/models/marketplace.rs index 0e6d63b..5ea4de2 100644 --- a/app/src-tauri/src/models/marketplace.rs +++ b/app/src-tauri/src/models/marketplace.rs @@ -202,6 +202,10 @@ pub struct ItemUpdate { pub item: MarketplaceItemRef, pub pinned: String, pub head: String, + /// Why the item cannot be installed at `head` (invalid there, or gone), + /// so the update would be refused; `None` when it can be applied. + #[serde(default)] + pub invalid_at_head: Option, } #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] diff --git a/app/src/components/marketplace/InstalledPane.test.tsx b/app/src/components/marketplace/InstalledPane.test.tsx index d691462..b8b5604 100644 --- a/app/src/components/marketplace/InstalledPane.test.tsx +++ b/app/src/components/marketplace/InstalledPane.test.tsx @@ -73,7 +73,7 @@ describe("InstalledPane", () => { it("badges and accepts an update for the matching install", async () => { const mp = api({ - updates: [{ item: { marketplace_id: "m1", kind: "agent", key: "rev" }, pinned: A, head: B }], + updates: [{ item: { marketplace_id: "m1", kind: "agent", key: "rev" }, pinned: A, head: B, invalid_at_head: null }], }); render(); fireEvent.click(screen.getByRole("button", { name: "Review update for rev" })); @@ -99,7 +99,7 @@ describe("InstalledPane", () => { ] as unknown as Project[], }); const mp = api({ - updates: [{ item: { marketplace_id: "m1", kind: "agent", key: "rev" }, pinned: A, head: B }], + updates: [{ item: { marketplace_id: "m1", kind: "agent", key: "rev" }, pinned: A, head: B, invalid_at_head: null }], }); render(); const global = screen.getByTestId("installed-global"); @@ -108,13 +108,30 @@ describe("InstalledPane", () => { expect(within(proj).queryByRole("button", { name: "Review update for rev" })).not.toBeInTheDocument(); }); + it("re-review round 2: an update that would be refused shows why instead of a Review button", () => { + const mp = api({ + updates: [ + { + item: { marketplace_id: "m1", kind: "agent", key: "rev" }, + pinned: A, + head: B, + invalid_at_head: 'unknown hook event "PreFoo"', + }, + ], + }); + render(); + const global = screen.getByTestId("installed-global"); + expect(within(global).queryByRole("button", { name: "Review update for rev" })).not.toBeInTheDocument(); + expect(within(global).getByText(/Update to bbbbbbbb cannot be installed: unknown hook event "PreFoo"/)).toBeInTheDocument(); + }); + it("I2: accepts the head that was reviewed even if the update list moves on", async () => { const C = "c".repeat(40); const item = { marketplace_id: "m1", kind: "agent" as const, key: "rev" }; - const mp = api({ updates: [{ item, pinned: A, head: B }] }); + const mp = api({ updates: [{ item, pinned: A, head: B, invalid_at_head: null }] }); const { rerender } = render(); fireEvent.click(screen.getByRole("button", { name: "Review update for rev" })); - rerender(); + rerender(); fireEvent.click(screen.getByRole("button", { name: "accept diff" })); await waitFor(() => expect(mp.update).toHaveBeenCalledWith(item, { type: "global" }, B)); }); diff --git a/app/src/components/marketplace/InstalledPane.tsx b/app/src/components/marketplace/InstalledPane.tsx index 5dd52fc..fb6cf43 100644 --- a/app/src/components/marketplace/InstalledPane.tsx +++ b/app/src/components/marketplace/InstalledPane.tsx @@ -100,9 +100,16 @@ export default function InstalledPane({ mp }: { mp: MarketplaceApi }) { {KIND_LABELS[i.kind].replace(/s$/, "").toLowerCase()} · {nameOf(i.marketplace_id)} · {i.commit.slice(0, 8)} {gone && Source removed} + {upd?.invalid_at_head && !gone && ( + // The update would be refused (re-review round 2), so it is + // explained rather than offered. + + Update to {upd.head.slice(0, 8)} cannot be installed: {upd.invalid_at_head} + + )}
- {upd && !gone && ( + {upd && !upd.invalid_at_head && !gone && ( diff --git a/app/src/components/settings/MarketplaceSettings.test.tsx b/app/src/components/settings/MarketplaceSettings.test.tsx index f187e4e..d8871cf 100644 --- a/app/src/components/settings/MarketplaceSettings.test.tsx +++ b/app/src/components/settings/MarketplaceSettings.test.tsx @@ -25,7 +25,19 @@ describe("MarketplaceSettings", () => { } as unknown as AppSettings, }); listMarketplaceUpdates.mockResolvedValue([ - { item: { marketplace_id: "m1", kind: "agent", key: "a" }, pinned: "a".repeat(40), head: "b".repeat(40) }, + { + item: { marketplace_id: "m1", kind: "agent", key: "a" }, + pinned: "a".repeat(40), + head: "b".repeat(40), + invalid_at_head: null, + }, + // Not applicable (re-review round 2): not counted as available. + { + item: { marketplace_id: "m1", kind: "hook", key: "h" }, + pinned: "a".repeat(40), + head: "b".repeat(40), + invalid_at_head: 'unknown hook event "PreFoo"', + }, ]); }); diff --git a/app/src/components/settings/MarketplaceSettings.tsx b/app/src/components/settings/MarketplaceSettings.tsx index 7f1dc25..5d3d56e 100644 --- a/app/src/components/settings/MarketplaceSettings.tsx +++ b/app/src/components/settings/MarketplaceSettings.tsx @@ -1,6 +1,7 @@ import { useEffect, useState } from "react"; import { useAppState } from "../../store/appState"; import { listMarketplaceUpdates } from "../../lib/tauri-commands"; +import { applicableUpdates } from "../../lib/marketplace"; import Button from "../ui/Button"; const plural = (n: number, one: string, many: string) => `${n} ${n === 1 ? one : many}`; @@ -14,7 +15,7 @@ export default function MarketplaceSettings() { let cancelled = false; listMarketplaceUpdates() .then((u) => { - if (!cancelled) setUpdateCount(u.length); + if (!cancelled) setUpdateCount(applicableUpdates(u).length); }) .catch(() => { if (!cancelled) setUpdateCount(null); diff --git a/app/src/lib/marketplace.ts b/app/src/lib/marketplace.ts index 587232e..c710e94 100644 --- a/app/src/lib/marketplace.ts +++ b/app/src/lib/marketplace.ts @@ -1,5 +1,6 @@ import type { ItemKind, + ItemUpdate, MarketplaceInstall, MarketplaceItemRef, MarketplaceSnapshot, @@ -27,6 +28,9 @@ export const KIND_LABELS: Record = { /** A marketplace is refreshed when its tab opens if the last fetch is older than this. */ export const STALE_AFTER_MS = 15 * 60 * 1000; +/** Updates that can actually be applied (not refused as invalid at head). */ +export const applicableUpdates = (updates: ItemUpdate[]) => updates.filter((u) => u.invalid_at_head === null); + export const itemRefKey = (r: MarketplaceItemRef) => `${r.marketplace_id}/${r.kind}/${r.key}`; /** Same shape as the item strings in a `SyncReport`. */ diff --git a/app/src/lib/types.ts b/app/src/lib/types.ts index e8edccf..636122d 100644 --- a/app/src/lib/types.ts +++ b/app/src/lib/types.ts @@ -357,6 +357,8 @@ export interface ItemUpdate { item: MarketplaceItemRef; pinned: string; head: string; + /** Why the item cannot be installed at `head`, so the update would be refused; null when it applies. */ + invalid_at_head: string | null; } export type FileChange = "added" | "removed" | "modified"; export interface FileDiff {