From 078c8d4f1ce76614ab2d5f0353217237afff7f3f Mon Sep 17 00:00:00 2001 From: hugocasa Date: Mon, 7 Sep 2026 12:19:50 +0200 Subject: [PATCH] fix: total local usage per integration, not per resource type name Co-Authored-By: Claude Opus 5 (1M context) --- backend/windmill-api/openapi.yaml | 12 +- backend/windmill-store/src/resources.rs | 156 +++++++++++------- .../src/lib/components/AppConnectInner.svelte | 10 +- .../flows/pickers/PickHubScript.svelte | 11 +- .../flows/pickers/PickHubScriptQuick.svelte | 9 +- .../lib/components/pickerPopularity.test.ts | 29 +++- .../src/lib/components/pickerPopularity.ts | 56 +++++-- 7 files changed, 201 insertions(+), 82 deletions(-) diff --git a/backend/windmill-api/openapi.yaml b/backend/windmill-api/openapi.yaml index 56918747d4..aeb7d27f36 100644 --- a/backend/windmill-api/openapi.yaml +++ b/backend/windmill-api/openapi.yaml @@ -8554,17 +8554,17 @@ paths: - resource_type - count - /w/{workspace}/resources/type/hub/picked: + /w/{workspace}/resources/type/hub/info: get: - summary: list the hub's most picked resource types - operationId: listHubPickedResourceTypes + summary: list what the hub knows about its resource types + operationId: listHubResourceTypeInfo tags: - resource parameters: - $ref: "#/components/parameters/WorkspaceId" responses: "200": - description: resource types ranked by hub picks, empty if the hub has no such ranking + description: each hub resource type with its integration and pick count, empty if the hub answers neither read content: application/json: schema: @@ -8574,10 +8574,14 @@ paths: properties: name: type: string + app: + description: the integration the resource type belongs to, which is not always its own name + type: string picks: type: integer required: - name + - app - picks /w/{workspace}/resources/type/hub/pick/{name}: diff --git a/backend/windmill-store/src/resources.rs b/backend/windmill-store/src/resources.rs index 3f524f0969..f68c5f0c7e 100644 --- a/backend/windmill-store/src/resources.rs +++ b/backend/windmill-store/src/resources.rs @@ -90,7 +90,7 @@ pub fn workspaced_service() -> Router { .route("/type/list", get(list_resource_types)) .route("/type/listnames", get(list_resource_types_names)) .route("/type/resource_counts", get(list_resource_counts_by_type)) - .route("/type/hub/picked", get(list_hub_picked_resource_types)) + .route("/type/hub/info", get(list_hub_resource_type_info)) .route("/type/hub/pick/{name}", post(pick_hub_resource_type)) .route("/type/get/{name}", get(get_resource_type)) .route("/type/exists/{name}", get(exists_resource_type)) @@ -2618,8 +2618,9 @@ struct HubCached { const HUB_RT_IDS_TTL: std::time::Duration = std::time::Duration::from_secs(60 * 60); const HUB_RT_PICKS_TTL: std::time::Duration = std::time::Duration::from_secs(5 * 60); -static HUB_RT_IDS: LazyLock>>>> = - LazyLock::new(|| std::sync::RwLock::new(None)); +static HUB_RT_IDS: LazyLock< + std::sync::RwLock>>>, +> = LazyLock::new(|| std::sync::RwLock::new(None)); static HUB_RT_PICKS: LazyLock>>>> = LazyLock::new(|| std::sync::RwLock::new(None)); @@ -2645,17 +2646,30 @@ fn hub_cache_put(cache: &std::sync::RwLock>>, hub_base_ur } #[derive(Deserialize)] -struct HubResourceTypeId { +struct HubResourceTypeEntry { id: i64, name: String, + app: String, } -/// Maps a resource type's name to the id the hub addresses it by. Windmill knows types by -/// name — the hub id is not stored anywhere locally — so reporting a pick has to resolve -/// one. `None` when the hub cannot be reached or does not answer with a list. -async fn hub_resource_type_ids(db: &DB, hub_base_url: &str) -> Option> { - if let Some(ids) = hub_cache_get(&HUB_RT_IDS, hub_base_url, HUB_RT_IDS_TTL) { - return Some(ids); +#[derive(Clone)] +struct HubResourceType { + id: i64, + /// The integration the type belongs to. Usually the type's own name, but not always: + /// `discord_webhook` and `discord_bot_configuration` are both `discord`, and only the + /// hub knows that. Without it a workspace holding a `discord_webhook` resource looks + /// like one that has never touched Discord. + app: String, +} + +/// What the hub knows about every published resource type, keyed by the name Windmill +/// addresses it by. `None` when the hub cannot be reached or does not answer with a list. +async fn hub_resource_types( + db: &DB, + hub_base_url: &str, +) -> Option> { + if let Some(index) = hub_cache_get(&HUB_RT_IDS, hub_base_url, HUB_RT_IDS_TTL) { + return Some(index); } let response = windmill_common::utils::http_get_from_hub( &windmill_common::utils::HTTP_CLIENT, @@ -2669,17 +2683,17 @@ async fn hub_resource_type_ids(db: &DB, hub_base_url: &str) -> Option = response - .json::>() + // Only the id and the app are kept. That listing carries every type's schema — around + // a megabyte — and neither reporting a pick nor grouping types by integration needs it. + let index: HashMap = response + .json::>() .await .ok()? .into_iter() - .map(|rt| (rt.name, rt.id)) + .map(|rt| (rt.name, HubResourceType { id: rt.id, app: rt.app })) .collect(); - hub_cache_put(&HUB_RT_IDS, hub_base_url, ids.clone()); - Some(ids) + hub_cache_put(&HUB_RT_IDS, hub_base_url, index.clone()); + Some(index) } #[derive(Serialize)] @@ -2706,10 +2720,7 @@ async fn pick_hub_resource_type( check_scopes(&authed, || "resources:write".to_string())?; let hub_base_url = (**windmill_common::HUB_BASE_URL.load()).clone(); let success = async { - let id = hub_resource_type_ids(&db, &hub_base_url) - .await? - .get(&name) - .copied()?; + let id = hub_resource_types(&db, &hub_base_url).await?.get(&name)?.id; let response = windmill_common::utils::http_get_from_hub( &windmill_common::utils::HTTP_CLIENT, &format!("{hub_base_url}/resource_types/{id}/pick"), @@ -2766,42 +2777,75 @@ mod hub_picks_tests { } } -/// The hub's own popularity ranking for resource types. Empty rather than an error when -/// the hub has no such endpoint, so the pickers reading this treat an older or private hub -/// as "no hub signal" and fall back to what the workspace itself uses. -async fn list_hub_picked_resource_types( - Extension(db): Extension, -) -> JsonResult> { - let hub_base_url = (**windmill_common::HUB_BASE_URL.load()).clone(); - if let Some(picks) = hub_cache_get(&HUB_RT_PICKS, &hub_base_url, HUB_RT_PICKS_TTL) { - return Ok(Json(picks)); - } - let picks = async { - let response = windmill_common::utils::http_get_from_hub( - &windmill_common::utils::HTTP_CLIENT, - &format!("{hub_base_url}/resource_types/picked"), - false, - Some(vec![("limit", "200".to_string())]), - Some(&db), - ) - .await - .ok()?; - if !response.status().is_success() { - return None; - } - Some( - response - .json::() - .await - .ok()? - .resource_types, - ) - } - .await - .unwrap_or_default(); +/// One hub resource type as the pickers need it. +#[derive(Serialize, Clone)] +struct HubResourceTypeInfo { + name: String, + /// The integration it belongs to, so a caller can total a workspace's resources per + /// integration rather than per type. + app: String, + picks: i64, +} - hub_cache_put(&HUB_RT_PICKS, &hub_base_url, picks.clone()); - Ok(Json(picks)) +/// What the hub knows about its resource types: which integration each belongs to, and how +/// often each has been picked. +/// +/// Empty rather than an error when the hub answers neither read, so the pickers treat an +/// older or private hub as "no hub signal" and fall back to what the workspace itself uses. +/// The two reads degrade independently: a hub that lists types but has no `picked` route +/// still supplies the type-to-integration mapping, which is what decides whether a +/// workspace's resources are recognised as belonging to an integration at all. +async fn list_hub_resource_type_info( + Extension(db): Extension, +) -> JsonResult> { + let hub_base_url = (**windmill_common::HUB_BASE_URL.load()).clone(); + let picks = match hub_cache_get(&HUB_RT_PICKS, &hub_base_url, HUB_RT_PICKS_TTL) { + Some(picks) => picks, + None => { + let fetched = async { + let response = windmill_common::utils::http_get_from_hub( + &windmill_common::utils::HTTP_CLIENT, + &format!("{hub_base_url}/resource_types/picked"), + false, + Some(vec![("limit", "200".to_string())]), + Some(&db), + ) + .await + .ok()?; + if !response.status().is_success() { + return None; + } + Some( + response + .json::() + .await + .ok()? + .resource_types, + ) + } + .await + .unwrap_or_default(); + hub_cache_put(&HUB_RT_PICKS, &hub_base_url, fetched.clone()); + fetched + } + }; + + let picks_by_name: HashMap = + picks.into_iter().map(|rt| (rt.name, rt.picks)).collect(); + let index = hub_resource_types(&db, &hub_base_url) + .await + .unwrap_or_default(); + + Ok(Json( + index + .into_iter() + .map(|(name, rt)| HubResourceTypeInfo { + picks: picks_by_name.get(&name).copied().unwrap_or(0), + name, + app: rt.app, + }) + .collect(), + )) } async fn get_resource_type( diff --git a/frontend/src/lib/components/AppConnectInner.svelte b/frontend/src/lib/components/AppConnectInner.svelte index ec49be4802..aa2d05ca1d 100644 --- a/frontend/src/lib/components/AppConnectInner.svelte +++ b/frontend/src/lib/components/AppConnectInner.svelte @@ -387,9 +387,10 @@ } /** - * Orders the browse list: what the hub sees people pick, then what this workspace already - * has resources of. Both signals are fetched, so the rows render in the order the lists - * arrived in and re-sort when this lands. + * Orders the browse list: the types this workspace already has resources of lead, ranked + * among themselves by the hub's pick counts, then everything else on the same counts. + * `byPopularity` carries the full rule. Both signals are fetched, so the rows render + * alphabetically and re-sort when this lands. */ let popularity: (a: string, b: string) => number = $state(alphabetical) @@ -998,6 +999,9 @@ if (step == 1) { loadConnects() loadResourceTypes() + // Opened on a specific type, `open()` skipped this; backing out to the browse + // list is the first time it is needed. + loadPopularity() } } diff --git a/frontend/src/lib/components/flows/pickers/PickHubScript.svelte b/frontend/src/lib/components/flows/pickers/PickHubScript.svelte index b6be224087..d84228dd35 100644 --- a/frontend/src/lib/components/flows/pickers/PickHubScript.svelte +++ b/frontend/src/lib/components/flows/pickers/PickHubScript.svelte @@ -13,7 +13,7 @@ import { alphabetical, byPopularity, - localResourceTypeCounts + localCountsByIntegration } from '$lib/components/pickerPopularity' interface Props { @@ -62,11 +62,12 @@ if ($disableHubStore) return try { hubNotAvailable = false - const integrations = await IntegrationService.listHubIntegrations({ - kind: filterKind - }) + // Independent reads, so they share one round trip before first paint. + const [integrations, local] = await Promise.all([ + IntegrationService.listHubIntegrations({ kind: filterKind }), + $workspaceStore ? localCountsByIntegration($workspaceStore) : {} + ]) const hubPicks = Object.fromEntries(integrations.map((x) => [x.name, x.picks ?? 0])) - const local = $workspaceStore ? await localResourceTypeCounts($workspaceStore) : {} popularity = byPopularity(hubPicks, local) allApps = integrations.map((x) => x.name).sort(popularity) } catch (err) { diff --git a/frontend/src/lib/components/flows/pickers/PickHubScriptQuick.svelte b/frontend/src/lib/components/flows/pickers/PickHubScriptQuick.svelte index b1bf7ff11e..d32e82c702 100644 --- a/frontend/src/lib/components/flows/pickers/PickHubScriptQuick.svelte +++ b/frontend/src/lib/components/flows/pickers/PickHubScriptQuick.svelte @@ -53,7 +53,7 @@ import { alphabetical, byPopularity, - localResourceTypeCounts + localCountsByIntegration } from '$lib/components/pickerPopularity' let customUi: undefined | FlowBuilderWhitelabelCustomUi = getContext('customUi') @@ -112,9 +112,12 @@ if ($disableHubStore) return try { hubNotAvailable = false - const integrations = await listHubIntegrationsCached({ kind: filterKind, refreshCount }) + // Independent reads, so they share one round trip before first paint. + const [integrations, local] = await Promise.all([ + listHubIntegrationsCached({ kind: filterKind, refreshCount }), + $workspaceStore ? localCountsByIntegration($workspaceStore) : {} + ]) const hubPicks = Object.fromEntries(integrations.map((x) => [x.name, x.picks ?? 0])) - const local = $workspaceStore ? await localResourceTypeCounts($workspaceStore) : {} popularity = byPopularity(hubPicks, local) allApps = integrations.map((x) => x.name).sort(popularity) } catch (err) { diff --git a/frontend/src/lib/components/pickerPopularity.test.ts b/frontend/src/lib/components/pickerPopularity.test.ts index aea93679f7..f9440981ad 100644 --- a/frontend/src/lib/components/pickerPopularity.test.ts +++ b/frontend/src/lib/components/pickerPopularity.test.ts @@ -1,5 +1,5 @@ import { describe, it, expect } from 'vitest' -import { alphabetical, byPopularity } from './pickerPopularity' +import { alphabetical, byPopularity, totalLocalCountsByApp } from './pickerPopularity' const order = (names: string[], hub: Record, local: Record = {}) => [...names].sort(byPopularity(hub, local)) @@ -51,3 +51,30 @@ describe('byPopularity', () => { expect(['stripe', 'ably', 'github'].sort(alphabetical)).toEqual(['ably', 'github', 'stripe']) }) }) + +describe('totalLocalCountsByApp', () => { + const HUB = [ + { name: 'discord_webhook', app: 'discord', picks: 0 }, + { name: 'discord_bot_configuration', app: 'discord', picks: 0 }, + { name: 'ms_teams_webhook', app: 'msteams', picks: 0 }, + { name: 'slack', app: 'slack', picks: 0 } + ] + + // The integration pickers list app names, the counts arrive keyed by resource type, and + // the two only usually agree. Without the mapping a workspace whose Discord credential is + // a `discord_webhook` never reaches the used-here tier at all. + it('totals a resource type under the integration it belongs to', () => { + expect(totalLocalCountsByApp({ discord_webhook: 2 }, HUB)).toEqual({ discord: 2 }) + }) + + it('sums the several types one integration can have', () => { + expect( + totalLocalCountsByApp({ discord_webhook: 2, discord_bot_configuration: 1 }, HUB) + ).toEqual({ discord: 3 }) + }) + + // What a workspace-made type, and an unreachable hub, both leave every entry with. + it('keeps a type the hub has no mapping for under its own name', () => { + expect(totalLocalCountsByApp({ c_acme: 1, slack: 2 }, HUB)).toEqual({ c_acme: 1, slack: 2 }) + }) +}) diff --git a/frontend/src/lib/components/pickerPopularity.ts b/frontend/src/lib/components/pickerPopularity.ts index eb759971e9..2d585ff858 100644 --- a/frontend/src/lib/components/pickerPopularity.ts +++ b/frontend/src/lib/components/pickerPopularity.ts @@ -17,13 +17,14 @@ export type PopularityCounts = Record */ const CACHE_MS = 60_000 -const hubPicksCached = createCache( - async ({ workspace }: { workspace: string }): Promise => { +type HubResourceTypeInfo = { name: string; app: string; picks: number } + +const hubInfoCached = createCache( + async ({ workspace }: { workspace: string }): Promise => { try { - const picked = await ResourceService.listHubPickedResourceTypes({ workspace }) - return Object.fromEntries(picked.map((rt) => [rt.name, rt.picks])) + return await ResourceService.listHubResourceTypeInfo({ workspace }) } catch { - return {} + return [] } }, { invalidateMs: CACHE_MS } @@ -46,20 +47,55 @@ const localCountsCached = createCache( * and on an instance that has switched the hub off — a closed environment must not spend a * request on hub.windmill.dev just to order a list. */ -export function hubResourceTypePicks(workspace: string): Promise { - if (get(disableHubStore)) return Promise.resolve({}) - return hubPicksCached({ workspace }) +export async function hubResourceTypePicks(workspace: string): Promise { + if (get(disableHubStore)) return {} + const info = await hubInfoCached({ workspace }) + return Object.fromEntries(info.map((rt) => [rt.name, rt.picks])) } /** * How many resources of each type this workspace holds — the only evidence about this - * particular team, and the same map the flow step picker reads for an integration, since an - * integration and the resource type authenticating it share a name. + * particular team. Keyed by resource type, which is what the add-resource drawer lists. */ export function localResourceTypeCounts(workspace: string): Promise { return localCountsCached({ workspace }) } +/** + * The same counts totalled per integration, which is what the flow step picker lists. + * + * A type usually shares its integration's name, but often enough it does not: + * `discord_webhook` and `discord_bot_configuration` are both Discord, `ms_teams_webhook` and + * `azure_bot` are both MS Teams. Only the hub knows that, so a workspace whose Discord + * credential is a `discord_webhook` would otherwise read as one that has never touched + * Discord — and since local usage is the leading tier, that decides which half of the list + * the integration lands in, not merely its position within one. + * + * A type the hub has no mapping for counts under its own name, which is the right guess and + * also what an unreachable hub leaves every type with. + */ +export async function localCountsByIntegration(workspace: string): Promise { + const [counts, info] = await Promise.all([ + localCountsCached({ workspace }), + get(disableHubStore) ? Promise.resolve([]) : hubInfoCached({ workspace }) + ]) + return totalLocalCountsByApp(counts, info) +} + +/** The mapping half of {@link localCountsByIntegration}, separated so it can be tested alone. */ +export function totalLocalCountsByApp( + counts: PopularityCounts, + hub: { name: string; app: string }[] +): PopularityCounts { + const appOf = new Map(hub.map((rt) => [rt.name, rt.app])) + const byApp: PopularityCounts = {} + for (const [name, count] of Object.entries(counts)) { + const app = appOf.get(name) ?? name + byApp[app] = (byApp[app] ?? 0) + count + } + return byApp +} + /** * Tell the hub a resource type was taken into a workspace, which is what its ranking counts. * Fire-and-forget: a hub that does not count picks must not be felt by the user who just