From 2bca050b32cdce43595527ea67fa03d157d50dc0 Mon Sep 17 00:00:00 2001 From: JeremyFunk Date: Wed, 29 Jul 2026 02:34:09 +0200 Subject: [PATCH 1/2] fix(alerts): make the Slack channel picker usable at scale MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The slack-bot destination's channel picker had three problems: the list was fetched once and went stale, search didn't work, and the backend truncated large workspaces at ~600 channels. Search: the Combobox was never given the root `items` prop, so Base UI did no filtering at all — every channel stayed visible while typing, and `ComboboxEmpty` (which keys off `filteredItems`) rendered unconditionally. We now rank client-side with a small scoring function (exact > prefix > word-boundary > substring > subsequence; `#` stripped, case-insensitive, member channels as the tie-break), cap the rendered rows at 50 with a "showing the closest 50 of N" footer, and hand the ranked ids back as `items` with `filter={null}` so Base UI's empty state and keyboard highlighting agree with what's on screen. Refresh: a Refresh button next to the Channel label re-fetches via `useAtomRefresh` on the stable query atom, spinning while in flight; the previousSuccess fallback keeps the list rendered so a refresh can't blank a selection mid-pick. Pagination: 200/page × 3 pages became 1000/page (Slack's documented max) × 20 pages, i.e. up to ~20k channels. The binding constraint is the Tier 2 rate limit (~20 req/min), not page latency, so the larger page is the cheaper trade; 429s are now waited out per `Retry-After` (3 attempts, clamped to 30s) instead of surfacing as an "unexpected payload" error, and hitting the page cap logs a warning rather than silently truncating. Co-Authored-By: Claude Fable 5 --- .../services/SlackIntegrationService.test.ts | 69 ++++++++++-- .../src/services/SlackIntegrationService.ts | 80 ++++++++++++-- .../components/alerts/destination-dialog.tsx | 101 +++++++++++++++--- .../alerts/slack-channel-search.test.ts | 75 +++++++++++++ .../components/alerts/slack-channel-search.ts | 101 ++++++++++++++++++ 5 files changed, 398 insertions(+), 28 deletions(-) create mode 100644 apps/web/src/components/alerts/slack-channel-search.test.ts create mode 100644 apps/web/src/components/alerts/slack-channel-search.ts diff --git a/apps/api/src/services/SlackIntegrationService.test.ts b/apps/api/src/services/SlackIntegrationService.test.ts index 9adaa258d..78bf6d97f 100644 --- a/apps/api/src/services/SlackIntegrationService.test.ts +++ b/apps/api/src/services/SlackIntegrationService.test.ts @@ -1,6 +1,6 @@ import { createCipheriv, randomBytes } from "node:crypto" import { afterEach, assert, describe, it } from "@effect/vitest" -import { ConfigProvider, Effect, Layer, Schema } from "effect" +import { ConfigProvider, Effect, Fiber, Layer, Schema } from "effect" import { TestClock } from "effect/testing" import { FetchHttpClient } from "effect/unstable/http" import { OrgId, UserId } from "@maple/domain/http" @@ -64,6 +64,9 @@ const makeLayer = ( /** Mirror of the service's (unexported) `SLACK_STATE_TTL_MS` — 10 minutes. */ const SLACK_STATE_TTL_MS = 10 * 60_000 +/** Mirror of the service's (unexported) `SLACK_MAX_CHANNEL_PAGES` runaway guard. */ +const SLACK_MAX_CHANNEL_PAGES = 20 + /** * Wrap the real ApiKeysService so `create` runs `inject` first. `apiKeys.create` * is the only seam between completeInstall's cross-org pre-check and its @@ -1048,6 +1051,7 @@ describe("SlackIntegrationService", () => { it.effect("listChannels caps pagination at SLACK_MAX_CHANNEL_PAGES pages", () => { const testDb = createTestDb(trackedDbs) + const pageUrls: string[] = [] let calls = 0 return Effect.gen(function* () { yield* Effect.promise(() => @@ -1063,17 +1067,20 @@ describe("SlackIntegrationService", () => { const slack = yield* SlackIntegrationService const channels = yield* slack.listChannels(asOrgId("org_lc")) // The mock ALWAYS hands back a next_cursor — the walk must stop at the - // page cap (SLACK_MAX_CHANNEL_PAGES = 3) instead of looping forever. - assert.strictEqual(calls, 3) - assert.deepStrictEqual( - channels.map((c) => c.id), - ["C-page-0", "C-page-1", "C-page-2"], - ) + // runaway guard (SLACK_MAX_CHANNEL_PAGES) instead of looping forever. + assert.strictEqual(calls, SLACK_MAX_CHANNEL_PAGES) + assert.strictEqual(channels.length, SLACK_MAX_CHANNEL_PAGES) + assert.strictEqual(channels[0]?.id, "C-page-0") + assert.strictEqual(channels.at(-1)?.id, `C-page-${SLACK_MAX_CHANNEL_PAGES - 1}`) + // 1000 per page (Slack's documented max) keeps a 10k-channel workspace + // inside the Tier 2 (~20 req/min) budget. + assert.strictEqual(new URL(pageUrls[0]!).searchParams.get("limit"), "1000") }).pipe( Effect.provide( withFetch( testDb, - slackApiFetch(CONVERSATIONS_URL, (_url, call) => { + slackApiFetch(CONVERSATIONS_URL, (url, call) => { + pageUrls.push(url) calls++ return jsonResponse({ ok: true, @@ -1086,6 +1093,52 @@ describe("SlackIntegrationService", () => { ) }) + it.effect("listChannels waits out a 429 and retries the same page", () => { + const testDb = createTestDb(trackedDbs) + let calls = 0 + return Effect.gen(function* () { + yield* Effect.promise(() => + insertWorkspace(testDb, { + id: "sw_lc429", + orgId: "org_lc", + teamId: "T-LC", + teamName: "LC", + botToken: "xoxb-lc-token", + apiKey: "maple_ak_lc", + }), + ) + const slack = yield* SlackIntegrationService + // `conversations.list` is Tier 2 — a 429 must back off, not surface as an + // "unexpected payload" error (Slack sends an empty body with the 429). + const fiber = yield* Effect.forkChild(slack.listChannels(asOrgId("org_lc"))) + // The fetch mock resolves on the real microtask queue while the sleep is + // on the TestClock, so alternate between draining one and advancing the + // other until the walk finishes. + for (let i = 0; i < 5; i++) { + yield* Effect.promise(() => new Promise((resolve) => setTimeout(resolve, 0))) + yield* TestClock.adjust(5_000) + } + const channels = yield* Fiber.join(fiber) + assert.strictEqual(calls, 2) + assert.deepStrictEqual( + channels.map((c) => c.id), + ["C1"], + ) + }).pipe( + Effect.provide( + withFetch( + testDb, + slackApiFetch(CONVERSATIONS_URL, (_url, call) => { + calls++ + return call === 0 + ? new Response("", { status: 429, headers: { "retry-after": "2" } }) + : jsonResponse({ ok: true, channels: [{ id: "C1", name: "one" }] }) + }), + ), + ), + ) + }) + it.effect("listChannels maps Slack ok:false to an upstream error", () => { const testDb = createTestDb(trackedDbs) return Effect.gen(function* () { diff --git a/apps/api/src/services/SlackIntegrationService.ts b/apps/api/src/services/SlackIntegrationService.ts index e1d66003a..4579d95cc 100644 --- a/apps/api/src/services/SlackIntegrationService.ts +++ b/apps/api/src/services/SlackIntegrationService.ts @@ -33,7 +33,31 @@ const SLACK_OAUTH_ACCESS_URL = "https://slack.com/api/oauth.v2.access" const SLACK_OAUTH_REVOKE_URL = "https://slack.com/api/auth.revoke" const SLACK_CONVERSATIONS_LIST_URL = "https://slack.com/api/conversations.list" const SLACK_AUTH_TEST_URL = "https://slack.com/api/auth.test" -const SLACK_MAX_CHANNEL_PAGES = 3 +/** + * Per-page size for `conversations.list`. Slack's documented maximum is 1000; + * their docs *recommend* <= 200 because very large pages are more likely to time + * out server-side. We take the max anyway and lean on the retry below, because + * the binding constraint here is the rate limit, not page latency: + * `conversations.list` is Tier 2 (~20 req/min), so at 200/page a 10k-channel + * workspace would need 50 requests — guaranteed 429s and a multi-minute wait. + * At 1000/page the same workspace is 10 requests, comfortably inside one window. + */ +const SLACK_CHANNELS_PER_PAGE = 1000 + +/** + * Safety cap on the cursor walk — a runaway `next_cursor` must not loop forever. + * At {@link SLACK_CHANNELS_PER_PAGE} this allows ~20k channels, well past the + * largest real workspaces, while still fitting the Tier 2 budget with the + * 429 backoff below. Hitting it logs a warning (the result is silently truncated + * otherwise). + */ +const SLACK_MAX_CHANNEL_PAGES = 20 + +/** Attempts per page when Slack answers 429; each waits out `Retry-After`. */ +const SLACK_RATE_LIMIT_RETRIES = 3 + +/** Upper bound on an honoured `Retry-After`, so one page can't hang the request. */ +const SLACK_MAX_RETRY_AFTER_MS = 30_000 /** Why a workspace binding was revoked without going through `uninstall`. */ export type SlackRevocationReason = "app_uninstalled" | "tokens_revoked" | "reconciliation" @@ -142,6 +166,17 @@ const DEAD_TOKEN_AUTH_TEST_ERRORS: ReadonlySet = new Set([ "token_expired", ]) +/** + * Slack's 429 `Retry-After` is whole seconds. Missing/garbage header → 1s, and + * anything longer than {@link SLACK_MAX_RETRY_AFTER_MS} is clamped so a single + * page can't hold the HTTP request open indefinitely. + */ +const retryAfterMs = (header: string | undefined): number => { + const seconds = Number.parseInt(header ?? "", 10) + if (!Number.isFinite(seconds) || seconds <= 0) return 1_000 + return Math.min(seconds * 1_000, SLACK_MAX_RETRY_AFTER_MS) +} + const SlackConversationsListSchema = Schema.Struct({ ok: Schema.Boolean, error: Schema.optionalKey(Schema.String), @@ -782,15 +817,21 @@ const make: Effect.Effect< return { uninstalled: false } }) - /** Fetch one `conversations.list` page; returns the page's channels + next cursor. */ + /** + * Fetch one `conversations.list` page; returns the page's channels + next cursor. + * + * `conversations.list` is Tier 2 (~20 req/min) and answers 429 with an empty + * body plus `Retry-After`, so a rate-limited page is retried here rather than + * being surfaced as an "unexpected payload" upstream error. + */ const fetchChannelPage = Effect.fnUntraced(function* (botToken: string, cursor: Option.Option) { const params = new URLSearchParams({ types: "public_channel,private_channel", exclude_archived: "true", - limit: "200", + limit: String(SLACK_CHANNELS_PER_PAGE), }) if (Option.isSome(cursor)) params.set("cursor", cursor.value) - const response = yield* httpClient + const request = httpClient .get(`${SLACK_CONVERSATIONS_LIST_URL}?${params.toString()}`, { headers: { authorization: `Bearer ${botToken}`, accept: "application/json" }, }) @@ -803,6 +844,24 @@ const make: Effect.Effect< }), ), ) + let response = yield* request + for (let attempt = 0; response.status === 429 && attempt < SLACK_RATE_LIMIT_RETRIES; attempt++) { + const waitMs = retryAfterMs(response.headers["retry-after"]) + yield* Effect.logWarning("Slack conversations.list rate limited — backing off", { + attempt: attempt + 1, + waitMs, + }) + yield* Effect.sleep(waitMs) + response = yield* request + } + if (response.status === 429) { + return yield* Effect.fail( + new IntegrationsUpstreamError({ + message: "Slack conversations.list is rate limited — try again in a minute", + status: 429, + }), + ) + } const json = yield* response.json.pipe( Effect.mapError( (error) => @@ -843,7 +902,11 @@ const make: Effect.Effect< }) /** - * Cursor-driven page walk, capped at {@link SLACK_MAX_CHANNEL_PAGES} pages. + * Cursor-driven page walk that runs to cursor exhaustion, with + * {@link SLACK_MAX_CHANNEL_PAGES} as a runaway guard rather than an intended + * limit. Truncation is logged: a workspace that trips the cap gets a silently + * short channel list, which is exactly the failure mode we can't see from the + * UI. */ const collectChannelPages = Effect.fnUntraced(function* (botToken: string) { const collected: Array = [] @@ -851,9 +914,14 @@ const make: Effect.Effect< for (let page = 0; page < SLACK_MAX_CHANNEL_PAGES; page++) { const { channels, next } = yield* fetchChannelPage(botToken, cursor) collected.push(...channels) - if (Option.isNone(next)) break + if (Option.isNone(next)) return collected cursor = next } + yield* Effect.logWarning("Slack channel list truncated at the page cap", { + pages: SLACK_MAX_CHANNEL_PAGES, + perPage: SLACK_CHANNELS_PER_PAGE, + channels: collected.length, + }) return collected }) diff --git a/apps/web/src/components/alerts/destination-dialog.tsx b/apps/web/src/components/alerts/destination-dialog.tsx index 4ec7e874e..5ac4d6258 100644 --- a/apps/web/src/components/alerts/destination-dialog.tsx +++ b/apps/web/src/components/alerts/destination-dialog.tsx @@ -10,7 +10,14 @@ import { ProviderLogo, type DestinationProvider, } from "@/components/alerts/destination-provider" -import { ArrowRightIcon, CircleInfoIcon, HazelIcon, LoaderIcon } from "@/components/icons" +import { + ArrowRightIcon, + ArrowRotateClockwiseIcon, + CircleInfoIcon, + HazelIcon, + LoaderIcon, +} from "@/components/icons" +import { CHANNEL_RESULT_LIMIT, rankChannels } from "@/components/alerts/slack-channel-search" import { MapleApiAtomClient } from "@/lib/services/common/atom-client" import { MapleApiV2AtomClient } from "@/lib/services/common/v2-atom-client" import { v2ErrorInfo } from "@/lib/error-messages" @@ -20,7 +27,7 @@ import type { HazelChannelsListResponse } from "@maple/domain/http" import type { V2SlackChannelList } from "@maple/domain/http/v2" import { Exit, Option } from "effect" import { Link } from "@tanstack/react-router" -import { useEffect, useState } from "react" +import { useEffect, useMemo, useState } from "react" import { Button } from "@maple/ui/components/ui/button" import { Dialog, @@ -597,11 +604,33 @@ function SlackBotFields({ Option.map(channelsResult.previousSuccess, (previous) => [...previous.value.channels]), ) : null - const channels = Result.builder(channelsResult) - .onSuccess((c) => [...c.channels]) - .orElse(() => previousChannels ?? ([] as V2SlackChannelList["channels"][number][])) + // Memoised on the Result so the array identity is stable between renders — + // ranking below keys off it, and a workspace can return thousands of rows. + const channels = useMemo( + () => + Result.builder(channelsResult) + .onSuccess((c) => [...c.channels]) + .orElse(() => previousChannels ?? ([] as V2SlackChannelList["channels"][number][])), + // eslint-disable-next-line react-hooks/exhaustive-deps + [channelsResult], + ) const channelsLoading = installed && channelsResult.waiting + // The Combobox's own filtering never ran here (Base UI only filters when the + // root is given `items`), so every channel stayed visible no matter what was + // typed. We rank and cap the list ourselves instead — see + // `slack-channel-search.ts` — and hand the result back as `items` so the + // popup's empty state and keyboard highlighting agree with what's rendered. + const [channelQuery, setChannelQuery] = useState("") + // Picking a channel makes Base UI write its label into the input; that is a + // selection, not a search, and must not narrow the list to one row. + const selectedLabel = channels.find((c) => c.id === form.slackChannelId)?.name + const searchQuery = + selectedLabel !== undefined && channelQuery.replace(/^#/, "") === selectedLabel ? "" : channelQuery + const visibleChannels = useMemo(() => rankChannels(channels, searchQuery), [channels, searchQuery]) + const visibleChannelIds = useMemo(() => visibleChannels.map((c) => c.id), [visibleChannels]) + const truncated = visibleChannels.length < channels.length + // `GET /v2/integrations/slack/channels` is admin-gated (`requireAdmin`), so a // regular member gets a 403 that no amount of retrying will clear. The v2 // envelope ({ error: { type, code, message } }) survives into the Result's @@ -726,18 +755,44 @@ function SlackBotFields({ return (
-
+
- {storedChannelName ? ( - - Currently #{storedChannelName} - - ) : null} +
+ {storedChannelName ? ( + + Currently #{storedChannelName} + + ) : null} + {/* The list is fetched once per dialog and cached by reactivity key, so + a channel created in Slack a minute ago isn't in it. Re-fetch in + place: `refreshChannels` pokes the same atom, and the previous + success keeps rendering, so the list never blanks mid-pick. */} + +
setChannelQuery(value)} itemToStringLabel={(value: string) => label(value)} onValueChange={(value) => { if (value == null) return @@ -757,9 +812,17 @@ function SlackBotFields({ className="w-full" /> - No matching channels. + {/* "Nothing matched" and "nothing loaded yet" are different problems + with different next steps — don't collapse them into one line. */} + + {channelsLoading + ? "Loading channels…" + : channels.length === 0 + ? "No channels loaded yet." + : "No matching channels."} + - {channels.map((channel) => ( + {visibleChannels.map((channel) => ( #{channel.name} @@ -777,6 +840,15 @@ function SlackBotFields({ ))} + {/* Big workspaces run to thousands of channels; rendering them all is + both slow and useless. Say that the list is a top-N so an absent + channel reads as "keep typing", not "we don't have it". */} + {truncated ? ( +

+ Showing the closest {CHANNEL_RESULT_LIMIT} of {channels.length} channels — keep + typing to narrow. +

+ ) : null} {channelsFailed ? ( @@ -788,7 +860,8 @@ function SlackBotFields({
) : channels.length === 0 && !channelsLoading ? (

- No channels returned. Make sure the Maple bot has been added to at least one channel. + No channels returned. Make sure the Maple bot has been added to at least one channel, + then hit Refresh.

) : null}

diff --git a/apps/web/src/components/alerts/slack-channel-search.test.ts b/apps/web/src/components/alerts/slack-channel-search.test.ts new file mode 100644 index 000000000..4fa11120e --- /dev/null +++ b/apps/web/src/components/alerts/slack-channel-search.test.ts @@ -0,0 +1,75 @@ +import { describe, expect, it } from "vitest" +import { CHANNEL_RESULT_LIMIT, rankChannels, scoreChannelName } from "./slack-channel-search" + +const channel = (name: string, is_member = true) => ({ name, is_member }) +const names = (list: ReadonlyArray<{ name: string }>) => list.map((c) => c.name) + +describe("scoreChannelName", () => { + it("orders exact > prefix > word-boundary > substring > subsequence", () => { + const scores = [ + scoreChannelName("alerts", "alerts"), + scoreChannelName("alerts-prod", "alerts"), + scoreChannelName("deploy-alerts", "alerts"), + scoreChannelName("stalerts", "alerts"), + scoreChannelName("a-long-eventful-run-tests", "alerts"), + ] as Array + expect(scores.every((s) => s !== null)).toBe(true) + for (let i = 1; i < scores.length; i++) expect(scores[i]!).toBeLessThan(scores[i - 1]!) + }) + + it("ignores a typed # and is case-insensitive", () => { + expect(scoreChannelName("General", "#gen")).toBe(scoreChannelName("general", "GEN")) + expect(scoreChannelName("general", "#general")).toBe(scoreChannelName("general", "general")) + }) + + it("returns null when the characters aren't all present in order", () => { + expect(scoreChannelName("deploy-alerts", "zzz")).toBeNull() + // Right characters, wrong order — subsequence is ordered on purpose. + expect(scoreChannelName("alerts", "strela")).toBeNull() + }) + + it("scores an empty query as a neutral match rather than no match", () => { + expect(scoreChannelName("anything", "")).toBe(0) + expect(scoreChannelName("anything", " ")).toBe(0) + }) + + it("prefers the shorter name among prefix matches", () => { + expect(scoreChannelName("dep", "dep")).toBeGreaterThan(scoreChannelName("dep-a-very-long", "dep")!) + }) +}) + +describe("rankChannels", () => { + it("puts a fuzzy typo's intended channel first", () => { + const channels = [ + channel("random"), + channel("design-eng"), + channel("deploy-alerts"), + channel("deploys"), + ] + expect(names(rankChannels(channels, "depaler"))[0]).toBe("deploy-alerts") + }) + + it("shows member channels first, then alphabetical, when the query is empty", () => { + const channels = [channel("zulu"), channel("beta", false), channel("alpha", false), channel("mike")] + expect(names(rankChannels(channels, ""))).toEqual(["mike", "zulu", "alpha", "beta"]) + }) + + it("caps the result count", () => { + const channels = Array.from({ length: 500 }, (_, i) => channel(`chan-${i}`)) + expect(rankChannels(channels, "").length).toBe(CHANNEL_RESULT_LIMIT) + expect(rankChannels(channels, "chan").length).toBe(CHANNEL_RESULT_LIMIT) + expect(rankChannels(channels, "chan", 5).length).toBe(5) + }) + + it("drops non-matching channels entirely", () => { + const channels = [channel("alerts"), channel("random"), channel("deploys")] + expect(names(rankChannels(channels, "alert"))).toEqual(["alerts"]) + expect(rankChannels(channels, "qqqq")).toEqual([]) + }) + + it("ranks a private/non-member channel by relevance, not membership, when searching", () => { + // Membership is only the tie-breaker — a much better name match wins. + const channels = [channel("alerts-archive"), channel("alerts", false)] + expect(names(rankChannels(channels, "alerts"))[0]).toBe("alerts") + }) +}) diff --git a/apps/web/src/components/alerts/slack-channel-search.ts b/apps/web/src/components/alerts/slack-channel-search.ts new file mode 100644 index 000000000..33944215b --- /dev/null +++ b/apps/web/src/components/alerts/slack-channel-search.ts @@ -0,0 +1,101 @@ +/** + * Ranking for the Slack channel picker. + * + * The picker holds every channel a workspace has (tens of thousands is a real + * shape), so search has to be both forgiving — people type `depaler` for + * `deploy-alerts` — and cheap enough to run on every keystroke over the whole + * list. Scoring is a single pass per channel with no allocation beyond the + * lowercased name, and the caller renders only the top {@link CHANNEL_RESULT_LIMIT}. + */ + +/** The shape this module needs; the wire type `V2SlackChannel` is a superset. */ +export interface RankableChannel { + readonly name: string + readonly is_member: boolean +} + +/** How many rows the picker renders. Beyond this the list is noise, not choice. */ +export const CHANNEL_RESULT_LIMIT = 50 + +/** + * Slack channel names are already lowercase, but the list also carries display + * names from older workspaces, and users type `#` out of habit. Normalise both + * sides the same way so neither can miss a match. + */ +const normalize = (value: string): string => value.trim().toLowerCase().replace(/^#+/, "") + +/** Characters that start a new "word" inside a channel name. */ +const isBoundary = (char: string): boolean => char === "-" || char === "_" || char === "." || char === " " + +/** + * Subsequence match: every query char appears in order. Returns a penalty + * proportional to how scattered the match is (0 = contiguous), or `null` when + * the chars don't all appear. + */ +const subsequencePenalty = (name: string, query: string): number | null => { + let nameIndex = 0 + let gaps = 0 + let firstIndex = -1 + for (const char of query) { + const found = name.indexOf(char, nameIndex) + if (found === -1) return null + if (firstIndex === -1) firstIndex = found + else gaps += found - nameIndex + nameIndex = found + 1 + } + return firstIndex + gaps +} + +/** + * Score a channel name against a query. Higher is better; `null` means no match. + * + * The tiers are deliberately far apart so a weaker match can never outrank a + * stronger one on length alone: exact > prefix > word-boundary prefix > + * substring > subsequence. Within a tier, earlier and shorter wins. + */ +export const scoreChannelName = (name: string, query: string): number | null => { + const haystack = normalize(name) + const needle = normalize(query) + if (needle.length === 0) return 0 + if (haystack === needle) return 10_000 + if (haystack.startsWith(needle)) return 9_000 - Math.min(haystack.length, 500) + + const index = haystack.indexOf(needle) + if (index !== -1) { + // `deploy-alerts` should surface for `alerts` ahead of `stale-alerting`: + // a match that starts a word beats one that lands mid-word. + const atWordStart = isBoundary(haystack[index - 1] ?? "") + const base = atWordStart ? 8_000 : 7_000 + return base - Math.min(index, 400) - Math.min(haystack.length, 500) / 100 + } + + const penalty = subsequencePenalty(haystack, needle) + if (penalty === null) return null + return 6_000 - Math.min(penalty, 500) - Math.min(haystack.length, 500) / 100 +} + +/** + * Filter + rank + cap the channel list for a query. + * + * With an empty query this is the "just opened the picker" view: channels the + * bot is already in first (those are the ones that will actually work without + * an `/invite`), then alphabetical. Ties at equal score resolve the same way, + * so ranking never reshuffles arbitrarily between keystrokes. + */ +export const rankChannels = ( + channels: ReadonlyArray, + query: string, + limit: number = CHANNEL_RESULT_LIMIT, +): Array => { + const scored: Array<{ channel: T; score: number }> = [] + for (const channel of channels) { + const score = scoreChannelName(channel.name, query) + if (score !== null) scored.push({ channel, score }) + } + scored.sort((a, b) => { + if (b.score !== a.score) return b.score - a.score + if (a.channel.is_member !== b.channel.is_member) return a.channel.is_member ? -1 : 1 + return a.channel.name.localeCompare(b.channel.name) + }) + return scored.slice(0, Math.max(limit, 0)).map((entry) => entry.channel) +} From 1837a30b62c9b319408ff99640994f76642ae2a6 Mon Sep 17 00:00:00 2001 From: JeremyFunk Date: Wed, 29 Jul 2026 05:17:14 +0200 Subject: [PATCH 2/2] fix(alerts): bound the Slack channel walk and fix picker selection state MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-ups on the channel picker. Frontend: selecting a *private* channel emptied the picker. Base UI writes the item's label into the input on selection, and the "is this a selection or a search" check compared that against the bare channel name — but a private channel's label carries a " (private)" suffix, so the check missed, the label stayed in `searchQuery`, and ranking found nothing. Selection detection now uses the same `channelLabel()` the popup renders (plus the plain `#name` / `name` forms). The truncation banner also lied: `visible.length < channels.length` is true for *any* narrowing, so filtering 60 channels to 3 claimed "showing the closest 50 of 60" — and it rendered under "No matching channels." too. We now rank one row past the cap so hitting the cap is distinguishable from "exactly 50 matched", and the message no longer prints the workspace total as a match count. The selected channel is pulled into the unfiltered view so the Combobox can render its own checkmark past the cap, and the input gets a clear button so a non-matching query isn't a dead end after Escape. Backend: nothing bounded the total request time. `SLACK_MAX_RETRY_AFTER_MS` bounds one sleep; 20 pages x 4 attempts x a 60s Retry-After is over half an hour, while Cloudflare's edge gives up at ~100s — the Worker kept sleeping and re-fetching for a client that was long gone. The walk now runs under a 45s wall-clock budget and returns the pages it collected (a partial picker beats a failed one), logging the existing truncation warning. With that in place the Retry-After clamp rises to 60s: `conversations.list` is Tier 2 with a 60s window, so clamping to 30s guaranteed the retry re-429'd and burned an attempt. Non-2xx responses are now handled explicitly — a Slack 5xx or a proxy's HTML error page fell through to `response.json` and surfaced as the unhelpful "returned a non-JSON response"; 5xx is retried within the budget. Tests: the 429 test drove the TestClock a fixed number of turns, so its failure mode was `Fiber.join` blocking to the vitest timeout rather than an assertion — it is now condition-driven via a shared `runInterleaved` helper. Adds coverage for the budget, the 5xx paths, selection echo, and the truncation flag, and fixes a search test that claimed to compare two prefix matches but compared an exact match against a prefix. Co-Authored-By: Claude Fable 5 --- .../services/SlackIntegrationService.test.ts | 178 +++++++++++++++++- .../src/services/SlackIntegrationService.ts | 100 ++++++++-- .../components/alerts/destination-dialog.tsx | 41 ++-- .../alerts/slack-channel-search.test.ts | 100 +++++++++- .../components/alerts/slack-channel-search.ts | 79 ++++++++ 5 files changed, 458 insertions(+), 40 deletions(-) diff --git a/apps/api/src/services/SlackIntegrationService.test.ts b/apps/api/src/services/SlackIntegrationService.test.ts index 78bf6d97f..7fa06a9f2 100644 --- a/apps/api/src/services/SlackIntegrationService.test.ts +++ b/apps/api/src/services/SlackIntegrationService.test.ts @@ -67,6 +67,45 @@ const SLACK_STATE_TTL_MS = 10 * 60_000 /** Mirror of the service's (unexported) `SLACK_MAX_CHANNEL_PAGES` runaway guard. */ const SLACK_MAX_CHANNEL_PAGES = 20 +/** Mirror of the service's (unexported) `SLACK_CHANNEL_WALK_BUDGET_MS`. */ +const SLACK_CHANNEL_WALK_BUDGET_MS = 45_000 + +/** + * Run an effect that interleaves real-promise fetch mocks with TestClock sleeps. + * + * The fetch mock resolves on the real microtask queue while the backoff sleeps + * sit on the TestClock, so neither drains the other and both have to be pumped. + * Pump until the fiber actually finishes rather than for a fixed number of + * turns: a fixed count is coupled to how many macrotask turns PGlite happens to + * take, and when it guesses low the failure mode is `Fiber.join` blocking to the + * vitest timeout instead of a readable assertion. Running out of steps fails + * loudly here instead. + */ +const runInterleaved = ( + effect: Effect.Effect, + options?: { readonly stepMs?: number; readonly maxSteps?: number }, +) => + Effect.gen(function* () { + const stepMs = options?.stepMs ?? 1_000 + const maxSteps = options?.maxSteps ?? 400 + let settled = false + const fiber = yield* Effect.forkChild( + effect.pipe( + Effect.onExit(() => + Effect.sync(() => { + settled = true + }), + ), + ), + ) + for (let step = 0; step < maxSteps && !settled; step++) { + yield* Effect.promise(() => new Promise((resolve) => setTimeout(resolve, 0))) + if (!settled) yield* TestClock.adjust(stepMs) + } + assert.isTrue(settled, `effect did not settle within ${maxSteps} interleaved steps`) + return yield* Fiber.join(fiber) + }) + /** * Wrap the real ApiKeysService so `create` runs `inject` first. `apiKeys.create` * is the only seam between completeInstall's cross-org pre-check and its @@ -1110,15 +1149,7 @@ describe("SlackIntegrationService", () => { const slack = yield* SlackIntegrationService // `conversations.list` is Tier 2 — a 429 must back off, not surface as an // "unexpected payload" error (Slack sends an empty body with the 429). - const fiber = yield* Effect.forkChild(slack.listChannels(asOrgId("org_lc"))) - // The fetch mock resolves on the real microtask queue while the sleep is - // on the TestClock, so alternate between draining one and advancing the - // other until the walk finishes. - for (let i = 0; i < 5; i++) { - yield* Effect.promise(() => new Promise((resolve) => setTimeout(resolve, 0))) - yield* TestClock.adjust(5_000) - } - const channels = yield* Fiber.join(fiber) + const channels = yield* runInterleaved(slack.listChannels(asOrgId("org_lc"))) assert.strictEqual(calls, 2) assert.deepStrictEqual( channels.map((c) => c.id), @@ -1139,6 +1170,135 @@ describe("SlackIntegrationService", () => { ) }) + it.effect("listChannels stops at the overall time budget and returns what it collected", () => { + const testDb = createTestDb(trackedDbs) + let calls = 0 + return Effect.gen(function* () { + yield* Effect.promise(() => + insertWorkspace(testDb, { + id: "sw_lcbudget", + orgId: "org_lc", + teamId: "T-LC", + teamName: "LC", + botToken: "xoxb-lc-token", + apiKey: "maple_ak_lc", + }), + ) + const slack = yield* SlackIntegrationService + // Page 1 is rate limited for a full Tier 2 window (60s), which is longer + // than the whole walk is allowed to take. The walk must give up at the + // budget and hand back page 0 rather than sleeping past Cloudflare's + // ~100s edge cutoff on a request nobody is listening to any more. + const channels = yield* runInterleaved(slack.listChannels(asOrgId("org_lc")), { + maxSteps: Math.ceil(SLACK_CHANNEL_WALK_BUDGET_MS / 1_000) + 30, + }) + assert.deepStrictEqual( + channels.map((c) => c.id), + ["C1"], + ) + // Exactly two: the successful page and the 429. A third would mean the + // 60s `Retry-After` had been clamped below the budget and retried — + // `conversations.list` is Tier 2, so 60 is the honest wait. + assert.strictEqual(calls, 2) + }).pipe( + Effect.provide( + withFetch( + testDb, + slackApiFetch(CONVERSATIONS_URL, (_url, call) => { + calls++ + if (call === 0) { + return jsonResponse({ + ok: true, + channels: [{ id: "C1", name: "one" }], + response_metadata: { next_cursor: "cursor-2" }, + }) + } + if (call === 1) { + return new Response("", { status: 429, headers: { "retry-after": "60" } }) + } + return jsonResponse({ ok: true, channels: [{ id: "C2", name: "two" }] }) + }), + ), + ), + ) + }) + + it.effect("listChannels retries a Slack 5xx and reports the status when it persists", () => { + const testDb = createTestDb(trackedDbs) + let calls = 0 + return Effect.gen(function* () { + yield* Effect.promise(() => + insertWorkspace(testDb, { + id: "sw_lc5xx", + orgId: "org_lc", + teamId: "T-LC", + teamName: "LC", + botToken: "xoxb-lc-token", + apiKey: "maple_ak_lc", + }), + ) + const slack = yield* SlackIntegrationService + // A 1000-channel page can time out server-side; Slack answers with an + // HTML error page, not JSON. That must not surface as the misleading + // "returned a non-JSON response". + const error = yield* runInterleaved(Effect.flip(slack.listChannels(asOrgId("org_lc")))) + assert.strictEqual(error._tag, "@maple/http/errors/IntegrationsUpstreamError") + assert.include(error.message, "HTTP 503") + assert.notInclude(error.message, "non-JSON") + // One attempt plus SLACK_RATE_LIMIT_RETRIES. + assert.strictEqual(calls, 4) + }).pipe( + Effect.provide( + withFetch( + testDb, + slackApiFetch(CONVERSATIONS_URL, () => { + calls++ + return new Response("upstream timeout", { + status: 503, + headers: { "content-type": "text/html" }, + }) + }), + ), + ), + ) + }) + + it.effect("listChannels recovers when a transient 5xx clears on retry", () => { + const testDb = createTestDb(trackedDbs) + let calls = 0 + return Effect.gen(function* () { + yield* Effect.promise(() => + insertWorkspace(testDb, { + id: "sw_lc5xxok", + orgId: "org_lc", + teamId: "T-LC", + teamName: "LC", + botToken: "xoxb-lc-token", + apiKey: "maple_ak_lc", + }), + ) + const slack = yield* SlackIntegrationService + const channels = yield* runInterleaved(slack.listChannels(asOrgId("org_lc"))) + assert.deepStrictEqual( + channels.map((c) => c.id), + ["C1"], + ) + assert.strictEqual(calls, 2) + }).pipe( + Effect.provide( + withFetch( + testDb, + slackApiFetch(CONVERSATIONS_URL, (_url, call) => { + calls++ + return call === 0 + ? new Response("", { status: 500 }) + : jsonResponse({ ok: true, channels: [{ id: "C1", name: "one" }] }) + }), + ), + ), + ) + }) + it.effect("listChannels maps Slack ok:false to an upstream error", () => { const testDb = createTestDb(trackedDbs) return Effect.gen(function* () { diff --git a/apps/api/src/services/SlackIntegrationService.ts b/apps/api/src/services/SlackIntegrationService.ts index 4579d95cc..a86b3c79a 100644 --- a/apps/api/src/services/SlackIntegrationService.ts +++ b/apps/api/src/services/SlackIntegrationService.ts @@ -53,11 +53,37 @@ const SLACK_CHANNELS_PER_PAGE = 1000 */ const SLACK_MAX_CHANNEL_PAGES = 20 -/** Attempts per page when Slack answers 429; each waits out `Retry-After`. */ +/** Attempts per page when Slack answers 429 or 5xx; each waits before retrying. */ const SLACK_RATE_LIMIT_RETRIES = 3 -/** Upper bound on an honoured `Retry-After`, so one page can't hang the request. */ -const SLACK_MAX_RETRY_AFTER_MS = 30_000 +/** + * Absolute ceiling on a single honoured `Retry-After`. + * + * This bounds ONE sleep, not the request — {@link SLACK_CHANNEL_WALK_BUDGET_MS} + * is what keeps the overall walk bounded. `conversations.list` is Tier 2 with a + * 60-second window, so Slack routinely answers `Retry-After: 60`; clamping that + * to anything shorter just guarantees the retry lands inside the same window and + * re-429s, burning an attempt for nothing. + */ +const SLACK_MAX_RETRY_AFTER_MS = 60_000 + +/** Backoff before retrying a 5xx (Slack sends no `Retry-After` on those). */ +const SLACK_SERVER_ERROR_BACKOFF_MS = 1_000 + +/** + * Wall-clock budget for the entire channel walk — every page, every retry, every + * backoff sleep. + * + * The per-sleep and per-page caps do not compose into anything usable on their + * own: {@link SLACK_MAX_CHANNEL_PAGES} pages × ({@link SLACK_RATE_LIMIT_RETRIES} + * + 1) attempts × a 60s `Retry-After` is over an hour of sleeping. Cloudflare's + * edge gives up on the response at ~100s, so past that point the Worker is + * burning CPU-seconds on a request nobody is listening to. 45s leaves generous + * headroom under that cutoff. Exhausting the budget is not an error: we return + * the channels collected so far (a partial picker beats a failed one) and log + * the same truncation warning as the page cap. + */ +const SLACK_CHANNEL_WALK_BUDGET_MS = 45_000 /** Why a workspace binding was revoked without going through `uninstall`. */ export type SlackRevocationReason = "app_uninstalled" | "tokens_revoked" | "reconciliation" @@ -168,9 +194,16 @@ const DEAD_TOKEN_AUTH_TEST_ERRORS: ReadonlySet = new Set([ /** * Slack's 429 `Retry-After` is whole seconds. Missing/garbage header → 1s, and - * anything longer than {@link SLACK_MAX_RETRY_AFTER_MS} is clamped so a single - * page can't hold the HTTP request open indefinitely. + * anything longer than {@link SLACK_MAX_RETRY_AFTER_MS} is clamped so one absurd + * header can't eat the whole {@link SLACK_CHANNEL_WALK_BUDGET_MS}. + */ +/** + * Statuses worth another attempt: the rate limit, and Slack's own 5xx (a + * 1000-channel page is big enough to occasionally time out server-side, which + * is exactly the risk {@link SLACK_CHANNELS_PER_PAGE} accepts). */ +const isRetryableSlackStatus = (status: number): boolean => status === 429 || status >= 500 + const retryAfterMs = (header: string | undefined): number => { const seconds = Number.parseInt(header ?? "", 10) if (!Number.isFinite(seconds) || seconds <= 0) return 1_000 @@ -845,9 +878,19 @@ const make: Effect.Effect< ), ) let response = yield* request - for (let attempt = 0; response.status === 429 && attempt < SLACK_RATE_LIMIT_RETRIES; attempt++) { - const waitMs = retryAfterMs(response.headers["retry-after"]) - yield* Effect.logWarning("Slack conversations.list rate limited — backing off", { + for ( + let attempt = 0; + isRetryableSlackStatus(response.status) && attempt < SLACK_RATE_LIMIT_RETRIES; + attempt++ + ) { + // 429 carries `Retry-After`; a 5xx doesn't, so back off linearly instead. + // Both are bounded by the caller's overall budget, not by this loop. + const waitMs = + response.status === 429 + ? retryAfterMs(response.headers["retry-after"]) + : SLACK_SERVER_ERROR_BACKOFF_MS * (attempt + 1) + yield* Effect.logWarning("Slack conversations.list failed — backing off", { + status: response.status, attempt: attempt + 1, waitMs, }) @@ -862,6 +905,18 @@ const make: Effect.Effect< }), ) } + // Anything else non-2xx (a Slack 5xx that outlived the retries, a proxy's + // HTML error page, an auth redirect) has no JSON body worth parsing — + // without this it fell through to `response.json` and surfaced as the + // thoroughly unhelpful "returned a non-JSON response". + if (response.status < 200 || response.status >= 300) { + return yield* Effect.fail( + new IntegrationsUpstreamError({ + message: `Slack conversations.list failed with HTTP ${response.status}`, + status: response.status, + }), + ) + } const json = yield* response.json.pipe( Effect.mapError( (error) => @@ -904,22 +959,33 @@ const make: Effect.Effect< /** * Cursor-driven page walk that runs to cursor exhaustion, with * {@link SLACK_MAX_CHANNEL_PAGES} as a runaway guard rather than an intended - * limit. Truncation is logged: a workspace that trips the cap gets a silently + * limit and {@link SLACK_CHANNEL_WALK_BUDGET_MS} as a hard wall-clock stop. + * Truncation is logged: a workspace that trips either bound gets a silently * short channel list, which is exactly the failure mode we can't see from the * UI. + * + * `collected` deliberately lives outside the timed effect — when the budget + * interrupts the walk mid-page we still return every page that completed. */ const collectChannelPages = Effect.fnUntraced(function* (botToken: string) { const collected: Array = [] - let cursor = Option.none() - for (let page = 0; page < SLACK_MAX_CHANNEL_PAGES; page++) { - const { channels, next } = yield* fetchChannelPage(botToken, cursor) - collected.push(...channels) - if (Option.isNone(next)) return collected - cursor = next - } - yield* Effect.logWarning("Slack channel list truncated at the page cap", { + const walk = Effect.gen(function* () { + let cursor = Option.none() + for (let page = 0; page < SLACK_MAX_CHANNEL_PAGES; page++) { + const { channels, next } = yield* fetchChannelPage(botToken, cursor) + collected.push(...channels) + if (Option.isNone(next)) return true + cursor = next + } + return false + }) + const outcome = yield* Effect.timeoutOption(walk, SLACK_CHANNEL_WALK_BUDGET_MS) + if (Option.isSome(outcome) && outcome.value) return collected + yield* Effect.logWarning("Slack channel list truncated", { + reason: Option.isNone(outcome) ? "time-budget" : "page-cap", pages: SLACK_MAX_CHANNEL_PAGES, perPage: SLACK_CHANNELS_PER_PAGE, + budgetMs: SLACK_CHANNEL_WALK_BUDGET_MS, channels: collected.length, }) return collected diff --git a/apps/web/src/components/alerts/destination-dialog.tsx b/apps/web/src/components/alerts/destination-dialog.tsx index 5ac4d6258..6237e118c 100644 --- a/apps/web/src/components/alerts/destination-dialog.tsx +++ b/apps/web/src/components/alerts/destination-dialog.tsx @@ -17,7 +17,12 @@ import { HazelIcon, LoaderIcon, } from "@/components/icons" -import { CHANNEL_RESULT_LIMIT, rankChannels } from "@/components/alerts/slack-channel-search" +import { + CHANNEL_RESULT_LIMIT, + channelLabel, + channelPickerView, + resolveSearchQuery, +} from "@/components/alerts/slack-channel-search" import { MapleApiAtomClient } from "@/lib/services/common/atom-client" import { MapleApiV2AtomClient } from "@/lib/services/common/v2-atom-client" import { v2ErrorInfo } from "@/lib/error-messages" @@ -623,13 +628,17 @@ function SlackBotFields({ // popup's empty state and keyboard highlighting agree with what's rendered. const [channelQuery, setChannelQuery] = useState("") // Picking a channel makes Base UI write its label into the input; that is a - // selection, not a search, and must not narrow the list to one row. - const selectedLabel = channels.find((c) => c.id === form.slackChannelId)?.name - const searchQuery = - selectedLabel !== undefined && channelQuery.replace(/^#/, "") === selectedLabel ? "" : channelQuery - const visibleChannels = useMemo(() => rankChannels(channels, searchQuery), [channels, searchQuery]) + // selection, not a search, and must not narrow the list to one row. Compare + // against the same label function the Combobox displays — a private channel's + // label carries a " (private)" suffix that a bare-name compare misses, which + // left `searchQuery` holding "#alerts (private)" and emptied the picker. + const selectedChannel = channels.find((c) => c.id === form.slackChannelId) + const searchQuery = resolveSearchQuery(channelQuery, selectedChannel) + const { visible: visibleChannels, truncated } = useMemo( + () => channelPickerView(channels, searchQuery, form.slackChannelId || null), + [channels, searchQuery, form.slackChannelId], + ) const visibleChannelIds = useMemo(() => visibleChannels.map((c) => c.id), [visibleChannels]) - const truncated = visibleChannels.length < channels.length // `GET /v2/integrations/slack/channels` is admin-gated (`requireAdmin`), so a // regular member gets a 403 that no amount of retrying will clear. The v2 @@ -741,10 +750,9 @@ function SlackBotFields({ // Unknown id (stale/stored channel not in the fetched list): prefer the // stored name, but never render an empty label — show the raw id. if (!channel) return form.slackChannelName ? `#${form.slackChannelName}` : id - return channel.is_private ? `#${channel.name} (private)` : `#${channel.name}` + return channelLabel(channel) } - const selectedChannel = channels.find((c) => c.id === form.slackChannelId) // Editing keeps the stored channel until a new one is picked — its id isn't // returned, so the form field is empty by design. Render the stored value as a // value (not as placeholder gray, which reads as "nothing configured"). @@ -809,6 +817,10 @@ function SlackBotFields({ // The stored channel is shown as a value above; the placeholder stays // the actual prompt so the field never looks pre-filled. placeholder={channelsLoading ? "Loading channels…" : "Search channels…"} + // Single-select never clears the input on close, so a query that + // matched nothing would survive an Escape and reopen as an empty + // popup with no way out. The clear button is the way out. + showClear className="w-full" /> @@ -842,11 +854,16 @@ function SlackBotFields({ {/* Big workspaces run to thousands of channels; rendering them all is both slow and useless. Say that the list is a top-N so an absent - channel reads as "keep typing", not "we don't have it". */} + channel reads as "keep typing", not "we don't have it". + `truncated` means matches were actually dropped, so this can never + render over an empty list — and while a query is active we don't + know the true match count, only that it exceeded the cap, so don't + print the workspace total as if it were one. */} {truncated ? (

- Showing the closest {CHANNEL_RESULT_LIMIT} of {channels.length} channels — keep - typing to narrow. + {searchQuery.trim().length > 0 + ? `Showing the closest ${CHANNEL_RESULT_LIMIT} matches — keep typing to narrow.` + : `Showing ${CHANNEL_RESULT_LIMIT} of ${channels.length} channels — type to narrow.`}

) : null} diff --git a/apps/web/src/components/alerts/slack-channel-search.test.ts b/apps/web/src/components/alerts/slack-channel-search.test.ts index 4fa11120e..21db27e10 100644 --- a/apps/web/src/components/alerts/slack-channel-search.test.ts +++ b/apps/web/src/components/alerts/slack-channel-search.test.ts @@ -1,5 +1,12 @@ import { describe, expect, it } from "vitest" -import { CHANNEL_RESULT_LIMIT, rankChannels, scoreChannelName } from "./slack-channel-search" +import { + CHANNEL_RESULT_LIMIT, + channelLabel, + channelPickerView, + rankChannels, + resolveSearchQuery, + scoreChannelName, +} from "./slack-channel-search" const channel = (name: string, is_member = true) => ({ name, is_member }) const names = (list: ReadonlyArray<{ name: string }>) => list.map((c) => c.name) @@ -34,7 +41,11 @@ describe("scoreChannelName", () => { }) it("prefers the shorter name among prefix matches", () => { - expect(scoreChannelName("dep", "dep")).toBeGreaterThan(scoreChannelName("dep-a-very-long", "dep")!) + // Both are prefix matches and neither is exact — otherwise this would be + // comparing tiers (exact > prefix) and prove nothing about length. + expect(scoreChannelName("dep-a", "dep")).toBeGreaterThan( + scoreChannelName("dep-a-very-long", "dep")!, + ) }) }) @@ -73,3 +84,88 @@ describe("rankChannels", () => { expect(names(rankChannels(channels, "alerts"))[0]).toBe("alerts") }) }) + +const pickable = (id: string, name: string, is_private = false, is_member = true) => ({ + id, + name, + is_private, + is_member, +}) + +describe("resolveSearchQuery", () => { + const publicChannel = pickable("C1", "alerts") + const privateChannel = pickable("C2", "alerts", true) + + it("treats the rendered label of the selected channel as 'not a search'", () => { + expect(resolveSearchQuery(channelLabel(publicChannel), publicChannel)).toBe("") + // The regression: a private channel's label carries " (private)", so a + // bare-name compare left the query set and emptied the picker. + expect(channelLabel(privateChannel)).toBe("#alerts (private)") + expect(resolveSearchQuery(channelLabel(privateChannel), privateChannel)).toBe("") + }) + + it("also accepts the plain #name / name forms of the selection", () => { + expect(resolveSearchQuery("#alerts", privateChannel)).toBe("") + expect(resolveSearchQuery("alerts", privateChannel)).toBe("") + expect(resolveSearchQuery(" #Alerts (PRIVATE) ", privateChannel)).toBe("") + }) + + it("leaves real typing alone", () => { + expect(resolveSearchQuery("dep", publicChannel)).toBe("dep") + expect(resolveSearchQuery("alerts-2", publicChannel)).toBe("alerts-2") + expect(resolveSearchQuery("alerts", undefined)).toBe("alerts") + }) +}) + +describe("channelPickerView", () => { + const many = (count: number) => Array.from({ length: count }, (_, i) => pickable(`C${i}`, `chan-${i}`)) + + it("does not claim truncation when a query merely narrows the list", () => { + const channels = [...many(60)] + // 60 channels, 3 matches — the old `visible.length < channels.length` check + // reported "showing the closest 50 of 60". + const view = channelPickerView(channels, "chan-1", null, 50) + expect(view.truncated).toBe(false) + expect(view.visible.length).toBeLessThan(50) + }) + + it("does not claim truncation when the match count exactly equals the cap", () => { + const view = channelPickerView(many(50), "", null, 50) + expect(view.visible.length).toBe(50) + expect(view.truncated).toBe(false) + }) + + it("claims truncation only once matches were actually dropped", () => { + const view = channelPickerView(many(51), "", null, 50) + expect(view.visible.length).toBe(50) + expect(view.truncated).toBe(true) + }) + + it("never reports truncation with nothing to show", () => { + const view = channelPickerView(many(500), "zzqqzz", null, CHANNEL_RESULT_LIMIT) + expect(view.visible).toEqual([]) + expect(view.truncated).toBe(false) + }) + + it("pulls the selected channel into the unfiltered view even when it's past the cap", () => { + const channels = many(500) + const selected = channels.at(-1)! + const view = channelPickerView(channels, "", selected.id, 50) + expect(view.visible.length).toBe(50) + expect(view.visible[0]).toBe(selected) + expect(view.truncated).toBe(true) + }) + + it("leaves a filtered view honest — the selection is not forced into search results", () => { + const channels = [...many(60), pickable("C-sel", "totally-unrelated")] + const view = channelPickerView(channels, "chan-1", "C-sel", 50) + expect(view.visible.some((c) => c.id === "C-sel")).toBe(false) + }) + + it("doesn't duplicate a selection that already ranks", () => { + const channels = many(10) + const view = channelPickerView(channels, "", "C3", 50) + expect(view.visible.filter((c) => c.id === "C3").length).toBe(1) + expect(view.visible.length).toBe(10) + }) +}) diff --git a/apps/web/src/components/alerts/slack-channel-search.ts b/apps/web/src/components/alerts/slack-channel-search.ts index 33944215b..ea4fcc52e 100644 --- a/apps/web/src/components/alerts/slack-channel-search.ts +++ b/apps/web/src/components/alerts/slack-channel-search.ts @@ -14,6 +14,12 @@ export interface RankableChannel { readonly is_member: boolean } +/** The extra fields the picker-state helpers need on top of {@link RankableChannel}. */ +export interface PickableChannel extends RankableChannel { + readonly id: string + readonly is_private: boolean +} + /** How many rows the picker renders. Beyond this the list is noise, not choice. */ export const CHANNEL_RESULT_LIMIT = 50 @@ -99,3 +105,76 @@ export const rankChannels = ( }) return scored.slice(0, Math.max(limit, 0)).map((entry) => entry.channel) } + +/** + * The exact string the Combobox shows for a channel — in the popup row, and, + * once a channel is picked, written by Base UI into the input itself. + * + * Selection detection ({@link resolveSearchQuery}) *must* compare against this + * and not against the bare name: a private channel's label carries a + * ` (private)` suffix, and comparing against `name` alone made every private + * pick look like a search for a channel that doesn't exist. + */ +export const channelLabel = (channel: Pick): string => + channel.is_private ? `#${channel.name} (private)` : `#${channel.name}` + +/** + * Base UI writes the selected item's label into the input, which fires + * `onInputValueChange` — indistinguishable from typing unless we recognise the + * label. Anything the input could plausibly be holding *because of* a selection + * counts: the rendered label, and the plain `#name` / `name` forms so a stored + * value or a manual retype of the name doesn't wipe the list either. + */ +const selectionEchoes = (channel: Pick): ReadonlyArray => [ + channelLabel(channel), + `#${channel.name}`, + channel.name, +] + +/** + * The query to actually search on: empty when the input merely echoes the + * current selection, otherwise what the user typed. + */ +export const resolveSearchQuery = ( + input: string, + selected: Pick | undefined, +): string => { + if (selected === undefined) return input + const typed = input.trim().toLowerCase() + return selectionEchoes(selected).some((echo) => echo.toLowerCase() === typed) ? "" : input +} + +export interface ChannelPickerView { + /** The rows to render, and the `items` handed to Base UI. */ + readonly visible: ReadonlyArray + /** True only when matches were actually dropped by the cap. */ + readonly truncated: boolean +} + +/** + * What the popup should show for a query. + * + * Two things the naive `visible.length < channels.length` version got wrong: + * ranking one row past the cap is the only way to tell "we dropped matches" + * apart from "exactly {@link CHANNEL_RESULT_LIMIT} channels matched" (any + * narrowing query made the old check true, so filtering 60 channels to 3 + * claimed 50 were shown); and with the cap in play the selected channel is + * often outside the top N, which leaves the Combobox unable to render its own + * checkmark. On the unfiltered view we pull the selection in explicitly — + * never on a filtered one, where showing a non-matching row would be a lie. + */ +export const channelPickerView = ( + channels: ReadonlyArray, + searchQuery: string, + selectedId: string | null, + limit: number = CHANNEL_RESULT_LIMIT, +): ChannelPickerView => { + const ranked = rankChannels(channels, searchQuery, limit + 1) + const truncated = ranked.length > limit + const visible = truncated ? ranked.slice(0, limit) : ranked + if (searchQuery.trim().length > 0 || selectedId === null || limit < 1) return { visible, truncated } + if (visible.some((channel) => channel.id === selectedId)) return { visible, truncated } + const selected = channels.find((channel) => channel.id === selectedId) + if (selected === undefined) return { visible, truncated } + return { visible: [selected, ...visible.slice(0, limit - 1)], truncated } +}