explorer: extract _selGen freshness primitive (post-mortem on PR #186 6-round Codex loop)

[rdhyee]

Background

PR #186 (cluster URL state, &h3=<cell>) went through six rounds of Codex review. Every round surfaced at least one P2 issue, all of them resolved before merge. This issue is a post-mortem: was that iteration count a sign of architectural rot, scope creep, or genuinely complicated interactions — and is there a small refactor that would have prevented most of it?

Filed at the author's request after a meta-conversation with Claude Code; will post Codex's independent take as a follow-up comment, then we discuss.

Author's question (verbatim)

are the 6 rounds of iterations needed to converge a sign of the need for some architectural refactoring, creeping complexity, or there are just a lot of darn complicated interactions that need trial and error to surface?

Claude Code's analysis

Honest read: ~80% of the findings are the same class of bug — async ops mutating shared state where the state can change mid-await — surfaced in different code paths. The remaining ~20% is a data-layer surprise (UBIGINT precision) plus a doc lag.

Looking at it that way:

Round	Surface	Class
R1	hashchange handler	async + shared-state race
R2	hydrateClusterUI internal await	same
R3a	boot deep-link	same
R3b	fetchClusterByH3 ignoring source filter	data consistency
R4a	source-filter doesn't invalidate selection	same
R4b	suppressHashWrite leak on early-return	cleanup edge
R5a	rehydrate-on-filter when meta truthy	same data consistency
R5b	UBIGINT > 2^53 → Number precision-loss	data-layer surprise
R6	stale docs	nit

So this was mostly one problem reappearing in different surfaces, not creeping complexity from feature pile-up.

The actual architectural smell

There's no single "selection store." Every path that touches selection (click handler, hashchange, boot, source-filter handler) imperatively does its own DOM writes, URL writes, and _globeState mutations. The _selGen token is a poor person's race guard duct-taped onto each path. We're now using it in four places.

A small refactor would prevent future bugs of this class

1. withFreshness(token, fn) — a wrapper that auto-bails the inner function's DOM mutations if the token's generation has bumped. ~15 lines.
2. (Optional, larger) consolidate selection mutations into a selectSample(pid) / selectCluster(h3) / clearSelection() trio that handles state + URL + DOM in one place.

Step 1 alone would have prevented R2 + R5a (the "internal await forgot to check freshness" bugs). Step 2 would prevent R3b/R4a/R5a (the "filter-X-doesn't-do-Y" bugs).

Wholesale rewrite (Redux/state machine): not justified. The OJS-imperative idiom is the project's convention; readers expect it.

Bottom-line answer

A bit of all three, but mostly the third. Six rounds reflect Codex doing what humans rarely do — re-reading the diff after each fix and spotting the next missed edge in the same family. We were genuinely converging, just not in straight-line steps. Worth filing this small follow-up so future selection-state work can't trip the same wire.

Refs

• PR explorer: persist selected cluster identity in URL via &h3=<cell> (Phase 1) #186 (cluster URL state, where the 6-round loop happened)
• EXPLORER_STATE.md (current state contract)
• EXPLORER_CLUSTER_URL_PROPOSAL.md (companion design doc)

Next step

Codex's independent take will be posted as a comment. Then discussion + decision on whether to scope the refactor (and at what size) before any code.

[rdhyee]

Codex take after reading the post-mortem:

I agree with the issue framing. The six review rounds are less evidence of broad architectural rot and more evidence that one invariant is currently too distributed: async selection work must not mutate runtime state, URL state, or side-panel DOM after a newer selection/filter/hash event has superseded it.

The round-by-round table makes the pattern clearer than my initial answer. Most findings were not independent complexity explosions; they were the same freshness/state-consistency problem showing up in hashchange, boot hydration, source-filter revalidation, and helper-internal awaits. The UBIGINT precision issue was a separate data-layer surprise, and the final doc issue was normal drift.

Adjusted recommendation:

1. Do extract a small freshness primitive soon, preferably before the next Explorer selection-state feature. This is justified now.
2. Keep it deliberately small. A Redux/state-machine rewrite is not warranted.
3. Treat a larger selectSample / selectCluster / clearSelection controller as optional phase 2 if the next feature again needs to touch click, hashchange, boot, filter, and URL code paths.

What I would want from phase 1 is a tiny helper that makes the safe pattern hard to forget. For example:

function beginSelectionWork(viewer) {
  viewer._selGen = (viewer._selGen || 0) + 1;
  const gen = viewer._selGen;
  return () => gen !== viewer._selGen;
}

or, better, a wrapper around awaited work / DOM commits so callers do not have to remember to check after every await. The exact shape can follow the existing imperative OJS style; the point is to centralize the generation-token contract and document when it must be used.

I would not block PR #186 on this refactor if the current behavior is verified. PR #186 exposed the need; issue #187 is the right follow-up. The practical next step is to scope phase 1 to _selGen freshness only, update EXPLORER_STATE.md with the async-selection invariant, and add one regression test that exercises a stale async selection path if Playwright can do so reliably.

[rdhyee]

Synthesis (Claude Code response to Codex)

Codex's take refines mine in two ways I agree with. Tracking convergence + my response below for the record.

Where we converge

• Small withFreshness(token, fn) primitive around _selGen: yes.
• No wholesale state-machine / Redux rewrite: yes — the OJS-imperative idiom is the project's convention.
• Don't block PR explorer: persist selected cluster identity in URL via &h3=<cell> (Phase 1) #186; treat this issue as the follow-up.
• The root cause is one invariant that's too distributed, not architectural rot.

Where Codex sharpened my framing

• YAGNI gate on Step 2 (selectSample / selectCluster / clearSelection controller). I had it as "optional"; Codex correctly tightens that to "only if the next selection feature again has to touch click, boot, hashchange, filter, and URL paths." Three call sites isn't enough to force consolidation.
• Make the invariant explicit, twice over:
  1. Add the async-selection invariant to EXPLORER_STATE.md so future readers don't have to derive it from code.
  2. Add a Playwright regression for stale async selection if it can be exercised reliably.

My take on Codex's additions

• Doc + invariant: unambiguous win, cheap.
• Playwright regression test: good idea but timing-flaky in headless. The race window is between an await's start and resolution; we can artificially widen it with a debug-only delay flag, but that ships test-only code in prod. I'd write a manual DevTools recipe for now, file a sub-issue if we want the paper trail, and revisit when something forces our hand.

Plan for the immediate follow-up

1. Extract withFreshness(token, fn) in explorer.qmd. Apply at: hashchange (pid + h3 branches), boot (pid + h3 branches), source-filter (cluster + sample re-validation). Estimated ~15 lines for the primitive + 4 call-site edits.
2. Add a paragraph to EXPLORER_STATE.md §4 (Widget-internal state) pinning the invariant verbatim: any async selection work must check _selGen freshness before mutating _globeState, the URL hash, or the side-panel DOM; use the withFreshness wrapper.
3. Defer Playwright test, with a short manual repro recipe in the docs.

Will open a small PR shortly.

[rdhyee]

For the record: this issue auto-closed via PR #188's "closes #187" trailer. Status:

• Step 1 shipped: freshSelectionToken primitive at top-level OJS scope, isStale() checks at all five user-input handler call sites (click, hashchange, source-filter, boot pid, boot h3) plus inside hydrateClusterUI. EXPLORER_STATE.md invariant added.
• Step 2 deferred: tracked in explorer: consolidate selection mutations into a controller (deferred Step 2 of #187) #189 with a YAGNI gate. Don't act on it until the next selection feature or selection-mutation bug forces our hand.
• Playwright stale-async regression: deferred too; manual DevTools recipe is sufficient until the test surface stabilizes.