Architecture refactor scout: github-31366062308

[github-actions]

cc @algolia/frontend-experiences-web

Architecture Refactor Scout

Run: github-31366062308

Summary

I scouted the whole monorepo, concentrating on the layer where behavior actually lives — the shared connectors in packages/instantsearch.js/src/connectors/, the core runtime in packages/instantsearch.js/src/lib/, the routing/state-mapping seam, and the React/Vue wrappers. No CONTEXT.md or docs/adr/ exists. The dominant friction is shallow-connector orchestration: each connector hand-writes the same lifecycle, event-wiring, highlight-escaping, and show-more state machines against the low-level algoliasearch-helper interface, so identical orchestration (the place real bugs live) is copy-pasted across dozens of files while pure helpers were extracted only for testability. A secondary cluster sits in InstantSearch.ts, where the search status/stalled-render state machine is spread across a helper override, three defer-based methods, and the index widget. The strongest candidates deepen a small, self-contained module so a caller learns one flag or one helper instead of an ordering ritual. I deliberately down-ranked the "turn every hook into a factory" ideas — those are typing conventions, not depth wins (see Non-Candidates).

Candidate Shortlist

candidate-1: Consolidate escapeHTML / TAG_PLACEHOLDER handling into a deep highlight-escaping module

• Recommendation strength: Strong
• Files:
  • packages/instantsearch.js/src/lib/utils/escape-highlight.ts (owns TAG_PLACEHOLDER, escapeHits, escapeFacets)
  • Repeated across ~9 connectors: connectors/hits/connectHits.ts, connectors/infinite-hits/connectInfiniteHits.ts, connectors/refinement-list/connectRefinementList.ts, connectors/autocomplete/connectAutocomplete.ts, connectors/related-products/connectRelatedProducts.ts, connectors/frequently-bought-together/connectFrequentlyBoughtTogether.ts, connectors/looking-similar/connectLookingSimilar.ts, connectors/trending-items/connectTrendingItems.ts, connectors/trending-facets/connectTrendingFacets.ts
• Problem: Every highlight-aware connector re-implements the same three-part ritual against raw helper state. In connectHits.ts it is visible as (a) getWidgetSearchParameters doing state.setQueryParameters(TAG_PLACEHOLDER) when escapeHTML, (b) dispose clearing those keys with an Object.keys(TAG_PLACEHOLDER).reduce(...) loop, and (c) getWidgetRenderState calling escapeHits(results.hits). The caller (each connector) must know the placeholder keys exist, that they must be set on search and unset on dispose, and that results must be escaped only when non-empty. The interface (escapeHTML: boolean) is tiny but the required knowledge is not — a textbook shallow/leaky module. The source even flags it: // @MAJOR: set this globally, not in the Hits widget to allow Hits to be conditionally used in connectHits.ts.
• Proposed change: Move the set/clear/escape orchestration behind the highlight-escaping module so a connector expresses intent ("this connector escapes hits/facets") once, and the placeholder lifecycle across getWidgetSearchParameters/dispose/getWidgetRenderState is handled inside the deep module rather than copied per connector.
• Benefits: Locality — the placeholder-key knowledge and its ordering (set on search, clear on dispose) live in one testable module instead of nine. Leverage — a caller learns one flag, not a protocol. Testability — the escaping/placeholder invariants become the natural test surface in escape-highlight's own suite instead of being re-verified in every connect*-test.ts.
• Risks: Touches many connectors, so the diff spans files; must preserve exact dispose-clearing semantics (returning modified SearchParameters) to avoid leaking placeholders into persisted URLs. Snapshot/common tests for hits, refinement-list, autocomplete must stay green.
• Verification: yarn jest packages/instantsearch.js/src/connectors/hits packages/instantsearch.js/src/connectors/refinement-list packages/instantsearch.js/src/lib/utils/__tests__/escape-highlight; yarn jest common-widgets -t "Hits" and -t "RefinementList"; yarn type-check.

Before:

flowchart LR
  Hits[connectHits] --> Placeholder[TAG_PLACEHOLDER keys]
  Infinite[connectInfiniteHits] --> Placeholder
  Refine[connectRefinementList] --> Placeholder
  Hits --> Order[set-on-search / clear-on-dispose ordering]

Loading

After:

flowchart LR
  Hits[connectHits] --> Escape[Deep highlight-escaping module]
  Infinite[connectInfiniteHits] --> Escape
  Refine[connectRefinementList] --> Escape
  Escape --> Placeholder[TAG_PLACEHOLDER + ordering hidden]

Loading

candidate-2: Extract the show-more state machine shared by menu, hierarchical-menu, and refinement-list

• Recommendation strength: Strong
• Files:
  • packages/instantsearch.js/src/connectors/refinement-list/connectRefinementList.ts
  • packages/instantsearch.js/src/connectors/menu/connectMenu.ts
  • packages/instantsearch.js/src/connectors/hierarchical-menu/connectHierarchicalMenu.ts
• Problem: All three connectors copy the identical show-more state machine: a mutable isShowingMore flag, a toggleShowMore = () => {} seed, a cachedToggleShowMore() indirection so the callback is stable across renders, a createToggleShowMore(renderOptions, widget) that flips the flag and re-calls widget.render!, a getLimit() that switches between limit and showMoreLimit, and a canToggleShowMore computation (confirmed duplicated across exactly these three connectors plus their tests). This is orchestration, not pure logic — the stable-callback and re-render coupling is exactly where subtle bugs hide, yet it is triplicated rather than living in one deep module.
• Proposed change: Extract the show-more toggling/limit state machine into a single helper the three connectors instantiate, so each connector supplies its limits and a render trigger and receives back the render-state fields (isShowingMore, toggleShowMore, canToggleShowMore) without re-implementing the caching and re-render dance.
• Benefits: Locality — one place to fix a show-more re-render or stability bug instead of three. Leverage — connectors declare their limits, not the mechanics of stable callbacks. Testability — the state machine gets its own focused test instead of being re-asserted in three connect*-test.ts files.
• Risks: The three connectors compute canToggleShowMore from slightly different exhaustiveness inputs (facet vs. hierarchical); the helper must accept that difference without over-generalizing. Must keep the exact same callback identity guarantees so consumers relying on referential stability don't re-bind.
• Verification: yarn jest packages/instantsearch.js/src/connectors/refinement-list packages/instantsearch.js/src/connectors/menu packages/instantsearch.js/src/connectors/hierarchical-menu; yarn jest common-widgets -t "RefinementList" / -t "Menu" / -t "HierarchicalMenu"; yarn type-check.

Before:

flowchart LR
  Refine[connectRefinementList] --> SM1[show-more machine copy]
  Menu[connectMenu] --> SM2[show-more machine copy]
  Hier[connectHierarchicalMenu] --> SM3[show-more machine copy]

Loading

After:

flowchart LR
  Refine[connectRefinementList] --> SM[Deep show-more helper]
  Menu[connectMenu] --> SM
  Hier[connectHierarchicalMenu] --> SM
  SM --> Detail[toggle stability + limit switching hidden]

Loading

candidate-3: Deepen connector lifecycle wiring behind a shared connector factory

• Recommendation strength: Worth exploring
• Files:
  • Pattern across ~36 connectors in packages/instantsearch.js/src/connectors/*/connect*.ts (e.g. connectHits.ts, connectPagination.ts, connectSearchBox.ts, connectRefinementList.ts, connectInfiniteHits.ts)
  • packages/instantsearch.js/src/lib/utils/checkRendering.ts, createSendEventForHits.ts, createSendEventForFacet.ts
• Problem: Every connector hand-writes the same wrapper: init calls renderFn({ ...this.getWidgetRenderState(initOptions), instantSearchInstance }, true), render does the same with false, and getRenderState spreads getWidgetRenderState under a per-widget key. Event connectors additionally repeat the lazy if (!sendEvent) sendEvent = createSendEventForHits({...}) guard. The genuinely per-connector logic is only getWidgetRenderState + getWidgetSearchParameters; the surrounding init/render/getRenderState/event-caching is boilerplate the connector interface forces every author to re-learn. No shared factory exists today (verified). Deletion test: deleting the boilerplate would push identical code back into every connector, so the missing factory would earn its keep.
• Proposed change: Introduce a deep connector factory that takes the per-connector pieces ($$type, getWidgetRenderState, getWidgetSearchParameters, optional event setup) and produces the full widget object with init/render/getRenderState and event caching wired consistently. Migrate connectors onto it incrementally.
• Benefits: Leverage — a new connector supplies only what varies. Locality — lifecycle-timing and event-caching bugs get one home. It also becomes the natural place candidates 1 and 2 plug into (escaping and show-more as opt-in behaviors) rather than each connector re-wiring them.
• Risks: Broad blast radius (~36 files) — must be staged (land the factory + migrate a few connectors first) to stay reviewable, or it drifts toward a rewrite, which is out of scope for a single PR. Subtle per-connector deviations (e.g. connectHits emits view:internal after render; recommend connectors differ) must be expressible or left un-migrated. Type inference across the generic connector shapes is delicate.
• Verification: Migrate 2–3 connectors in the first PR; yarn jest packages/instantsearch.js/src/connectors; yarn jest common-connectors; yarn type-check; diff render-state output before/after with existing connector tests.

Before:

flowchart LR
  Caller[Each connect*] --> Boiler[init / render / getRenderState boilerplate]
  Caller --> Events[lazy sendEvent caching]
  Caller --> Logic[getWidgetRenderState only real part]

Loading

After:

flowchart LR
  Caller[Each connect*] --> Factory[Deep connector factory]
  Factory --> Boiler[lifecycle + event caching hidden]
  Caller --> Logic[supplies only getWidgetRenderState]

Loading

candidate-4: Extract the search status / stalled-render scheduler out of InstantSearch

• Recommendation strength: Worth exploring
• Files:
  • packages/instantsearch.js/src/lib/InstantSearch.ts (the mainHelper.search override, scheduleSearch, scheduleRender, scheduleStalledRender, fields status, error, _searchStalledTimer, _stalledSearchDelay)
  • packages/instantsearch.js/src/widgets/index/index.ts (calls scheduleStalledRender/scheduleRender from helper listeners)
  • packages/instantsearch.js/src/lib/utils/defer.ts
• Problem: The search-lifecycle state machine is smeared across the class. mainHelper.search sets this.status = 'loading' and calls scheduleRender; scheduleRender (a defer) resets status/error to idle only when there are no pending requests and clears _searchStalledTimer; scheduleStalledRender sets a setTimeout that flips status to 'stalled'; and the index widget reaches in to trigger stalled renders. To understand one state transition a reader must bounce between a helper override, three deferred methods, two instance fields, and a widget. The public surface (three schedule* methods + mutable status/error) is nearly as large as the behavior, and callers must know the ordering (loading → stalled → idle/error) that is only implicit.
• Proposed change: Pull the status transitions, the stalled timer, and the defer wiring into one deep scheduler that InstantSearch and the index widget drive through a small set of intent calls (search requested, results arrived, error), so status/error/timer state stops being directly mutated from multiple sites.
• Benefits: Locality — the stalled/idle/loading transitions and the 200ms delay live in one module with its own tests. Depth — callers signal events instead of manipulating timers and flags. Reduces the chance that a new call site forgets to clear the stalled timer.
• Risks: status/error are read by widgets and exposed via use/render state, so the extraction must preserve observable timing exactly; the mainHelper.search override is entangled with derived-helper search dispatch, so the seam must be drawn carefully to avoid pulling in unrelated search-dispatch logic. Higher review scrutiny than the connector candidates.
• Verification: yarn jest packages/instantsearch.js/src/lib/__tests__/InstantSearch; tests exercising status/stalled behavior and defer; yarn jest common-widgets smoke; yarn type-check.

Before:

flowchart LR
  Search[mainHelper.search override] --> Status[status/error fields]
  Index[index widget] --> Stalled[scheduleStalledRender]
  Stalled --> Timer[_searchStalledTimer]
  Render[scheduleRender] --> Status

Loading

After:

flowchart LR
  Search[mainHelper.search override] --> Sched[Deep render/status scheduler]
  Index[index widget] --> Sched
  Sched --> Status[status/error + stalled timer hidden]

Loading

candidate-5: Deepen the router middleware's uiState-route-uiState orchestration

• Recommendation strength: Speculative
• Files:
  • packages/instantsearch.js/src/middlewares/createRouterMiddleware.ts
  • packages/instantsearch.js/src/lib/routers/history.ts
  • packages/instantsearch.js/src/lib/stateMappings/simple.ts, singleIndex.ts, index.ts
• Problem: The router middleware is a thin connector that nonetheless carries real ordering knowledge: subscribe() must router.read() and merge into _initialUiState before wiring router.onUpdate(); createURL chooses between _initialUiState and live getWidgetUiState() based on whether widgets exist; and it manually merges previousUiState with nextState[indexId]. The stateMapping seam below it is nearly pass-through (simple.ts mostly strips the configure key), and the roundtrip invariant stateToRoute(routeToState(uiState)) ≈ uiState is assumed but unenforced. A caller writing a custom stateMapping or router must reconstruct this ordering from reading the middleware. Because two adapters (router, stateMapping) genuinely vary here, the seam should stay — but the orchestration between them is where knowledge leaks.
• Proposed change: Concentrate the read→merge→update→write ordering (and the initial-vs-live uiState decision) inside the middleware as a single deep flow, so the router and stateMapping adapters only implement their narrow contracts and callers stop needing to know the sequencing. Optionally surface the roundtrip expectation as an explicit check at the seam.
• Benefits: Locality — routing-sequence bugs (a common source of URL/UI drift) get one home. Depth — custom router/stateMapping authors implement small contracts without learning the orchestration. Keeps the genuinely-varying seam intact while hiding its choreography.
• Risks: Routing is history/SSR-sensitive; changes risk regressions in URL sync, browser back/forward, and Next.js integrations. Marked speculative because the exact deep-module boundary needs an implementation-stage mini-explore, and part of the friction is essayistic rather than a clean duplication to collapse.
• Verification: yarn jest packages/instantsearch.js/src/middlewares/__tests__packages/instantsearch.js/src/lib/routers; routing common tests; targeted E2E E2E_FLAVOR=react E2E_BROWSER=chromium yarn test:e2e for URL sync and back/forward; yarn type-check.

Before:

flowchart LR
  Caller[Custom router / stateMapping] --> Order[knows read/merge/update/write ordering]
  Mw[router middleware] --> Router[history.ts]
  Mw --> Mapping[stateMapping pass-through]

Loading

After:

flowchart LR
  Caller[Custom router / stateMapping] --> Contract[small adapter contract only]
  Mw[Deep routing orchestration] --> Router[history.ts]
  Mw --> Mapping[stateMapping]
  Mw --> Order[read/merge/update/write hidden]

Loading

Top Recommendation

Implement candidate-1 (consolidate escapeHTML / TAG_PLACEHOLDER handling) first. It scores highest on the rubric: depth (a tiny escapeHTML flag hiding a set-on-search / clear-on-dispose / escape-results protocol), locality (the placeholder-key knowledge collapses from ~9 connectors into the one module that already owns TAG_PLACEHOLDER and escapeHits), and leverage (every highlight-aware connector benefits, and the change makes the natural test surface the escaping module's own suite). The debt is already acknowledged in-code (// @MAJOR: set this globally, not in the Hits widget…), so the direction is uncontroversial. PR size is moderate and mechanical: identical small deletions per connector plus one deepened module, all covered by existing connector and common-widget tests. It is also lower-risk and more self-contained than candidate-3 (broad) or candidate-4 (timing-sensitive), making it the best first move.

Next Step

To trigger implementation of 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).

Non-Candidates

• "Turn the 32 React connector hooks into a factory." Each hook in react-instantsearch-core/src/connectors/ is a ~20-line typed binding of generics onto the already-deep useConnector. The real depth lives in useConnector.ts; a factory would erase per-connector TypeScript inference for no behavioral gain. This is a typing convention, not module-depth friction.
• "Migrate all legacy instantsearch.js/src/components/* widgets to instantsearch-ui-components." Real duplication exists (SearchBox/Pagination/RefinementList markup lives per flavor), but a full migration is a multi-PR program, not one reviewable change, and overlaps with the existing /port-widget workflow. Out of scope as a single depth refactor.
• "Add a validation layer to stateMappings." The stateMappings are near pass-through; adding runtime roundtrip validation introduces a seam where nothing yet varies enough to justify it, contradicting the rubric's "don't introduce a seam unless something varies." Folded into candidate-5 as an optional check rather than its own candidate.
• "Extract per-flavor widgetParams boilerplate in Vue components." The repeated widgetParams computed properties are thin prop→param maps; collapsing them yields marginal depth and mostly churns 30 components without hiding meaningful behavior.
• Dependency upgrades / formatting / algoliasearch-helper internals. Explicitly out of scope; per repo guidance, behavior changes belong in connectors, not the mature helper package.