refactor(web): unify exploration action layer (ADR-500 / -083 / -034 affirmation)#363
Conversation
The generic `location ~* \.(js|css|...)$` rule applied `Cache-Control: public, immutable; expires 1y` to every JS file — including `config.js`, the runtime config written by docker-entrypoint.sh from env vars at container start. Browsers would keep the previous deploy's API URL, OAuth client ID, and app name for up to a year regardless of redeploys. Add an exact-match `location = /config.js` block with `no-store, no-cache, must-revalidate` ahead of the regex. Vite's content-hashed bundles under /assets/ still get the immutable rule, which is correct for them. First commit in the exploration-action-layer refactor — addresses the "loads from browser cache after restart" symptom independently of the exploration ledger work.
Lands the unified action layer for graph-mutating operations: loadExplore,
loadPath, followConcept, addAdjacent, removeNode, runCypher. Each method
mirrors the behavior of its current call site (SearchBar load handlers,
useGraphNavigation handlers, runCypher path), consolidated under one
invariant set:
1. Exactly one addExplorationStep per action.
2. loadMode 'clean' resets the exploration session before stepping.
3. Depth defaults are constants (FOLLOW_DEPTH, ADD_ADJACENT_DEFAULT_DEPTH,
DEFAULT_MAX_HOPS), not magic numbers per site.
No call site uses the hub yet — file is unused scaffolding. Subsequent
commits migrate SearchBar, ExplorerView click handling, and
useGraphNavigation one at a time. The QueryClient import is reserved for
the idempotent-re-search commit (no behavior yet).
Affirms ADR-500 (GraphProgram canonical execution) and ADR-083
(explorationSession canonical ledger).
handleLoadExplore, handleLoadPath, and handleExecuteCypher now delegate to useExplorationActions. Removes the inline addExplorationStep / setSearchParams / setRawGraphData / mergeRawGraphData / resetExplorationSession choreography that each handler did differently. Drops imports of stepToCypher, extractGraphFromPath, mapWorkingGraphToRawGraph, and statementsToProgram from SearchBar — those concerns now live in the hub. Behaviorally equivalent: the hub mirrors each handler's prior implementation. The visible payoff lands in later commits (idempotent re-search, persistence flip).
Both the primary and path-destination autocomplete dropdowns stayed visible after the user clicked a result, because the visibility predicate (`debouncedQuery && results && results.length > 0`) didn't know the parent had locked in a selection. The lingering dropdown overlapped the next controls (depth slider, Find Paths button) and required clicking outside to dismiss. ConceptSearchInput now takes an `isSelected` prop and dismisses the dropdown once it flips true. To support reconsidering without retyping, pressing Enter on a selected, non-empty input re-opens the dropdown with the existing cached results; another selection or any text edit dismisses it again. Both call sites (primary + destination) wire `isSelected`. Empty-state placeholder follows the same gating — it shouldn't pop up "No results" the moment after a successful pick.
…he hub handleFollowConcept, handleAddToGraph, and handleRemoveFromGraph each duplicated the exploration-step recording and graph-mutation logic that useExplorationActions now owns. They become thin wrappers that delegate to actions.followConcept / addAdjacent / removeNode, keeping the alert-on- failure UX layer that the context-menu callers rely on. useGraphNavigation's external shape is unchanged — 2D, 3D-V1, and 3D-V2 keep calling it without modification. handleTravelPath, handleSendToPolarity, and handleSendPathToReports stay in this file because they touch presentation concerns (camera animation, navigation, report creation) that don't belong in the action hub. The previously-unused `mergeGraphData` positional argument is now explicitly marked unused in the signature; callers still pass it for backward compatibility.
After any action that grows the graph (Add Adjacent / Follow / Load),
left- and right-clicks on V2 nodes stopped responding. Camera drag / pan /
zoom still worked, so the failure was scoped to InstancedMesh raycasting.
Root cause: `<instancedMesh args={[undefined, undefined, nodes.length]}>`
relies on three.js's constructor allocating `instanceMatrix` sized for the
initial `count`. When `nodes.length` grows, three.js doesn't reallocate
that buffer — it just bumps `count`. `computeBoundingSphere()` then walks
0..count reading past the array end, producing NaN bounds. The raycaster
early-outs against the NaN sphere and every node pointer event silently
misses.
The earlier 15-frame `boundingSphere = null` mitigation (commit 9eeec8a)
only helps while the sim is animating, and even then re-derives the bad
sphere from the same too-small buffer.
Fix: `key={nodes.length}` on the Nodes InstancedMesh forces React to
unmount and remount on count change, so three.js allocates a fresh
correctly-sized `instanceMatrix`. Same treatment applied to Arrows, which
has the same pattern keyed on edge `usableCount` — Arrows don't carry
pointer handlers but the wrong-size buffer still produces visual glitches.
Discovered while testing the exploration-action refactor: Add Adjacent
from the V2 right-click menu reproduced the bug deterministically.
…e entry
The persisted `rawGraphData` snapshot in localStorage was the root of the
"empty graph after follow-concept on a fresh browser" bug: a stale graph
rehydrates with concept IDs that no longer exist server-side, and any
action against those IDs gets an empty response that wipes the visible
graph. The snapshot also lets users interact with data that was already
gone, which is misleading rather than helpful.
Drop `rawGraphData` from the persist's partialize. The exploration
session (the durable ledger of what the user explored) is still persisted
and is now surfaced two ways:
1. As an "Autosave" entry pinned to the top of the saved-queries
panel — distinguished visually (primary tint + history icon), no
delete button, single slot at all times. Replaying it goes through
the same useQueryReplay path as any saved query, which executes
against the server and produces fresh data.
2. As a "Restore last session" call-to-action on the welcome state of
each explorer when no graph is loaded. Same replay path.
Implementation lives in a new `useAutosave` hook (synthesizes a
ReplayableDefinition from explorationSession on read) — no second
storage layer, no auto-save side effects, no API round-trip just for
per-browser session state. Clean loads (loadMode 'clean') already reset
the session, so the autosave disappears naturally when the user starts
fresh.
Affirms ADR-083 (explorationSession + useQueryReplay as the canonical
session + replay mechanism).
… server Re-search-the-same-term used to silently return React Query's cached result because every search hook had a multi-minute staleTime and no user action forced an invalidation. Rather than blanket-invalidating on every load (which would punish the common case where the user just wants to see the data they already loaded), expose an explicit 'Refresh' button in the search bar header. Click → `useQueryReplay.replayQuery(autosave)`, which executes the current exploration session as a GraphProgram (ADR-500) against `/query/program`. That path doesn't go through React Query's subgraph cache at all — it always returns fresh data by construction — so React Query's existing staleTime values for `useSubgraph` / `useFindConnection` / etc. are left alone. Button is hidden when there's no autosave (no session to refresh) and spins while replaying. Sits next to the mode dial so it's always visible across smart-search / block-builder / cypher-editor modes.
The right-click "Add Adjacent Nodes" item used to fetch at the hub's
default depth (1) with no way to ask for more. It now opens a submenu
with three depth choices: 1 (direct neighbors), 2, and 3. Each leaf
calls actions.addAdjacent(nodeId, {depth: N}) through the existing
useGraphNavigation → useExplorationActions chain.
Cap at 3: API enforces server-side, and depth-2/3 fetches are noticeably
slower on dense graphs — surfacing 4+ would invite multi-second clicks.
Touches the shared buildContextMenuItems helper, so 2D, 3D-V1, and 3D-V2
all pick up the submenu uniformly. Handler signature extended:
handleAddToGraph(nodeId, depth?) — depth is optional, undefined preserves
the previous default-depth behavior for any caller that doesn't supply it.
ExplorerView passed an `onNodeClick` prop that triggered a depth-2
add-adjacent + searchParams update. None of the three force-graph
explorers (2D, 3D-V1, 3D-V2) wire this prop — each handles left-click
locally with its own node-inspector logic (info panel for graph
explorers, document viewer for DocumentExplorer). The handler was
dead code.
Drop the callback, the `onNodeClick={handleNodeClick}` wiring, and the
now-unused `stepToCypher` import. The `ExplorerProps.onNodeClick` field
stays as an optional escape hatch for future explorers; this just stops
feeding it a phantom implementation that nobody calls.
Behaviorally invisible — no explorer's left-click behavior changes.
10 tests covering the hub's contract: every action records exactly one
exploration step, 'clean' loadMode resets the session before stepping,
depth defaults are honored, the context-menu depth selector (1/2/3) is
respected, removeNode emits a subtractive (-) cypher step, runCypher
resets the session and records one step per statement, and runCypher
short-circuits on an empty statement list.
Tests run against the real Zustand store — the invariants ARE about how
the store mutates, so mocking it would tautologize the assertions.
apiClient is mocked (vi.mock) since these tests don't need a running API.
Adds:
- web/vitest.config.ts — separate from vite.config.ts so the dev
container's bind-mount of vite.config.ts isn't disturbed.
- jsdom devDep — required by @testing-library/react renderHook.
- 'test' and 'test:watch' scripts in package.json.
Run: `cd web && npm test`. CI hook for this will follow once we have
a TypeScript test step alongside the existing TypeScript Build Check.
ReviewCritical (blocks merge)1. The 10-test invariant suite fails 10/10 in this checkout.
Root cause: The PR description's "Tests" section says "Run with Pick one of:
This isn't a flake or environment quirk — it's reproducible from a clean install of this branch. Should-fix (land on a follow-up before merge or as fast-follow)2. Hub docstring claims an invariant the implementation doesn't satisfy.
But The docstring is load-bearing — it's what the next contributor will trust when they decide whether their new action needs to add cache plumbing. 3.
Two options:
Option 1 is the right call — the hub's invariant should hold across the file boundary. 4. Stale comment in The wiring it cites was removed in commit 5. Remove
6. Drop
Nits7.
Questions for you (don't decide alone)8.
On replay through If the latter, an 9.
Verified per code-reading (not test-covered)
Persistence sequence (your #3). Dead-code claim in Autosave round-trip (your #2). SummaryThe hub design is right: the four divergent call sites collapsing into one is exactly the consolidation this codebase needed, and the per-step ledger invariants are clean. The But the test suite that's supposed to pin those invariants is non-functional in this checkout, the hub's headline invariant doesn't match its implementation, and one call site ( |
* Add localStorage shim setupFile so the 10-test suite passes on hosts where vitest's jsdom env doesn't pre-wire `globalThis.localStorage` before module load. Without this, Zustand's persist captures `undefined` and every `setItem` throws. Reproduced on a fresh checkout; tests now green on both host and dev container. * Hub docstring rewritten to match the actual fetch path: per-action calls intentionally do not invalidate React Query cache (the Refresh button covers always-fresh on demand). The aspirational `queryClient.fetchQuery` invariant misled review. * removeNode's recorded Cypher narrowed from `MATCH (c)-[r]-(n) RETURN c, r, n` to `MATCH (c) RETURN c` so autosave-replay subtracts ONLY the named node — the previous shape would silently expand removal to every 1-hop neighbour on every replay, diverging from the in-memory immediate behavior. * handleTravelPath now routes its graph mutation through `actions.loadPath`; the path fetch stays in the wrapper because the camera animation needs the path object. Restores the single-writer invariant (the previous direct `addExplorationStep` + `mergeRawGraphData` calls were a counter-example). * Drop unused `useQueryClient` import + `void queryClient` apologia from the hub. * Drop unused `_mergeGraphData` parameter from `useGraphNavigation`; update the three call sites in 2D / 3D-V1 / 3D-V2. * Migrate version gate `if (version < 1)` → `if (version === 0)` so future schema bumps don't accidentally re-run earlier migrations. * V2 left-click rationale comment refreshed — the original referenced ExplorerView's onNodeClick wiring removed in 1d39b4f. Drive-by: drop `stepToCypher` and `RawGraphData` imports from useGraphContextMenu — both became unused once handleTravelPath delegated to the hub.
|
Addressed in 5d65097:
Tests now green; TS clean; HMR clean. Ready for re-review or merge. |
1 participant
Summary
Unifies the four+ divergent call sites that mutated
rawGraphDataand wroteto
explorationSessioninto a single hub (useExplorationActions). Closesthe long-standing footguns that the divergent paths produced: re-search
silently returning cache,
cleanloads not resetting the session, thepost-restart "stale graph from localStorage" surprise, and (caught while
testing) V2 pointer events dying after any node-count growth.
This is a refactor that affirms existing architecture, not a new design
decision. No ADR required.
Affirms
execution model.
runCypherand the autosave's Refresh button both gothrough
apiClient.executeProgram.explorationSessionis now consistentlythe canonical ledger;
useQueryReplayis the canonical replay.context-menu builder and one action surface.
User-visible changes
through the server (always-fresh by construction, no React Query cache
invalidation needed). Hidden when there's no session to refresh.
session, restorable from any state. Clear-graph drops it; new clean
search starts a new one. Replaces the silent localStorage rehydrate
that produced empty graphs after follow-concept on stale IDs.
Restore last session as a one-click action.
context menu in all three explorers.
filled-in input re-opens the suggestions for reconsidering.
testing Add Adjacent in V2; was a latent InstancedMesh buffer bug.
Mechanism changes
useExplorationActionshook is the single writer for graph-mutatingactions:
loadExplore,loadPath,followConcept,addAdjacent,removeNode,runCypher. Each invariant — exactly one step recorded,cleanresets the session, depth defaults are constants — is enforcedin one place.
useGraphNavigationbecomes a thin wrapper that delegates to the hubfor follow / add / remove (preserving the alert-on-failure UX); other
presentation-layer concerns (camera travel, polarity nav, report nav)
stay where they were.
SearchBarload handlers are mechanical wrappers around the hub.ExplorerView.handleNodeClickremoved — no explorer wired it.The intentional reversal
rawGraphDatais dropped from the persist'spartialize. The previous"graph stays visible after reload" behavior was explicit (there's a
comment in ExplorerView saying so), but produced the worst class of bug:
the user clicks a node from the rehydrated graph, the API returns empty
for the now-stale ID, and the visible graph is wiped. The autosave entry
the persistence-flip commit; no ADR opened — discussed in PR description
as the appropriate weight.
A
version: 1bump +migratestrips the legacyrawGraphDatafromexisting browsers' localStorage so the fix applies on first load after
deploy, not just to fresh installs.
Tests
10-test suite (
web/src/hooks/useExplorationActions.test.tsx) pins thehub's ledger invariants. Run with
cd web && npm test. Adds Vitest config(
vitest.config.tsseparate fromvite.config.ts), atestscript, andjsdomas a devDep.Out of scope
runCypher).Commits (11)
Each commit is shippable on its own — migration sites move one at a time.
Merge
Regular merge (
--merge) — each commit documents a distinct step.