fix(alerts): make the Slack channel picker usable at scale - #279
Open
JeremyFunk wants to merge 2 commits into
Open
fix(alerts): make the Slack channel picker usable at scale#279JeremyFunk wants to merge 2 commits into
JeremyFunk wants to merge 2 commits into
Conversation
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 <noreply@anthropic.com>
Contributor
|
Your Pullfrog Router balance is exhausted. You have a payment method on file but auto-reload is disabled, so runs paused once your balance went past the overdraft buffer. Top up balance → · Enable auto-reload →
|
🍁 Maple PR previewThe preview is ready:
Deployed from |
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 <noreply@anthropic.com>
Contributor
|
Your Pullfrog Router balance is exhausted. You have a payment method on file but auto-reload is disabled, so runs paused once your balance went past the overdraft buffer. Top up balance → · Enable auto-reload →
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes three reported problems with the channel picker on the slack-bot alert destination.
1. Search was broken, now fuzzy + ranked
The Combobox root was never given the
itemsprop, so Base UI did no filtering at all — typing narrowed nothing, andComboboxEmpty(which keys offfilteredItems) rendered its "No matching channels." line unconditionally. That's the "practically unusable" part.New
apps/web/src/components/alerts/slack-channel-search.tsranks client-side:depalerfindsdeploy-alerts)#stripped on both sides, case-insensitive; within a tier, earlier index and shorter name win; ties break on bot membership then alphabeticallyThe ranked ids go back to the Combobox as
itemswithfilter={null}(no double filtering), so the empty state and keyboard highlighting match what's rendered. No new dependency; pure function, unit-tested (10 cases).2. Refresh button
A
Refreshbutton next to the Channel label callsuseAtomRefreshon the stable query atom and spins while in flight. The existingpreviousSuccessfallback keeps the list on screen, so refreshing never blanks a selection mid-pick. The "no channels returned" hint now points at it, and the popup's empty state distinguishes loading / nothing-loaded / nothing-matched.3. Channel cap raised ~600 → ~20k
limit=200×SLACK_MAX_CHANNEL_PAGES=3becomes 1000/page × 20 pages. Slack's docs recommend ≤200 per page (large pages can time out), but the binding constraint here is the rate limit, not page latency:conversations.listis Tier 2 (~20 req/min), so a 10k-channel workspace needs 50 requests at 200/page (guaranteed 429s, multi-minute wait) versus 10 at 1000/page. The page count is now a runaway guard, not an intended limit — the walk runs to cursor exhaustion and logs a warning if it ever trips the cap instead of silently truncating.429s are also handled: Slack answers them with an empty body (which previously decoded as an "unexpected payload" upstream error). We now honour
Retry-After(3 attempts, clamped to 30s) and retry the same page.Verification
bun typecheck— 18/18 greenapps/apifull Vitest suite — 104 files passed, including the updated page-cap test and a new TestClock-driven 429-retry testapps/web— 117/123 files pass; the 6 failures are pre-existing onmain(verified by re-running them stashed): the two known jsdom-localStorage suites plus 4 alerts/model suites failing on an unrelatedSchemaError: Missing key at ["environments"]Not verified
No browser check — dev servers were off limits for this run, so the Combobox
items+filter={null}wiring is verified by types and the pure-function tests only.🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Review fixes
Follow-up commit addressing confirmed review findings:
label into the input on selection; the selection check compared it against the
bare channel name, so a private channel's
(private)suffix made every picklook like a search for a non-existent channel. Selection detection now uses the
same
channelLabel()the popup renders.(20 pages x 4 attempts x a 60s
Retry-After) while Cloudflare's edge gave up at~100s. Added a 45s overall wall-clock budget; on exhaustion it returns the pages
collected so far and logs the truncation warning.
SLACK_MAX_RETRY_AFTER_MS'sdoc comment now says what it actually bounds.
Retry-After: 60clamped to 30s —conversations.listis Tier 2 with a 60swindow, so the clamp guaranteed the retry re-429'd. Ceiling raised to 60s, now
safe under the overall budget.
visible.length < channels.lengthwas true whenever the query filtered, so 3 matches out of 60rendered "Showing the closest 50 of 60", including under "No matching channels."
Ranking now runs one row past the cap so hitting the cap is distinguishable from
"exactly 50 matched", and the copy no longer prints the workspace total as a
match count.
fell through to
response.jsonand surfaced as "returned a non-JSON response".Non-2xx now fails with the status; 5xx is retried within the budget.
items— past the 50-row cap theCombobox couldn't render its own checkmark. The selection is now pulled into the
unfiltered view (never into a filtered one, which would show a non-matching row).
(
showClear) so Escape-and-reopen isn't a dead end.turn count was coupled to PGlite's macrotask behaviour, so a low guess blocked
Fiber.jointo the vitest timeout. Replaced with a condition-drivenrunInterleavedhelper that fails loudly if the effect doesn't settle.Also fixed a
slack-channel-search.test.tscase that claimed to compare two prefixmatches but actually compared an exact match against a prefix. New coverage: the
time budget, the 5xx retry/report paths, selection-echo resolution, and the
truncation flag.