Architecture refactor scout: github-29726330621

[github-actions]

cc @algolia/frontend-experiences-web

Architecture Refactor Scout

Run: github-29726330621

Summary

I inspected the instantsearch.js core package — where every connector, the InstantSearch runtime (src/lib/InstantSearch.ts, routing, state mappings), the Chat/AI subsystem, and the insights/event machinery live — plus the cross-flavor UI seam in instantsearch-ui-components and its React/Vue consumers. The dominant friction is shallow, copy-pasted logic inside the connector layer: the facet connectors each re-implement the same UI-state pruning, facet-type-conflict validation, and show-more caching dance inline, so the same normalization and ordering knowledge is duplicated 4–6 times with no owning module and no single test surface. A second cluster of friction is callers knowing too much about ordering (InstantSearch must call refreshUiState() before reading UI state) and one over-wide connector (connectChat, 847 lines) that wires transport construction, request rewriting, and Agent Studio URL building inline rather than behind a module. The candidates below prefer high locality and small interface improvements; each maps to one reviewable PR.

Candidate Shortlist

candidate-1: Deepen index-UI-state pruning behind one module

• Recommendation strength: Strong
• Files:
  • packages/instantsearch.js/src/connectors/menu/connectMenu.ts:403-420
  • packages/instantsearch.js/src/connectors/numeric-menu/connectNumericMenu.ts:488-505
  • packages/instantsearch.js/src/connectors/refinement-list/connectRefinementList.ts (local removeEmptyRefinementsFromUiState)
  • packages/instantsearch.js/src/connectors/hierarchical-menu/connectHierarchicalMenu.ts
  • packages/instantsearch.js/src/connectors/rating-menu/connectRatingMenu.ts
  • packages/instantsearch.js/src/connectors/breadcrumb/connectBreadcrumb.ts
  • new: packages/instantsearch.js/src/lib/utils/ (owning module) + __tests__
• Problem: Six connectors each define a private removeEmptyRefinementsFromUiState(indexUiState, attribute) with the identical three-step shape — bail if the widget key is absent, delete the attribute when its value is "empty", delete the widget key when it becomes empty. The only thing that varies is the per-widget "empty" sentinel (menu uses === undefined, numericMenu uses === ':', others an empty array/string). This is a shallow function copied per connector: the normalization rule for "an index UI-state slice is empty and should be pruned" has no owning module, so a bug fix or edge case must be applied in six places and there is no single place to test the invariant.
• Proposed change: Introduce one small utility in src/lib/utils that owns the prune-empty-refinement invariant, parameterized by the widget UI-state key and a per-widget emptiness predicate (the only genuine variation). Each connector calls it instead of defining its own copy; delete the six local functions.
• Benefits: Locality — the pruning rule lives in one file with one focused test, instead of being smeared across six connectors. Leverage — callers learn one tiny interface instead of re-deriving the pattern. Testability — the emptiness/normalization edge cases (the sentinels that differ per widget) become the natural test surface of one module rather than incidental coverage inside six connector suites.
• Risks: Low. Pure refactor, no behavior change intended. Must preserve each connector's exact emptiness predicate (the sentinels genuinely differ) — the migration is easy to get subtly wrong by unifying predicates that should stay distinct. Covered by existing connector getWidgetUiState tests + common connector tests.
• Verification: yarn jest packages/instantsearch.js/src/connectors/{menu,numeric-menu,refinement-list,hierarchical-menu,rating-menu,breadcrumb}; yarn jest common-connectors; add a focused unit test for the new util covering each sentinel.

Before:

flowchart LR
  Menu[connectMenu] --> P1[removeEmpty copy]
  Numeric[connectNumericMenu] --> P2[removeEmpty copy]
  Refine[connectRefinementList] --> P3[removeEmpty copy]
  Hier[connectHierarchicalMenu] --> P4[removeEmpty copy]

Loading

After:

flowchart LR
  Menu[connectMenu] --> Deep[pruneIndexUiState]
  Numeric[connectNumericMenu] --> Deep
  Refine[connectRefinementList] --> Deep
  Hier[connectHierarchicalMenu] --> Deep
  Deep --> Rule[emptiness rule hidden]

Loading

candidate-2: Consolidate facet-type conflict validation

• Recommendation strength: Worth exploring
• Files:
  • packages/instantsearch.js/src/connectors/refinement-list/connectRefinementList.ts:496-517
  • packages/instantsearch.js/src/connectors/menu/connectMenu.ts:352-363
  • packages/instantsearch.js/src/connectors/hierarchical-menu/connectHierarchicalMenu.ts:441-464
  • packages/instantsearch.js/src/connectors/toggle-refinement/connectToggleRefinement.ts:444-455
  • new owning module under packages/instantsearch.js/src/lib/utils/
• Problem: Inside getWidgetSearchParameters, several facet connectors each hand-roll the same guard: check whether attribute is already registered as a hierarchical / conjunctive / disjunctive facet by a different widget, emit the same-shaped warning, and return searchParameters unchanged on conflict. Each connector therefore has to know which facet types conflict, the warning wording, and the "return state untouched on conflict" contract. This is orchestration/validation knowledge leaked identically into every faceting connector; there is no module that owns "is it safe for this widget to claim this attribute as facet type X?".
• Proposed change: Extract a single validation helper that, given the search parameters, an attribute, the facet type this widget wants, and the widget name, reports whether a conflict exists and produces the standard warning. Connectors call it and branch on the result; the warning text and conflict matrix live in one place.
• Benefits: Leverage — connectors express intent ("I want this attribute as a disjunctive facet") instead of re-deriving the conflict matrix. Locality — warning wording and the set of conflicting types change in one file. Testability — the conflict matrix becomes a directly testable unit rather than being probed through four connector suites.
• Risks: Moderate. Warning messages are semi-public (developers assert on console output in tests); wording and __DEV__ gating must be preserved byte-for-byte. Connectors differ slightly in what they log — the extraction must keep per-connector widget names and not over-unify.
• Verification: yarn jest packages/instantsearch.js/src/connectors/{refinement-list,menu,hierarchical-menu,toggle-refinement}; grep tests asserting the warning text still pass; add a unit test for the conflict matrix.

Before:

flowchart LR
  Refine[connectRefinementList] --> V1[conflict check copy]
  Menu[connectMenu] --> V2[conflict check copy]
  Toggle[connectToggleRefinement] --> V3[conflict check copy]

Loading

After:

flowchart LR
  Refine[connectRefinementList] --> Deep[assertFacetType]
  Menu[connectMenu] --> Deep
  Toggle[connectToggleRefinement] --> Deep
  Deep --> Matrix[conflict matrix + warning hidden]

Loading

candidate-3: Fold refreshUiState() into the UI-state read

• Recommendation strength: Worth exploring
• Files:
  • packages/instantsearch.js/src/lib/InstantSearch.ts:843-891 (setUiState, getUiState)
  • packages/instantsearch.js/src/widgets/index/index.ts:1070-1079 (refreshUiState, getWidgetUiState)
• Problem: Reading UI state correctly requires the caller to know an ordering invariant: InstantSearch.setUiState and getUiState must call this.mainIndex.refreshUiState() before this.mainIndex.getWidgetUiState({}), because getWidgetUiState reads a localUiState cache that refreshUiState() rebuilds from the child widgets. This is exactly "callers know too much about ordering": the index module's interface exposes a stale cache plus a manual refresh step, and every reader must remember to sequence them (and getUiState even guards the refresh behind this.started). The seam leaks a cache-invalidation detail that belongs inside the module.
• Proposed change: Make the index UI-state read responsible for its own freshness so callers get correct state without sequencing a refresh — e.g. have the read recompute (or lazily invalidate) localUiState internally, and remove the explicit refreshUiState() calls at the two InstantSearch call sites. Keep any refresh entry point only if a genuine external trigger still needs it.
• Benefits: Depth — a real ordering invariant moves from the caller into the module; the interface becomes "ask for UI state, get correct UI state." Locality — cache-freshness logic concentrates in index.ts. Testability — correctness no longer depends on callers replaying a two-step ritual, so tests target the read directly.
• Risks: Medium. refreshUiState() may be deliberately separated for performance (avoid recomputing on every read) or called from other paths; must confirm callers and the render loop don't rely on the current explicit-refresh timing. Behavior-sensitive around started/SSR — routing and SSR tests must stay green.
• Verification: yarn jest packages/instantsearch.js/src/lib/__tests__/routing; yarn jest packages/instantsearch.js/src/widgets/index; run the getUiState/setUiState and server-side tests; confirm no other refreshUiState() caller regresses.

Before:

flowchart LR
  IS[InstantSearch.getUiState] --> R[refreshUiState first]
  IS --> G[getWidgetUiState]
  G --> C[stale localUiState cache]

Loading

After:

flowchart LR
  IS[InstantSearch.getUiState] --> Deep[getWidgetUiState]
  Deep --> C[freshness handled inside]

Loading

candidate-4: Extract chat transport setup out of connectChat

• Recommendation strength: Worth exploring
• Files:
  • packages/instantsearch.js/src/connectors/chat/connectChat.ts (transport block ~445-557, agentId handling ~359, options types ~133-160)
  • packages/instantsearch.js/src/lib/ai-lite/transport.ts (DefaultChatTransport)
• Problem: connectChat (847 lines — the largest connector) inlines transport construction as one of several unrelated jobs. In the transport block it decides between a caller-supplied transport and a DefaultChatTransport, re-wraps the caller's prepareSendMessagesRequest to also strip data-* message parts (handling both sync and promise return paths), extracts credentials from agentId, and builds Agent Studio request URLs. The connector caller must understand that agentId and transport are mutually exclusive, that data-part filtering is layered after user preparation, and the sync/async branching of the wrapped preparer. This transport-wiring knowledge is a distinct concern with its own invariants that currently has no module.
• Proposed change: Move transport selection + prepareSendMessagesRequest wrapping + data-part filtering + Agent Studio URL/credential derivation into one module that takes the chat options and returns a ready transport. connectChat calls it once and stops knowing how a transport is assembled. Scope the PR to the transport seam only — leave streaming/tool orchestration alone.
• Benefits: Depth — a lot of setup behavior sits behind a small "build the transport for these options" interface. Locality — the agentId-vs-transport rules and data-part filtering live in one testable module instead of the connector body. It shrinks the most over-wide connector without touching its riskier streaming/tool paths.
• Risks: Medium. Chat is actively evolving (most recent commit touched it), so rebase risk is real; keep the PR narrow. Must preserve the exact sync/async handling of a caller-provided async prepareSendMessagesRequest and the mutual-exclusivity typing of agentId/transport.
• Verification: yarn jest common-widgets -t "Chat widget common tests"; yarn jest packages/instantsearch.js/src/connectors/chat; add unit tests for the transport module covering agentId path, custom-transport path, and data-part filtering with sync + async preparers.

Before:

flowchart LR
  Caller[connectChat] --> T1[choose transport]
  Caller --> T2[wrap prepareSendMessagesRequest]
  Caller --> T3[filter data parts]
  Caller --> T4[Agent Studio URL + creds]

Loading

After:

flowchart LR
  Caller[connectChat] --> Deep[createChatTransport options]
  Deep --> T2[request rewriting hidden]
  Deep --> T4[agentId wiring hidden]

Loading

candidate-5: Deepen the show-more toggle state into a helper

• Recommendation strength: Worth exploring
• Files:
  • packages/instantsearch.js/src/connectors/refinement-list/connectRefinementList.ts:240-256
  • packages/instantsearch.js/src/connectors/menu/connectMenu.ts (show-more block ~185-198)
  • packages/instantsearch.js/src/connectors/hierarchical-menu/connectHierarchicalMenu.ts (~214-235)
  • packages/instantsearch.js/src/connectors/rating-menu/connectRatingMenu.ts (~220-230)
• Problem: Every connector that supports showMore re-implements the same stateful caching dance: a mutable isShowingMore flag, a stable cachedToggleShowMore wrapper delegating to a reassignable toggleShowMore, and a createToggleShowMore(renderOptions, widget) factory that flips the flag and re-renders. The reason for the indirection — the render function must hand the caller the same function reference across renders so it only binds once — is a non-obvious invariant re-encoded in four connectors. The knowledge "how to expose a stable toggle over mutable show-more state" has no owning module.
• Proposed change: Extract a small helper that owns the show-more state and returns a stable toggle plus the current isShowingMore, given a re-render callback. Each connector instantiates it once and reads/exposes its outputs, dropping the hand-rolled cache/flag/factory triad.
• Benefits: Depth — the stable-reference invariant (the actual subtle part) lives behind a tiny interface. Locality — a change to how show-more re-renders happens in one place. Testability — the "same reference across renders" guarantee becomes directly unit-testable rather than implicitly relied upon in four suites.
• Risks: Low–medium. Must preserve reference stability exactly (regressing it would force callers to re-bind and could break memoized flavor wrappers). Connectors differ slightly in defaults (limit/showMoreLimit); the helper must not swallow those differences.
• Verification: yarn jest packages/instantsearch.js/src/connectors/{refinement-list,menu,hierarchical-menu,rating-menu}; yarn jest common-connectors; add a unit test asserting the toggle keeps a stable identity across renders and flips state.

Before:

flowchart LR
  Refine[connectRefinementList] --> S1[flag + cache + factory]
  Menu[connectMenu] --> S2[flag + cache + factory]
  Hier[connectHierarchicalMenu] --> S3[flag + cache + factory]

Loading

After:

flowchart LR
  Refine[connectRefinementList] --> Deep[createShowMoreState]
  Menu[connectMenu] --> Deep
  Hier[connectHierarchicalMenu] --> Deep
  Deep --> Inv[stable-reference invariant hidden]

Loading

Top Recommendation

Implement candidate-1 first. It has the highest locality (six adjacent connectors plus one new util), the smallest and lowest-risk diff (pure deduplication of an already-private function, no public interface change), and the clearest natural test surface (the per-widget emptiness sentinels become one module's unit tests instead of incidental connector coverage). It also establishes the "give a repeated connector rule an owning module" pattern that candidates 2 and 5 extend, making it a good sequencing anchor. If maximum depth-per-risk is preferred over safety, candidate-3 is the deepest single win (it removes an ordering invariant from callers) but it touches behavior-sensitive routing/SSR paths, so it is better as a follow-up once the safe dedup lands.

Next Step

To implement a selected candidate, run:

/implement candidate-1

Replace candidate-1 with the id of the candidate you want to implement (candidate-1 through candidate-5 from the shortlist above).

Non-Candidates

• Splitting AbstractChat.processStreamWithCallbacks (~680-line stream state machine in ai-lite/abstract-chat.ts). Genuinely the deepest friction in the repo, but it is not one reviewable PR — it entangles streaming, tool lifecycle, and partial-JSON accumulation, and chat is under active change. Too broad and too risky for this stage; candidate-4 carves off the safe, self-contained transport seam instead.
• Sharing SearchBox markup into instantsearch-ui-components. The same form/input/buttons/icons markup is duplicated across JS, React, and Vue, but CLAUDE.md documents that SearchBox layout is intentionally flavor-specific (IME/composition handling, refs, state). This conflicts with an existing decision and would be a large cross-flavor change, not a locality-preserving refactor.
• Making the insights sendEventToInsights a real seam / breaking the InsightsEvent reverse import from middlewares into connectors and utils. There is a real leak (connectors import a middleware type; the core keeps a noop stub the middleware overwrites at runtime), but untangling it touches middleware init, the automatic-insights path, and the event factory at once — closer to a broad rewrite than a small interface improvement. Left out to avoid inflating a speculative seam.
• Introducing a generic defer/debounce-based SchedulingManager in InstantSearch.ts. Consolidating scheduleSearch/scheduleRender/scheduleStalledRender is tempting, but the scheduling utils are already deep and the coordination is intertwined with status transitions and the render loop; extracting it risks a speculative abstraction with no varying adapter behind the seam.
• Building a single FacetRefinementConnectorFactory that all faceting connectors sit behind. This would fold candidates 1, 2, and 5 into one mega-module, but it is a broad rewrite of the connector layer with high review and migration risk. Prefer the incremental, independently-reviewable extractions above.