Architecture refactor scout: github-30796046658

[github-actions]

cc @algolia/frontend-experiences-web

Architecture Refactor Scout

Run: github-30796046658

Summary

I inspected the InstantSearch monorepo with a focus on packages/instantsearch.js/src, where the shared connectors and the lib/ runtime live, plus the newer lib/ai-lite chat subsystem and the routing/state-mapping seam. The dominant friction is repeated shallow logic sitting in front of a missing deep module: several connectors independently re-implement the same normalization, decoration, and show-more state machinery, so each caller has to know rules (empty-refinement cleanup, insights metadata ordering, show-more limit toggling) that should sit behind one small interface. A parallel, smaller instance of the same pattern exists in the two stateMappings implementations, which duplicate the "strip configure" rule. The chat subsystem is deep but new and tangled; it has one localized extraction worth flagging but higher migration risk. I deliberately avoided the large lifecycle/scheduling rewrites in InstantSearch.ts/index.ts — real depth wins live there but not as one reviewable PR. The candidates below are ranked by locality and interface leverage per unit of review risk.

Candidate Shortlist

candidate-1: Consolidate insights hit/item decoration into one deep util

• Recommendation strength: Strong
• Files:
  • packages/instantsearch.js/src/connectors/hits/connectHits.ts (~180–191)
  • packages/instantsearch.js/src/connectors/infinite-hits/connectInfiniteHits.ts (~387–396)
  • packages/instantsearch.js/src/connectors/related-products/connectRelatedProducts.ts (182–191)
  • packages/instantsearch.js/src/connectors/looking-similar/connectLookingSimilar.ts (181–190)
  • packages/instantsearch.js/src/connectors/trending-items/connectTrendingItems.ts (202–211)
  • packages/instantsearch.js/src/connectors/frequently-bought-together/connectFrequentlyBoughtTogether.ts
  • packages/instantsearch.js/src/connectors/autocomplete/connectAutocomplete.ts
  • packages/instantsearch.js/src/connectors/answers/connectAnswers.ts
  • packages/instantsearch.js/src/lib/utils/hits-absolute-position.ts, hits-query-id.ts
• Problem: Eight connectors each chain addAbsolutePosition(...) and then addQueryID(...) in sequence before handing hits to createSendEventForHits/transformItems. The caller must know (a) that insights needs both __position and __queryID attached, (b) that they must be applied in that order, and (c) the correct positional arguments — search connectors pass (hits, page, hitsPerPage) while recommend connectors pass (hits, 0, 1). Two shallow utils plus an ordering rule are leaked into every caller; if insights ever needs a third field, all eight sites change.
• Proposed change: Introduce a single decoration util that owns the "attach insights metadata to results" concern and internally composes the existing position/query-id logic, so each connector makes one call with the values it already has. The two existing utils become the private implementation behind it.
• Benefits: Locality — the "what does insights need on a hit, and in what order" knowledge collapses into one module and its test. Leverage — one call replaces a fixed two-step chain in eight connectors. Testability — the ordering/positional invariants become one focused unit test instead of being re-asserted per connector.
• Risks: Recommend vs. search connectors pass different position arguments, so the interface must accommodate both without forcing a fake page/hitsPerPage; must preserve current output exactly (existing connectHitsWithInsights / connectInfiniteHitsWithInsights tests guard this). Low behavior risk — pure data decoration.
• Verification: yarn jest packages/instantsearch.js/src/connectors/hits packages/instantsearch.js/src/connectors/infinite-hits, the recommend connector suites, and yarn jest common-connectors.

Before:

flowchart LR
  Hits[connectHits] --> Pos[addAbsolutePosition]
  Hits --> Qid[addQueryID]
  Rel[connectRelatedProducts] --> Pos
  Rel --> Qid
  Trend[connectTrendingItems] --> Pos
  Trend --> Qid

Loading

After:

flowchart LR
  Hits[connectHits] --> Deco[decorate-for-insights]
  Rel[connectRelatedProducts] --> Deco
  Trend[connectTrendingItems] --> Deco
  Deco --> Pos[addAbsolutePosition]
  Deco --> Qid[addQueryID]

Loading

candidate-2: Extract shared removeEmptyRefinementsFromUiState util

• Recommendation strength: Strong
• Files:
  • packages/instantsearch.js/src/connectors/refinement-list/connectRefinementList.ts (569–589)
  • packages/instantsearch.js/src/connectors/menu/connectMenu.ts (403–420)
  • packages/instantsearch.js/src/connectors/hierarchical-menu/connectHierarchicalMenu.ts (508–528)
  • packages/instantsearch.js/src/connectors/numeric-menu/connectNumericMenu.ts (488–506)
  • packages/instantsearch.js/src/connectors/rating-menu/connectRatingMenu.ts (479–496)
  • packages/instantsearch.js/src/connectors/breadcrumb/connectBreadcrumb.ts (337–357)
  • new util under packages/instantsearch.js/src/lib/utils/
• Problem: Six connectors each define a private removeEmptyRefinementsFromUiState(indexUiState, attribute) with the identical three-step shape: bail if the widget's namespace is absent, delete the attribute if empty, delete the namespace if it becomes empty. The only variation is the namespace key (refinementList, menu, hierarchicalMenu, numericMenu, ratingMenu) and the emptiness predicate (array .length === 0 vs === undefined). This is normalization knowledge copied six times; a fix to the cleanup rule (e.g. a new falsy shape) must be applied in six places or it drifts.
• Proposed change: Add one shared util that takes the uiState, the namespace key, and the attribute (and, where needed, an emptiness predicate) and performs the delete-if-empty / delete-namespace-if-empty cleanup. Each connector deletes its local copy and calls the util in getWidgetUiState.
• Benefits: Locality — the uiState cleanup contract lives and is tested in one place. Leverage — a ~20-line private function in six files becomes one call site each. Testability — the delete-container edge cases become one parameterized unit test instead of six near-duplicate suites.
• Risks: The predicate variance (length === 0 vs undefined) must be preserved per connector; a naïve merge could change which entries survive. Behavior-preserving, so risk is contained; existing per-connector uiState tests guard it.
• Verification: yarn jest packages/instantsearch.js/src/connectors/refinement-list packages/instantsearch.js/src/connectors/menu packages/instantsearch.js/src/connectors/hierarchical-menu (plus numeric/rating/breadcrumb) and yarn jest common-connectors.

Before:

flowchart LR
  RL[connectRefinementList] --> R1[removeEmpty copy]
  Menu[connectMenu] --> R2[removeEmpty copy]
  HM[connectHierarchicalMenu] --> R3[removeEmpty copy]
  NM[connectNumericMenu] --> R4[removeEmpty copy]

Loading

After:

flowchart LR
  RL[connectRefinementList] --> U[removeEmptyRefinements util]
  Menu[connectMenu] --> U
  HM[connectHierarchicalMenu] --> U
  NM[connectNumericMenu] --> U

Loading

candidate-3: Extract the show-more state machine shared by facet connectors

• Recommendation strength: Worth exploring
• Files:
  • packages/instantsearch.js/src/connectors/refinement-list/connectRefinementList.ts (240–260, 422, 435–455)
  • packages/instantsearch.js/src/connectors/menu/connectMenu.ts (185–202, 254, 292–329)
  • packages/instantsearch.js/src/connectors/hierarchical-menu/connectHierarchicalMenu.ts (225–239, 307, 344–414)
• Problem: All three facet connectors independently maintain the same mutable show-more machinery: a let isShowingMore = false, a createToggleShowMore(renderOptions, widget) that flips the flag and calls widget.render(), a getLimit() returning isShowingMore ? showMoreLimit : limit, and a canToggleShowMore computation. The toggle/limit protocol (mutate flag → recompute limit → re-render) is caller knowledge duplicated across three connectors; a change to how show-more interacts with rendering means editing three state machines in lock-step.
• Proposed change: Extract the show-more behavior into one module that owns the isShowingMore state, exposes the toggle and the effective limit, and reports whether toggling is currently possible, given limit/showMoreLimit. Each connector composes it instead of re-declaring the trio of locals.
• Benefits: Locality — the show-more state transitions concentrate in one tested module. Leverage — roughly 40–60 lines per connector collapse into construction + a few method calls. Testability — toggle/limit/canToggleShowMore transitions become one unit test rather than being re-verified inside each connector's render tests.
• Risks: More invasive than candidates 1–2 because the state is mutable and threaded through render; the re-render call (widget.render()) and canToggleShowMore derivation differ slightly per connector (hierarchical uses hasMoreItems). Must keep the render trigger identical to avoid subtle re-render regressions.
• Verification: yarn jest packages/instantsearch.js/src/connectors/refinement-list packages/instantsearch.js/src/connectors/menu packages/instantsearch.js/src/connectors/hierarchical-menu and the show-more E2E/common suites.

Before:

flowchart LR
  RL[connectRefinementList] --> S1[isShowingMore + getLimit + toggle]
  Menu[connectMenu] --> S2[isShowingMore + getLimit + toggle]
  HM[connectHierarchicalMenu] --> S3[isShowingMore + getLimit + toggle]

Loading

After:

flowchart LR
  RL[connectRefinementList] --> SM[showMore state module]
  Menu[connectMenu] --> SM
  HM[connectHierarchicalMenu] --> SM

Loading

candidate-4: Share the configure-stripping rule between state mappings

• Recommendation strength: Worth exploring
• Files:
  • packages/instantsearch.js/src/lib/stateMappings/simple.ts (3–8, 23, 38)
  • packages/instantsearch.js/src/lib/stateMappings/singleIndex.ts (3–8, 18, 22)
  • packages/instantsearch.js/src/lib/stateMappings/index.ts
• Problem: Both state mappings define an identical private getIndexStateWithoutConfigure that strips the configure key when mapping between UI state and route state. The rule "configure never belongs in the URL" is copied into two adapters, and neither the shared StateMapping seam nor its tests document why. A caller reading either mapping cannot tell the exclusion is a shared invariant rather than a per-mapping choice.
• Proposed change: Move the configure-stripping into one shared helper the two mappings call, so the exclusion rule is expressed once at the state-mapping seam. Keep each mapping's index-shape logic (single vs. multi-index) local.
• Benefits: Locality — the "what gets excluded from the route" invariant lives in one place instead of two. Leverage — small, but it turns an implicit duplicated rule into a named, testable one. Testability — the exclusion becomes a single unit rather than being re-asserted in simple-test and singleIndex-test.
• Risks: Small surface, low risk; main care is not accidentally changing behavior for the multi-index case (singleIndex maps a single index name; simple iterates all indices). Existing state-mapping tests guard both.
• Verification: yarn jest packages/instantsearch.js/src/lib/stateMappings and the routing integration tests under lib/routers/__tests__.

Before:

flowchart LR
  Simple[simple stateMapping] --> C1[strip configure copy]
  Single[singleIndex stateMapping] --> C2[strip configure copy]

Loading

After:

flowchart LR
  Simple[simple stateMapping] --> Rule[strip-configure rule]
  Single[singleIndex stateMapping] --> Rule

Loading

candidate-5: Extract a ToolInputAccumulator from AbstractChat streaming

• Recommendation strength: Speculative
• Files:
  • packages/instantsearch.js/src/lib/ai-lite/abstract-chat.ts (~130–150 parseToolInputDelta, ~1167–1210 tool-input-delta handling, plus toolRawInputByCallId state)
  • packages/instantsearch.js/src/lib/ai-lite/stream-parser.ts (~88–104)
  • packages/instantsearch.js/src/connectors/chat/connectChat.ts (~569–576 shouldRepairToolInput)
• Problem: Accumulating streamed tool-call JSON fragments and deciding when to repair partial JSON is spread across three modules. AbstractChat holds the raw-input map and inline parse/repair logic, stream-parser extracts JSON from lines, and the connector supplies a shouldRepairToolInput predicate that depends on tool.streamInput — a streaming detail the connector shouldn't need to reason about. Understanding "how a tool's input is assembled mid-stream" means bouncing between all three.
• Proposed change: Move the raw-fragment map, the delta parsing, and the repair decision into one module that owns tool-input accumulation and exposes a small "feed delta / read current input" surface, so AbstractChat's stream handler and the connector stop touching the accumulation internals directly.
• Benefits: Locality — streaming/repair knowledge concentrates in one testable module. Leverage — the processStream tool-input branch shrinks and the connector's predicate loses its dependency on stream internals. Testability — repair edge cases (fragmented/invalid JSON) become direct unit tests instead of end-to-end stream tests.
• Risks: Highest of the five — this is new, actively evolving code with subtle streaming/ordering behavior and comparatively thin tests; a wrong extraction could regress tool-call parsing silently. Should be scoped carefully and paired with new focused tests.
• Verification: yarn jest packages/instantsearch.js/src/lib/ai-lite packages/instantsearch.js/src/connectors/chat and yarn jest common-widgets -t "Chat widget common tests".

Before:

flowchart LR
  Chat[AbstractChat.processStream] --> Raw[toolRawInput map + parse]
  Parser[stream-parser] --> Raw
  Conn[connectChat] --> Repair[shouldRepairToolInput]
  Repair --> Raw

Loading

After:

flowchart LR
  Chat[AbstractChat.processStream] --> Acc[ToolInputAccumulator]
  Conn[connectChat] --> Acc
  Acc --> Raw[raw fragments + repair]
  Acc --> Parser[stream-parser]

Loading

Top Recommendation

Implement candidate-1 (Consolidate insights hit/item decoration into one deep util) first. It has the strongest depth-per-review-risk ratio: it hides a genuine cross-cutting concern (what insights requires on a hit, and the required order of the two decoration steps) that is currently leaked verbatim into eight connectors, yet the change is pure data decoration with zero state or lifecycle involvement. Locality is high (one new util plus mechanical one-line edits per connector), leverage is high (eight call sites reduced to one call each, and future insights fields land in one place), and the existing connectHitsWithInsights/connectInfiniteHitsWithInsights and common-connectors suites already pin the expected output, so the PR is small, self-verifying, and low-risk. Candidate-2 is a close second and could reasonably follow in the same spirit.

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 above).

Non-Candidates

• Rewrite the InstantSearch/index search-scheduling lifecycle (InstantSearch.ts scheduling trio + index.ts helper-search overrides + _isolated branching). Real depth wins exist here, but they span multiple files and lifecycle phases and cannot land as one reviewable, behavior-preserving PR — rejected as too broad.
• A universal createConnectorWidget() factory to absorb every connector's init/render/getRenderState/getWidgetUiState/getWidgetSearchParameters boilerplate. Tempting given the repetition, but it touches 20+ connectors at once and reshapes the connector contract — too sweeping and too risky for a single PR.
• Centralized insights dedup policy across factory + middleware (createSendEventForHits, createSendEventForFacet, createInsightsMiddleware). The three dedup strategies are inconsistent, but unifying them changes cross-layer behavior and would need its own design discussion — out of scope for a locality-focused refactor.
• SSR hydration orchestration (lib/server.ts + InstantSearch.ts hydration + per-index render-state init). Genuine knowledge scatter, but the seam crosses three modules and an external calling contract; not a single-PR deepening.
• Generic configure-exclusion aside: only the two state mappings (candidate-4) share it cleanly; broadening it into a general route-schema/validator on StateMapping would be a speculative abstraction and is intentionally excluded.