Architecture refactor scout: github-28359285057

[github-actions]

cc @algolia/frontend-experiences-web

Architecture Refactor Scout

Run: github-28359285057

Summary

I inspected the InstantSearch monorepo with a focus on packages/instantsearch.js/src (connectors, widgets, lib/ orchestration, routers, state mappings, middlewares) plus the shared instantsearch-ui-components layer and how the three flavors consume it. No CONTEXT.md or docs/adr/ exists. The dominant architectural friction is in the connector layer: connectors are the natural deep modules of this codebase (a small connectX(renderFn) interface hiding search/state/event behavior), but several recurring responsibilities have been copied inline across many connectors instead of living behind one interface. The clearest cases are hit post-processing (escape → position → queryID → transform) duplicated across 8 connectors, and uiState namespace-pruning duplicated across 6 facet connectors. A secondary cluster of friction is in URL routing, where the simple/singleIndex state mappings are near pass-throughs while createRouterMiddleware and the history router carry hidden orchestration (merge, dedup, write-conditionality) that callers cannot see. The cross-flavor createXComponent({ createElement, Fragment }) renderer-injection pattern is real boilerplate but spans three packages and ~45 files, so it is recorded as a non-candidate rather than a single reviewable PR.

Candidate Shortlist

candidate-1: Deepen hit post-processing into one connector-facing helper

• Recommendation strength: Strong
• Files:
  • packages/instantsearch.js/src/connectors/hits/connectHits.ts (lines 176-193)
  • packages/instantsearch.js/src/connectors/infinite-hits/connectInfiniteHits.ts
  • packages/instantsearch.js/src/connectors/related-products/connectRelatedProducts.ts (lines 179-196)
  • packages/instantsearch.js/src/connectors/trending-items/connectTrendingItems.ts (lines 199-216)
  • packages/instantsearch.js/src/connectors/looking-similar/connectLookingSimilar.ts
  • 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
  • helpers: lib/utils/escape-highlight.ts (escapeHits), lib/utils/hits-absolute-position.ts (addAbsolutePosition), lib/utils/hits-query-id.ts (addQueryID)
• Problem: Eight connectors independently repeat the same four-step pipeline before handing results to transformItems: conditionally escapeHits(results.hits) when escapeHTML, then addAbsolutePosition(hits, page, hitsPerPage), then addQueryID(hits, queryID), then transformItems(...). The ordering is an undocumented invariant — addAbsolutePosition must run before addQueryID, and both must run before transformItems or downstream insights events silently emit undefined positions/queryIDs (the insights exploration confirmed _buildEventPayloadsForHits reads __position/__queryID). Each caller must know this ordering, the escape gate, and the exact argument shapes of three utilities. The three utilities are individually shallow (each is a thin map), so the real knowledge lives in the orchestration that every connector re-copies.
• Proposed change: Introduce one connector-facing helper that takes the raw results (plus escapeHTML, transformItems) and returns the processed items, hiding the escape gate, the position/queryID enrichment, and their required ordering behind a single call. Connectors stop importing and sequencing escapeHits/addAbsolutePosition/addQueryID themselves. No public connector interface changes.
• Benefits: Locality — the escape→position→queryID→transform invariant lives in one place instead of eight. Leverage — a caller learns one call instead of three utilities plus their ordering. Testability — the ordering invariant becomes the natural test surface; today it can only be verified by reaching past each connector into the three pure utilities and asserting on enriched hits.
• Risks: Subtle per-connector differences must be preserved (e.g. recommend connectors operate on recommendations/hits and may skip transformItems defaults differently; connectAnswers and connectAutocomplete have their own shapes). Review must confirm the helper covers each variant rather than flattening them. Snapshot/insights tests across all eight connectors should be re-run.
• Verification: yarn jest connectors/hits connectors/infinite-hits connectors/related-products connectors/trending-items connectors/looking-similar connectors/frequently-bought-together connectors/autocomplete connectors/answers; confirm insights __position/__queryID payloads unchanged; run type-check.

Before:

flowchart LR
  Conn[8 connectors] --> Escape[escapeHits]
  Conn --> Pos[addAbsolutePosition]
  Conn --> QID[addQueryID]
  Conn --> Order[ordering invariant]

Loading

After:

flowchart LR
  Conn[8 connectors] --> Prep[processHits helper]
  Prep --> Escape[escapeHits]
  Prep --> Pos[addAbsolutePosition]
  Prep --> QID[addQueryID]
  Prep --> Order[ordering invariant hidden]

Loading

candidate-2: Consolidate uiState namespace-pruning behind one helper

• Recommendation strength: Strong
• Files:
  • packages/instantsearch.js/src/connectors/menu/connectMenu.ts (lines 403-420)
  • packages/instantsearch.js/src/connectors/refinement-list/connectRefinementList.ts (lines 569-589)
  • packages/instantsearch.js/src/connectors/hierarchical-menu/connectHierarchicalMenu.ts (lines 508+)
  • packages/instantsearch.js/src/connectors/numeric-menu/connectNumericMenu.ts (lines 488-505)
  • packages/instantsearch.js/src/connectors/rating-menu/connectRatingMenu.ts (lines 479-496)
  • packages/instantsearch.js/src/connectors/breadcrumb/connectBreadcrumb.ts (lines 337-357)
• Problem: Six connectors each define a private removeEmptyRefinementsFromUiState(indexUiState, attribute) with the identical shape — guard the namespace exists, delete the attribute when its value is empty, delete the namespace when it becomes empty, return. The only differences are the namespace key (menu, refinementList, hierarchicalMenu, numericMenu, ratingMenu) and the per-namespace "is empty" predicate (=== undefined vs !value || value.length === 0). This is duplicated knowledge about how IndexUiState is structured and pruned, copied per connector; a fix to the pruning rule (e.g. how empty arrays are treated) must be applied in six places and is easy to get subtly inconsistent — the variants already differ between connectMenu and connectRefinementList.
• Proposed change: Extract one shared lib/utils helper that prunes a given namespace+attribute from an IndexUiState, parameterized by the namespace key and an emptiness predicate. Each connector's getWidgetUiState calls it instead of carrying its own copy. The connector interface is unchanged; only the private duplicated function is removed.
• Benefits: Locality — the "prune empty refinement, then drop empty namespace" rule lives once. Leverage — connectors express intent (prune my namespace) without re-encoding IndexUiState traversal. Testability — one focused unit test covers the pruning semantics for all namespaces and emptiness variants, instead of six near-duplicate tests.
• Risks: The emptiness predicates genuinely differ per namespace, so the helper must accept that variation rather than assume one rule; review must confirm each connector's exact semantics are preserved (especially numericMenu/ratingMenu numeric emptiness vs refinementList array emptiness). Mutation-in-place behavior (these functions delete keys on the passed object) must be kept identical.
• Verification: yarn jest connectors/menu connectors/refinement-list connectors/hierarchical-menu connectors/numeric-menu connectors/rating-menu connectors/breadcrumb; assert getWidgetUiState output is byte-identical before/after for refined and cleared states; run type-check.

Before:

flowchart LR
  C1[connectMenu] --> P1[own pruning copy]
  C2[connectRefinementList] --> P2[own pruning copy]
  C3[4 more connectors] --> P3[own pruning copies]

Loading

After:

flowchart LR
  C1[connectMenu] --> H[pruneUiState helper]
  C2[connectRefinementList] --> H
  C3[4 more connectors] --> H
  H --> Detail[IndexUiState pruning rule]

Loading

candidate-3: Move URL-sync orchestration out of state mappings into the router middleware

• Recommendation strength: Worth exploring
• Files:
  • packages/instantsearch.js/src/lib/stateMappings/simple.ts (45 lines)
  • packages/instantsearch.js/src/lib/stateMappings/singleIndex.ts (26 lines)
  • packages/instantsearch.js/src/middlewares/createRouterMiddleware.ts (lines 46-125)
• Problem: The simple and singleIndex state mappings are near pass-throughs — both exist mainly to call getIndexStateWithoutConfigure (stripping configure), with singleIndex additionally hard-wiring an index name and a hidden default (routeToState({}) → { [indexName]: {} }). Meanwhile createRouterMiddleware carries the real orchestration that callers can't see: topLevelCreateURL (lines 47-72) reconstructs full uiState by branching on mainIndex.getWidgets().length === 0 and reducing nextState into a previousUiState (an effectively no-op re-spread), onStateChange hand-rolls dedup with a lastRouteState + isEqual guard, and subscribe mutates _initialUiState after construction with a load-bearing ordering requirement (it must run before widgets are added — the initialUiState warning documents users violating this). Knowledge of uiState reconstruction, dedup, and init ordering is split across the middleware and the state-mapping seam.
• Proposed change: Treat the state mapping as a thin adapter (uiState ⇄ routeState only) and pull the uiState reconstruction, write-dedup, and initial-state-merge ordering into the router middleware as one cohesive flow, so the middleware is the single place that owns "translate state and decide whether/what to write." Do not change the public stateMapping/router interfaces; the goal is to stop leaking orchestration into the mappings and to make the dedup/init ordering explicit in one module.
• Benefits: Locality — URL-sync ordering and dedup live in one module instead of being spread between middleware and mappings. Leverage — the state mapping shrinks to a clean adapter whose only job is the route shape. Testability — dedup and the initial-state-merge ordering become directly testable on the middleware rather than implied through mapping round-trips.
• Risks: Routing is heavily covered by RoutingManager tests and is a public, behavior-sensitive area (back/forward, initialUiState interaction, multi-index). Any change to the merge or dedup could shift URL output. The stateMapping interface is public, so the adapter contract must stay stable. Higher review burden than candidates 1–2.
• Verification: yarn jest lib/__tests__/RoutingManager middlewares/__tests__/createRouterMiddleware lib/stateMappings; manual e2e of URL sync (yarn test:e2e routing specs) for query/refinement/back-forward; confirm emitted URLs unchanged for representative states.

Before:

flowchart LR
  MW[router middleware] --> Recon[reconstruct + dedup + init order]
  MW --> SM[stateMapping]
  SM --> Strip[strip configure - shallow]

Loading

After:

flowchart LR
  MW[router middleware] --> Orch[owns reconstruct + dedup + init order]
  MW --> SM[stateMapping adapter]
  SM --> Shape[route shape only]

Loading

candidate-4: Make the history router's write() own its write-conditionality

• Recommendation strength: Worth exploring
• Files:
  • packages/instantsearch.js/src/lib/routers/history.ts (lines 178-191 write, 206-217 popstate, 284-311 shouldWrite)
  • caller: packages/instantsearch.js/src/middlewares/createRouterMiddleware.ts (line ~94, unconditional router.write)
• Problem: router.write(routeState) looks like a plain side effect to its only real caller (the middleware), but it is conditional on three pieces of mutable instance state that the caller cannot see: inPopState (set/cleared around popstate handling), latestAcknowledgedHistory (detects when another SPA library called pushState), and isDisposed/_cleanUrlOnDispose. During a popstate the pending writeTimer is cleared and inPopState is set, so the middleware's follow-up write() silently no-ops while the middleware believes the write succeeded. The conditionality is spread across write, shouldWrite, and the popstate handler, and is invisible at the seam.
• Proposed change: Consolidate the write-conditionality so write() presents a single honest contract — "give me a route state; I decide whether/when to commit it" — with the popstate/dispose/foreign-history state machine fully internal and, where useful, observable (e.g. the decision is computed in one place rather than read from scattered flags at call time). The public Router interface stays the same; this deepens the existing module rather than adding a seam.
• Benefits: Locality — the "should this URL actually be written" decision concentrates in one place. Leverage — callers stop needing a mental model of popstate/dispose timing to reason about whether a write lands. Testability — the decision becomes a single testable unit instead of an emergent property of timer + flag interplay.
• Risks: Highest behavior-change risk of the shortlist — SPA history handling has many edge cases (browser back/forward, dispose, coexistence with other routers, writeDelay). Regressions here are user-visible. Requires careful e2e coverage and is the least "mechanical" candidate.
• Verification: yarn jest lib/routers/__tests__/history; routing e2e specs including back/forward and dispose; verify no extra/missing pushState calls under popstate by asserting on history length transitions.

Before:

flowchart LR
  MW[middleware] --> Write[write - looks unconditional]
  Write --> Flags[inPopState / isDisposed / history len]
  Write --> Maybe[silent no-op]

Loading

After:

flowchart LR
  MW[middleware] --> Write[write - owns the decision]
  Write --> SM[internal write-decision state machine]

Loading

candidate-5: Fold per-connector sendEvent lazy-init into the render-state helpers

• Recommendation strength: Speculative
• Files:
  • packages/instantsearch.js/src/connectors/menu/connectMenu.ts (lines ~234-263)
  • packages/instantsearch.js/src/connectors/refinement-list/connectRefinementList.ts (lines ~378-389)
  • packages/instantsearch.js/src/connectors/hierarchical-menu/connectHierarchicalMenu.ts
  • packages/instantsearch.js/src/connectors/rating-menu/connectRatingMenu.ts
  • helper: packages/instantsearch.js/src/lib/utils/createSendEventForFacet.ts
• Problem: createSendEventForFacet is already a deep helper (it owns the isFacetRefined "only send when checked ON" rule, the custom-payload branch, and dev-mode validation). The leak is at the call sites: each facet connector declares a let sendEvent and lazily memoizes it inside getWidgetRenderState (if (!sendEvent) { sendEvent = createSendEventForFacet({...}); }), then hand-wires the refine callback to call it. The same lazy-init-for-stable-identity ceremony recurs alongside the broader connectorState memoization (refine/createURL/sendEvent) seen in 15+ connectors, so callers re-encode the "memoize once across renders" contract by hand.
• Proposed change: Provide a small render-state helper that owns the lazy-once memoization of sendEvent (and potentially refine/createURL) so connectors declare intent rather than re-implement the if (!x) x = ... pattern. Scope this PR to the facet connectors only to keep it reviewable. Because createSendEventForFacet is already deep, the net gain is removing call-site boilerplate, not adding new behavior — hence Speculative.
• Benefits: Locality — the memoize-once contract lives in one helper. Leverage — connectors stop carrying mutable let sendEvent state. Testability — stable-identity behavior across renders becomes a single test.
• Risks: Borders on a speculative abstraction (rubric caution): the win is modest and the existing pattern is load-bearing. If the helper tries to also absorb refine/createURL it risks over-generalizing across connectors that differ. Keep narrow or skip.
• Verification: yarn jest connectors/menu connectors/refinement-list connectors/hierarchical-menu connectors/rating-menu; assert sendEvent/refine referential stability across re-renders is preserved; run type-check.

Before:

flowchart LR
  C[facet connectors] --> Let[let sendEvent + lazy init]
  Let --> Util[createSendEventForFacet - deep]

Loading

After:

flowchart LR
  C[facet connectors] --> Helper[render-state memo helper]
  Helper --> Util[createSendEventForFacet - deep]

Loading

Top Recommendation

Implement candidate-1 (deepen hit post-processing into one connector-facing helper) first. It scores highest on every rubric axis: depth (a single small call hides escape-gating, position/queryID enrichment, and their required ordering — behavior that today is re-derived in 8 callers), locality (all changes are inside packages/instantsearch.js/src/connectors plus one new lib/utils helper), and leverage (it removes the most-repeated invariant in the connector layer and the one with a real silent-failure mode — missing __position/__queryID breaks insights). The PR is bounded and mechanical: introduce one helper, replace eight inlined pipelines, and the existing per-connector tests plus an insights payload assertion fully pin the behavior. candidate-2 is an equally clean second once this lands.

Next Step

To implement a selected candidate, run:

/implement candidate-1

Replace candidate-1 with the id of the candidate you want to build (candidate-2 through candidate-5).

Non-Candidates

• Cross-flavor createXComponent({ createElement, Fragment }) renderer-injection boilerplate. Repeated ~45 times across instantsearch.js, react-instantsearch, and vue-instantsearch (each with a createElement as Pragma cast and a per-flavor pragma shim). Real friction, but it spans three packages and dozens of files and the seam genuinely earns its keep (multi-renderer support), so it is not a single reviewable PR and risks becoming a broad rewrite.
• Rewriting the connector init/render → renderFn(getWidgetRenderState(...), isFirstRendering) contract. The boilerplate is real across 33 connectors, but it is the load-bearing public connector interface; changing it touches every connector and every flavor widget — too broad and not a depth win.
• Per-flavor prop adapters (React Hits.tsx, Vue Hits.js, UI-component Hits.tsx). Each flavor hand-translates className/slot/onClick shapes. This is inherent to bridging different rendering models and is per-component, not one localized deep module.
• InstantSearch.ts lifecycle (start/scheduleSearch/scheduleRender/setUiState) and the index-widget helper overrides. Genuine god-module orchestration, but it is the highest-risk core of the library and any refactor would be large and behavior-sensitive — not a small, reviewable, locality-bounded PR.
• Splitting _buildEventPayloadsForHits / per-event-type payload builders in createSendEventForHits.ts. The function is already reasonably deep and lives in one file; extracting builders would be churn without a clear interface-depth gain.

[FabienMotte]

/implement candidate-1

[github-actions]

Working on candidate-1.

Workflow run: https://github.com/algolia/instantsearch/actions/runs/28933997106

I'll open a draft PR here if the implementation produces changes.

[github-actions]

Architecture refactor workflow failed while working on candidate-1.

Workflow run: https://github.com/algolia/instantsearch/actions/runs/28933997106