fix(connectors): make selector dropdowns resolve options the drain has not reached - #6027
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
PR SummaryMedium Risk Overview Connector selector UI now uses drain state ( Selector hooks: Confluence spaces API accepts optional Reviewed by Cursor Bugbot for commit 10bb508. Configure here. |
Greptile SummaryFixes connector selector dropdowns so options still loading via background page drain can be resolved and empty states no longer claim a miss while the list is filling.
Confidence Score: 5/5This follow-up pass found no remaining blocking failures tied to prior Greptile threads, so the PR appears safe to merge from that scope. No blocking failure remains from prior Greptile findings or eligible follow-up issues on the current head.
|
| Filename | Overview |
|---|---|
| apps/sim/app/api/tools/confluence/selector-spaces/route.ts | Adds exact-key space lookup with concurrent current/archived queries and shared response mapping. |
| apps/sim/app/workspace/[workspaceId]/knowledge/[id]/components/connector-selector-field/connector-selector-field.tsx | Wires detail resolution and drain-aware empty messages into the connector selector UI. |
| apps/sim/hooks/selectors/use-selector-query.ts | Hardens detail-query enablement and adds multi-id detail resolution sharing the detail cache. |
| packages/emcn/src/components/combobox/combobox.tsx | Adds ref-backed, deduped onSearchChange for searchable combobox consumers. |
Reviews (8): Last reviewed commit: "refactor(connectors): resolve selected o..." | Re-trigger Greptile
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
ac07b0c to
030df2b
Compare
waleedlatif1
changed the title
|
@cursor review |
030df2b to
3f145fc
Compare
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 3f145fc. Configure here.
…not exist The connector selector field ignored the drain state the shared selector hook already exposes. Paginated selectors fill in the background and the combobox filters client-side, so a not-yet-drained option is genuinely absent from the list — and the dropdown said "No spaces found", which reads as "this space does not exist" and sends users off to enter the value by hand. That is what happened with a Confluence space on a site with thousands of personal spaces. It now reports that the list is still filling, surfaces a failed drain instead of claiming to load forever, and is honest when the drain stops at its page cap.
…ng on the drain The connector selector fills by draining pages in the background and filters client-side, so an option is only findable once its page has arrived. On a large Confluence site that is ~38 seconds, during which searching for a real space returned nothing. Resolves the typed value and the selected value directly through the selector's `fetchById`, merging both into the option list, so an exact key is selectable immediately regardless of drain progress. `confluence.spaces.fetchById` now uses the documented v2 `keys` filter instead of scanning only the first page — which never resolved a space sorting beyond it. Adds `onSearchChange` to the shared Combobox so a consumer can observe the search box, held in a ref so its identity stays stable for the handlers that capture it without declaring it. Also fixes a pre-existing hazard in useSelectorOptionDetail: a caller-supplied `enabled` replaced the guard that checks a definition declares `fetchById`, so `queryFn`'s non-null assertion would throw for the ~12 selectors without one.
3f145fc to
3d779e9
Compare
…turning one page `/api/tools/confluence/pages` issued a single request and discarded the `_links.next` cursor Confluence returns, so the picker showed only the first `limit` pages of however many exist — a search for a real page found nothing, with no error and no truncation signal. Threads the documented opaque cursor and moves the selector to `fetchPage`, so the option list drains like `confluence.spaces` and reports real `hasMore` / `truncated` state. The `title` server-side filter is unchanged and now paginates too. `limit` is deliberately left at its existing default: the endpoint's maximum is not confirmable from Atlassian's published reference, and raising it is not needed for correctness. Also guards `data.results`, which threw when the response omitted it.
waleedlatif1
changed the title
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 58fb25a. Configure here.
…okup The exact-key lookup omitted `status`, assuming that matched a space whether it was current or archived. Atlassian documents a `current,archived` default for `/pages` but documents no default for `/spaces`, where `status` takes a single value rather than an array — so the assumption was unverified, and resolving only current spaces would silently miss archived ones. Archived spaces are reachable through the paged path and sync works against them. Queries `current` first and falls back to `archived` only when it finds nothing, so the common case stays one request and the behaviour no longer depends on an undocumented default.
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 63603b5. Configure here.
…en key lookup Six adversarial verification passes against the provider spec and the surrounding plumbing found three defects in the prior commits. Reverts the Confluence pages selector to a single request. Draining it was not strictly better: with no search term `title` is unset, so opening the dropdown walked the entire site — up to 50 sequential requests and 2,500 options where there had been one request and 50 — and the route forwards no abort signal upstream, so superseded drains still bill the tenant's rate limit. The same fan-out reached `loadAllSelectorOptions`, and the justification rested on `title` semantics Atlassian does not publish. The list cap is a real gap, but it needs confirmed server-side search, not brute force. Keeps the `results` guard. Restores selector display names for CloudWatch blocks. Narrowing a caller's `enabled` against `definition.enabled` disabled a detail query those selectors satisfy without AWS context, so collapsed blocks rendered "-" instead of the log group name. A caller that opts in is now narrowed only by the hard precondition that `fetchById` exists, which is what `queryFn` actually asserts. Queries both space statuses concurrently rather than sequentially: the key is user-typed text, so a miss dominates while typing and paid two round-trips. Each row now falls back to the status its own call requested. Also drops the "enter the value directly" empty states — the combobox is not editable, so there is no such affordance — and dedupes `onSearchChange`, which several reset paths fire redundantly.
waleedlatif1
changed the title
|
@cursor review |
…d key lookup Addresses three review findings. A key lookup failed entirely when either status leg errored, discarding a match the other leg had found. Only a total failure is fatal now. Resolved option labels are remembered for the lifetime of the field. Both lookups key on values that change — the search box clears on select and close, and a multi-select field resolves no id at all — so the label for a just-picked option vanished a debounce later and the trigger fell back to a raw id. This covers the multi-select case, which is what the Confluence space field uses. A failed exact-value lookup now reads differently from a failed list load, rather than being reported as "still loading" or "none found".
|
@cursor review |
…ext changes Resolved ids are only meaningful within one selector context, so switching credential, domain, or a dependency has to discard what was remembered under the old one — the queries re-key, but the remembered labels would linger and mislabel until the field remounted. Keyed on the serialized context rather than its identity: the context memo also depends on `sourceConfig`, so its identity changes on unrelated field edits and would clear the cache far more often than intended.
Replaces the remembered-label map with per-id queries over the selected values, following the pattern the knowledge-base selector already uses. The map only ever held ids searched for in the current session, so a multi-select field restored from saved config still rendered raw ids — the case that actually matters. It also needed two effects and a serialized-context key to avoid leaking labels across a context change, all of which disappear: queries key on the context, so a label cannot outlive it. Gates the speculative lookup of typed text on a new `resolvesUnknownIds` flag, set only where `fetchById` returns null for an id that does not exist. Most implementations resolve a record by id, so every partial keystroke was a failed upstream request, retried once, and its error made "could not check that exact value" the normal empty state on those selectors. Also: pass handlers straight to the combobox rather than through identity wrappers that defeated its memo, move the latest-callback ref write into an effect, rename the raw search setter so the deduping write path cannot be bypassed, and correct the prop and staleTime docs.
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 10bb508. Configure here.
1 participant
Summary
Connector selector option lists fill by draining pages in the background and are filtered client-side, so an option that has not arrived yet is indistinguishable from one that does not exist. Production logs for the reporting workspace show a 24-page, 38-second drain at the exact minute of the report.
1. The empty state lied. The field ignored the drain state the shared hook already exposes (
hasMore/isFetchingMore/truncated/error), so a still-filling list rendered "No spaces found" — which reads as this space does not exist and sends users off to enter the value by hand. It now distinguishes still-loading, failed-to-load, and too-many-to-list.erroris checked first: a failed page halts the drain but leaveshasMoreset, which would otherwise claim to be loading forever.2. An exact key could not be resolved without waiting for the drain.
confluence.spaces.fetchByIdfetched the first page and scanned it, so a space sorting beyond page 1 never resolved — on a ~6,000-space site, nearly all of them. It now uses the documented v2keysfilter. Bothcurrentandarchivedare queried explicitly and concurrently:statustakes a single value on/spacesand Atlassian documents no default there (unlike/pages, where it is an array defaulting tocurrent,archived), and archived spaces are selectable and sync against.The typed value and the selected value are resolved through
fetchByIdand merged into the option list, so an exact key is selectable immediately and a just-picked option does not vanish when the search box resets.3.
useSelectorOptionDetailcould throw. A caller-suppliedenabledreplaced the guard checking that a definition declaresfetchById, soqueryFn's non-null assertion threw for the ~12 selectors without one. A caller'senabledis now narrowed by that hard precondition — but deliberately not bydefinition.enabled, which describes when a list can be fetched and gates on context (credential, domain, region) that resolving one known id may not need.cloudwatch.*echoes an id back without calling AWS, so gating on the list predicate would blank the log-group name on collapsed canvas blocks.Adds
onSearchChangeto the sharedCombobox, read through a ref so its identity stays stable for the handlers that capture it without declaring it, and deduped because several reset paths fire redundantly.Deliberately not done
confluence.pages. An earlier revision did this and it was reverted: with no search termtitleis unset, so opening the dropdown walked the entire site — up to 50 sequential requests and 2,500 options where there had been one request and 50 — the route forwards no abort signal upstream so superseded drains still bill the tenant's rate limit, and the same fan-out reachedloadAllSelectorOptions. Its justification also rested ontitlematch semantics Atlassian does not publish. The 50-item cap is a real gap that needs confirmed server-side search, not brute force.sort=nameon/spaces. Atlassian documentssort's allowed values only on the different/data-policies/spacesendpoint. A rejected value 400s, our route maps that to 502, and the dropdown breaks entirely.space.title~can false-match on the creator's username.Type of Change
Testing
Typecheck,
lint:check(19/19),check:api-validation, and 497 tests pass. Root cause established from CloudWatch (/ecs/sim-production/us-east-1/app), not inferred.Reviewed by the eight
/cleanuppasses and then by six adversarial verification passes against the Atlassian OpenAPI spec and the surrounding plumbing. Between them they found five real defects in this diff — theenabledguard, a vanishing selected option, a combobox stale closure, an unverifiedstatusdefault, and the pages-drain regression — all fixed or reverted here.Not runtime-verified. Nothing here has executed against a live Confluence instance, and there is no test file for these routes. Remaining unknowns, all needing a live call:
titlematch semantics, whether/spacessilently clampslimit=250, andkeyscase-sensitivity.Checklist