diff --git a/apps/api/src/services/SlackIntegrationService.test.ts b/apps/api/src/services/SlackIntegrationService.test.ts index 9adaa258..7fa06a9f 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,48 @@ 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 + +/** 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 @@ -1048,6 +1090,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 +1106,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 +1132,173 @@ 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 channels = yield* runInterleaved(slack.listChannels(asOrgId("org_lc"))) + 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 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 e1d66003..a86b3c79 100644 --- a/apps/api/src/services/SlackIntegrationService.ts +++ b/apps/api/src/services/SlackIntegrationService.ts @@ -33,7 +33,57 @@ 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 or 5xx; each waits before retrying. */ +const SLACK_RATE_LIMIT_RETRIES = 3 + +/** + * 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" @@ -142,6 +192,24 @@ 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 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 + 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 +850,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 +877,46 @@ const make: Effect.Effect< }), ), ) + let response = yield* request + 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, + }) + 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, + }), + ) + } + // 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) => @@ -843,17 +957,37 @@ 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 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)) break - cursor = next - } + 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 4ec7e874..6237e118 100644 --- a/apps/web/src/components/alerts/destination-dialog.tsx +++ b/apps/web/src/components/alerts/destination-dialog.tsx @@ -10,7 +10,19 @@ 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, + 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" @@ -20,7 +32,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 +609,37 @@ 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. 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]) + // `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 @@ -712,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"). @@ -726,18 +763,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 @@ -754,12 +817,24 @@ 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" /> - 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 +852,20 @@ 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` 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 ? ( +

+ {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} {channelsFailed ? ( @@ -788,7 +877,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 00000000..21db27e1 --- /dev/null +++ b/apps/web/src/components/alerts/slack-channel-search.test.ts @@ -0,0 +1,171 @@ +import { describe, expect, it } from "vitest" +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) + +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", () => { + // 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")!, + ) + }) +}) + +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") + }) +}) + +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 new file mode 100644 index 00000000..ea4fcc52 --- /dev/null +++ b/apps/web/src/components/alerts/slack-channel-search.ts @@ -0,0 +1,180 @@ +/** + * 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 +} + +/** 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 + +/** + * 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) +} + +/** + * 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 } +}