Architecture refactor scout: github-30248671647

[github-actions]

cc @algolia/frontend-experiences-web

Architecture Refactor Scout

Run: github-30248671647

Summary

I scouted the whole monorepo, focusing on where behavior actually lives: the connectors in packages/instantsearch.js/src/connectors/, the runtime in src/lib/ (InstantSearch.ts, routing, insights), and the shared UI layer in instantsearch-ui-components. No CONTEXT.md or docs/adr/ exist. The recurring friction is shallow orchestration with leaked details: the same facet-connector bookkeeping (UI-state cleanup, maxValuesPerFacet, show-more state) is copy-pasted across six connectors instead of sitting behind one deep module; the search status/stalled state machine is smeared across InstantSearch.ts and the index widget as loose timers and manual status assignments; and the router+stateMapping seam splits normalization between createRouterMiddleware and history.ts. The strongest, most reviewable wins are the facet-connector consolidations and the stalled-search coordinator — each is high-locality, has a small natural test surface, and maps cleanly to one PR.

Candidate Shortlist

candidate-1: Deepen facet UI-state normalization

• Recommendation strength: Strong
• Files:
  • packages/instantsearch.js/src/connectors/refinement-list/connectRefinementList.ts (removeEmptyRefinementsFromUiState at ~569–589; maxValuesPerFacet at ~530–542)
  • 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–505)
  • packages/instantsearch.js/src/connectors/rating-menu/connectRatingMenu.ts (~479–496)
  • packages/instantsearch.js/src/connectors/breadcrumb/connectBreadcrumb.ts (~337–357)
  • New home under packages/instantsearch.js/src/lib/utils/
• Problem: Every facet connector defines its own removeEmptyRefinementsFromUiState(indexUiState, attribute) — six near-identical copies that only differ in the widget key (refinementList vs menu vs numericMenu…) and the "is this refinement empty?" test (.length === 0, === undefined, === ':', typeof !== 'number'). The same shape appears again for the maxValuesPerFacet bump in the three list-style connectors. This is pass-through bookkeeping: the knowledge of how a facet clears itself out of uiState is scattered instead of living in one place, so a bug in the cleanup logic must be fixed six times, and each connector's getWidgetUiState reader must re-derive the same invariant.
• Proposed change: Move the empty-refinement cleanup (and, as a second commit, the maxValuesPerFacet computation) into a single shared helper module that takes the widget key and an "is-empty" predicate, and have each connector call it. The connectors keep only their widget-specific predicate; the traversal/deletion/parent-pruning invariant lives once behind a small interface.
• Benefits: Locality — one place owns "remove an empty facet refinement from uiState without leaving dangling parent keys." Leverage — six call sites collapse onto one interface. Testability — the invariant becomes directly unit-testable instead of being re-verified through each connector's getWidgetUiState tests.
• Risks: Low. Pure functions, no lifecycle. Main care is preserving each connector's exact empty-check semantics (the ':' and typeof number cases are easy to get subtly wrong). Cross-flavor common connector tests already exercise the behavior.
• Verification: yarn jest packages/instantsearch.js/src/connectors/{refinement-list,menu,hierarchical-menu,numeric-menu,rating-menu,breadcrumb} and yarn jest common-connectors.

Before:

flowchart LR
  RL[connectRefinementList] --> C1[copy: removeEmpty + maxValues]
  Menu[connectMenu] --> C2[copy: removeEmpty + maxValues]
  HM[connectHierarchicalMenu] --> C3[copy: removeEmpty + maxValues]
  NM[connectNumericMenu] --> C4[copy: removeEmpty]
  RM[connectRatingMenu] --> C5[copy: removeEmpty]
  BC[connectBreadcrumb] --> C6[copy: removeEmpty]

Loading

After:

flowchart LR
  RL[connectRefinementList] --> Deep[facet uiState util]
  Menu[connectMenu] --> Deep
  HM[connectHierarchicalMenu] --> Deep
  NM[connectNumericMenu] --> Deep
  RM[connectRatingMenu] --> Deep
  BC[connectBreadcrumb] --> Deep
  Deep --> Detail[hidden: traversal + parent pruning + predicate]

Loading

candidate-2: Extract a stalled-search / status scheduling coordinator

• Recommendation strength: Strong
• Files:
  • packages/instantsearch.js/src/lib/InstantSearch.ts (_searchStalledTimer at 230/385/779; scheduleRender at 811–827; scheduleStalledRender at 829–836; status/error assignments)
  • packages/instantsearch.js/src/widgets/index/index.ts (the derived-helper 'search' listener that calls scheduleStalledRender())
• Problem: The search status state machine (idle → loading → stalled → idle) is not a module — it is a set of loose fields and timers on InstantSearch. scheduleRender both renders and conditionally clears _searchStalledTimer and resets status/error (811–820); scheduleStalledRender owns a raw setTimeout that flips status = 'stalled' (829–836); index/index.ts reaches back in to call scheduleStalledRender() on every derived 'search'; and dispose must remember to clearTimeout(this._searchStalledTimer) (779). To understand "why is the UI showing a stalled spinner," a reader must bounce between three regions and reconstruct the timing (_stalledSearchDelay) and the reset rules themselves. The status transition is an implementation detail leaked into the render scheduler and the index widget.
• Proposed change: Give the status/stalled concern its own module owned by InstantSearch: it holds the timer and the status/error transitions, exposes a small set of intent methods (e.g. "a search started," "a render happened," "dispose") and a readable status, and internally decides when to schedule a stalled render and when to reset. scheduleRender stops hand-managing the timer; index/index.ts signals search activity through the coordinator instead of poking scheduleStalledRender directly.
• Benefits: Depth — a genuine state machine (timer + transitions + reset invariants) sits behind a tiny interface. Locality — all stalled/status logic lands in one file with one test surface, and dispose can't forget the timer. Leverage — InstantSearch.ts shrinks toward pure orchestration.
• Risks: Medium — touches the core search lifecycle, so mis-ordering a transition could change when widgets see status. Mitigated by the existing InstantSearch lifecycle and stalled-search tests; keep the public status/error fields behaviorally identical.
• Verification: yarn jest packages/instantsearch.js/src/lib/__tests__/InstantSearch plus any stalled-search specs; confirm status timing is unchanged in common widget tests that assert loading/stalled states.

Before:

flowchart LR
  Index[index widget] --> Stalled[scheduleStalledRender timer]
  Render[scheduleRender] --> Stalled
  Render --> Status[status/error fields]
  Dispose[dispose] --> Stalled

Loading

After:

flowchart LR
  Index[index widget] --> Coord[status coordinator]
  Render[scheduleRender] --> Coord
  Dispose[dispose] --> Coord
  Coord --> Detail[hidden: timer + status transitions + reset]

Loading

candidate-3: Extract facet show-more state into one module

• Recommendation strength: Worth exploring
• Files:
  • packages/instantsearch.js/src/connectors/refinement-list/connectRefinementList.ts (~240–260, canToggleShowMore at ~431–435)
  • packages/instantsearch.js/src/connectors/menu/connectMenu.ts (~185–202, ~305)
  • packages/instantsearch.js/src/connectors/hierarchical-menu/connectHierarchicalMenu.ts (~216–239, ~398)
• Problem: The three list-style facet connectors each re-implement the same show-more bookkeeping: an isShowingMore flag, a cachedToggleShowMore/createToggleShowMore pair (to keep a stable function reference across renders), a getLimit() that switches between limit and showMoreLimit, and a canToggleShowMore derivation. Nine isShowingMore/cachedToggleShowMore/getLimit occurrences per connector confirm the copy. Callers of getWidgetRenderState must understand a subtle lazy-initialization dance (the toggle starts as a no-op and is reassigned mid-render). The interface each connector exposes is nearly as complex as the implementation — a shallow, duplicated pattern.
• Proposed change: Extract the show-more state (flag + stable toggle factory + getLimit + canToggleShowMore derivation) into one module the three connectors instantiate. Each connector supplies its limit/showMoreLimit/showMore config and the render trigger; the stability and limit-switching invariants live once.
• Benefits: Locality and testability of the toggle-stability + limit-switching logic; the three connectors keep only wiring. Removes a recurring source of subtle "stale toggle reference" bugs.
• Risks: Medium — the toggle factory closes over renderOptions/widget.render, so the extracted module must not leak that coupling back out; canToggleShowMore differs slightly (refinement-list factors in hasExhaustiveItems, menu/hierarchical use raw counts), so the predicate must stay caller-supplied. Overlaps thematically with candidate-1 but is a distinct PR (render state, not uiState).
• Verification: yarn jest packages/instantsearch.js/src/connectors/{refinement-list,menu,hierarchical-menu} and the show-more common tests.

Before:

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

Loading

After:

flowchart LR
  RL[connectRefinementList] --> Deep[show-more state module]
  Menu[connectMenu] --> Deep
  HM[connectHierarchicalMenu] --> Deep
  Deep --> Detail[hidden: stable toggle + limit switching]

Loading

candidate-4: Consolidate router + stateMapping orchestration

• Recommendation strength: Worth exploring
• Files:
  • packages/instantsearch.js/src/middlewares/createRouterMiddleware.ts (orchestration: topLevelCreateURL, subscribe/onStateChange, dedupe via isEqual)
  • packages/instantsearch.js/src/lib/routers/history.ts (writeDelay, cleanUrlOnDispose, windowTitle, parseURL/createURL, shouldWrite)
  • packages/instantsearch.js/src/lib/stateMappings/{simple,singleIndex}.ts
  • packages/instantsearch.js/src/lib/InstantSearch.ts (routing wired at construction, ~403–406)
• Problem: The router+stateMapping seam is leaky. Route de-duplication happens in the middleware (isEqual(lastRouteState, routeState)) while a separate shouldWrite check lives in history.ts; write normalization (writeDelay, cleanUrlOnDispose, windowTitle) sits on the router but the middleware owns the round-trip routeToState/stateToRoute composition and initial-state merge. Understanding one URL update means bouncing between InstantSearch.ts, createRouterMiddleware.ts, and history.ts, and the internal generic BrowserHistoryArgs<TRouteState> leaks out to react-instantsearch-router-nextjs. The round-trip invariant routeToState(stateToRoute(x)) ≈ x is documented but nowhere enforced.
• Proposed change: Pull the router↔stateMapping dance into one internal orchestration module that the middleware delegates to — it owns the compose order, the single de-dupe point, and lifecycle sequencing — leaving createRouterMiddleware as thin glue and keeping the public Router/StateMapping contracts untouched.
• Benefits: One place owns "map uiState↔route and decide when to write," so the two de-dupe checks unify and the normalization split closes. No public API change; flavors keep depending only on the middleware/router/stateMapping contracts.
• Risks: Medium — routing is timing-sensitive (subscribe runs before started, popstate ordering), so behavior parity must be proven against the existing routing/RoutingManager tests; larger surface than candidates 1–3, so scope carefully to stay one PR.
• Verification: yarn jest packages/instantsearch.js/src/lib/routers and the routing/RoutingManager middleware tests; smoke the Next.js router integration.

Before:

flowchart LR
  IS[InstantSearch] --> MW[router middleware]
  MW --> Dedupe1[dedupe: isEqual]
  MW --> Map[stateMapping compose]
  MW --> Hist[history.ts]
  Hist --> Dedupe2[shouldWrite + normalization]

Loading

After:

flowchart LR
  IS[InstantSearch] --> MW[thin middleware]
  MW --> Orch[routing orchestrator]
  Orch --> Detail[hidden: compose order + single dedupe + normalization]

Loading

candidate-5: Deepen the insights event-sending seam

• Recommendation strength: Speculative
• Files:
  • packages/instantsearch.js/src/lib/utils/createSendEventForHits.ts (payload building, chunking, "internal" click dedupe at ~166–194)
  • packages/instantsearch.js/src/lib/utils/createSendEventForFacet.ts
  • packages/instantsearch.js/src/middlewares/createInsightsMiddleware.ts (automatic/internal tagging, viewedObjectIDs dedupe)
  • Facet connectors that build filter payloads inline (connectRatingMenu, connectToggleRefinement, connectNumericMenu, connectMenu)
• Problem: To follow a single click event you hop through connector → createSendEventForHits → payload builder → sendEventToInsights → middleware → insights client. Payload shape, chunk size, "internal" de-dupe, and serialization are split between the two factory utilities and the middleware, and several facet connectors hand-assemble filters: [...] payloads inline. Callers know too much about payload structure and enrichment ordering (__queryID/__position must already be set), and the automatic-vs-manual insights distinction is invisible to connectors.
• Proposed change: Concentrate event-shape construction (and its escaping/chunking/dedupe) behind one seam so connectors emit an intent by type rather than assembling payloads, and the middleware consumes a uniform event. Keep a compatibility path for existing sendEvent/bindEvent callers.
• Benefits: Would hide payload/serialization details behind a small interface and remove the inline filter-payload duplication in facet connectors, concentrating event knowledge in one place.
• Risks: High/uncertain — insights is behavior-sensitive and widely relied on; the natural boundary isn't obvious yet and a full registry could balloon past a single reviewable PR. Marked Speculative: worth a focused mini-explore before committing, and likely better split (e.g. just the inline facet-filter payloads first).
• Verification: yarn jest packages/instantsearch.js/src/lib/utils/__tests__/createSendEventFor* and the insights middleware tests; yarn jest common-connectors for sendEvent coverage.

Before:

flowchart LR
  Conn[connectors] --> Hits[createSendEventForHits]
  Conn --> Facet[createSendEventForFacet]
  Conn --> Inline[inline filter payloads]
  Hits --> MW[insights middleware]
  Facet --> MW
  Inline --> MW
  MW --> Detail[tagging + dedupe + client]

Loading

After:

flowchart LR
  Conn[connectors] --> Deep[event seam]
  Deep --> MW[insights middleware]
  Deep --> Detail[hidden: payload shape + chunk + dedupe + serialize]

Loading

Top Recommendation

Implement candidate-1 (Deepen facet UI-state normalization) first. It is the cleanest match for the rubric: six connectors carry a near-identical removeEmptyRefinementsFromUiState (plus the maxValuesPerFacet bump in three of them), so the change has maximum locality (one new util owns the invariant), obvious leverage (six call sites collapse to one small interface), and real depth (parent-key pruning + the widget-specific empty predicate hide behind a tiny surface). The PR size is small and low-risk — pure functions with an existing cross-flavor test net — making it the safest first step before the higher-risk lifecycle work in candidate-2.

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

• Unified cross-flavor UI adapter for the create<Name>Component({ createElement, Fragment }) factories. Each flavor's glue (React hooks, Vue renderCompat/pragma shims, the Preact renderer) is fundamentally framework-specific; a single adapter would become a meta-framework. The only genuinely local sub-win — consolidating React's duplicated Highlight/ReverseHighlight/Snippet class-name wrappers — is presentation glue with little depth, so it doesn't clear the bar.
• Refactoring algoliasearch-helper. Per repo conventions it is mature and separately versioned; behavior fixes belong in connectors, not here.
• A broad InstantSearch.ts decomposition. Splitting scheduling + uiState sync + middleware orchestration together would be 500+ lines and unreviewable. candidate-2 deliberately carves out only the stalled/status concern.
• ai-lite/abstract-chat.ts (the largest file at ~54KB) and the chat connector/runtime. Recently added and still in flux; refactoring now risks churn against active development rather than deepening a settled module.
• Extracting init()/render() and getRenderState() boilerplate from every connector. Tempting (100% duplicated) but the widget factory isn't class-based, so any extraction adds an awkward higher-order wrapper for near-zero depth gain — pass-through, fails the deletion test.