Skip to content

Architecture: should we refactor explorer.qmd before the next big feature? (analysis + options) #249

@rdhyee

Description

@rdhyee

The question

With several substantial features recently landed or in flight in explorer.qmd
— heatmap mode (#241, #242, #246), the collection facet (#244), cross-filter
facets (#156), URL-state contract (#163/#164) — is the explorer codebase messy
enough that we should refactor before adding the next big feature, or keep
shipping incrementally?

This issue is the analysis to decide that, not a proposal to rewrite.

Diagnosis (measured)

explorer.qmd is ~4,580 lines across 9 OJS cells. The complexity is not
uniform — it's concentrated in one cell:

Block ~Lines Health
~40 helper functions (facetFilterSQL, readHash/buildHash, card renderers, bbox math, …) 730 ✅ small, single-purpose
viewer cell (Cesium setup + click/hover handlers) 270 ✅ cohesive
facetFilters cell 105 ✅ fine
tableView cell 455 ⚠️ large but cohesive
zoomWatcher cell ~2,200 ❌ god-closure
perfPanel cell 95 ✅ fine

~48% of the file is a single closure. zoomWatcher co-mingles: H3 cluster
loading (loadRes), point-mode sample loading (loadViewportSamples), mode
transitions (enterPointMode/exitPointMode/applyLayerVisibility), the
heatmap subsystem (refreshHeatmap/renderHeatmap/clearHeatmap + imagery
layer), camera (moveEnd/camera.changed) handlers, facet-count refresh, and
several stale-request guards. Recent features (heatmap, collection facet) mostly
landed in or adjacent to this cell.

Why it's monolithic (it's the platform, not just neglect)

Quarto OJS has no ergonomic way to share mutable state across cells, so
stateful, tightly-coupled logic tends to collapse into one closure (the shared
loading, requestId, loadResGen, heatmapReqId, currentRes, etc.).
EXPLORER_STATE.md already documents the escape hatch — hoisting state onto
viewer._globeState / viewer._heatmap* so logic can be split across cells.

The risk / tension

explorer.qmd is dense with hard-won, issue-specific fixes. A non-exhaustive
list visible in the comments: #185 (terrain depth-test bypass), #221 (viewport
padding so map/table/count agree), #233 (heatmap mutual-exclusivity + self-
refresh race), #156 (cross-filter pid IN (...) predicate), _suppressHashWrite
(camera↔hash echo prevention), loadResGen/requestId/freshSelectionToken
(stale-result guards), #127 (TOC overlap intercepting clicks).

A big-bang rewrite is precisely the move that silently drops one of these guards
and reintroduces a solved bug. The cost of a regression here is high (the page is
the project's flagship demo).

What we already have for safe decomposition

Two of the three prerequisites for a low-risk refactor are already in place:

  • Test net: ~24 test files under tests/ (Playwright + pytest), plus a
    search-perf baseline.
  • State contract: EXPLORER_STATE.md defines URL params, hash channel,
    DOM-as-state, and viewer.* widget state — the seams along which logic can be
    separated.
  • Module-extraction precedent: the explorer already imports an ES module at
    runtime — await import('assets/js/source-palette.js'). Pure logic can move
    into assets/js/*.js the same way and become independently unit-testable.

Options

A — Refactor-first (restructure explorer.qmd before the next feature).
Cleaner base for future work. Cons: high regression risk against the dense
fix-set above; a large, hard-to-review diff; delays features; and it collides
with any concurrent work in this same file. Not recommended now.

B — Don't refactor; keep shipping. Fastest short term, lowest immediate
risk. Cons: zoomWatcher keeps accreting; each subsequent change gets slower and
riskier. Defers the problem rather than addressing it.

C — Strangler / refactor-as-you-go (recommended). Decompose incrementally
behind the existing test net:

  1. Strengthen tests around whatever the next feature touches (cheap, compounding).
  2. Extract pure logic into assets/js/*.js ES modules — SQL builders
    (facetFilterSQL, sourceFilterSQL, textSearchWhere/Score), the hash
    codec (readHash/buildHash), bbox math (paddedViewportBounds/
    viewerBboxSQL), card HTML renderers, escapers. Pure functions → easy unit
    tests, ~500 lines out of the qmd, near-zero risk.
  3. Later, last, one seam at a time: carve the heatmap subsystem out of
    zoomWatcher into its own cell/module (its state is already namespaced
    viewer._heatmap* / heatmapImageryLayer). Highest-risk seam — only behind
    green tests.

Recommendation

Don't refactor-first; adopt C. The codebase hasn't crossed the
"stop-and-rewrite" line — it's one bloated closure plus otherwise-clean code,
with a real test net and a state contract. Highest-leverage, lowest-risk path is
opportunistic module extraction that lets zoomWatcher shrink across a few small
PRs, rather than blocking features on a big restructure.

Proposed next step

Before committing to any option, produce a refactor map: enumerate every
extractable seam in zoomWatcher (and the pure helpers), risk-rated and in
dependency order, with the target module/cell for each. Low commitment; doubles
as the execution plan if we pick C.

Open questions for the team

  1. Is the single-file Quarto-OJS approach a long-term fit, or should the explorer
    eventually become a bundled app (e.g. a Vite entry embedded in the page) with
    real modules + a test harness? (Bigger fork; out of scope for C but worth
    naming.)
  2. How do we coordinate refactor PRs with concurrent feature work in
    explorer.qmd to avoid merge churn? (Refactor windows? Feature freeze on the
    touched seam?)
  3. Is EXPLORER_STATE.md the right home to also document the module boundaries
    as they're extracted?

Analysis generated while landing #244 / #246; line counts are against main at
the time of writing.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type
    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions