Retire ValueNodeWrapper redirect useLayoutEffect: make getNextOrPrevious skip non-viable Tab targets

[CarlosNZ]

Summary

Teach getNextOrPrevious to skip non-viable Tab targets (filtered-out or non-editable nodes) so Tab navigation lands on a good node up front, and retire the redirect useLayoutEffect in ValueNodeWrapper that currently corrects the landing after the fact. This is the follow-up already flagged in the code comment ("See V2-roadmap §16 for the followup: hoisting filter-awareness into getNextOrPrevious...").

Current behaviour (the band-aid)

getNextOrPrevious (src/utils/keyboard.ts:152) is a purely structural walker: it returns the next/previous value node in document order, recursing into collections and skipping empty ones. It knows nothing about search-filter visibility or editability, so Tab can deposit the editing target on a node that can't host an editor.

That mis-landing is corrected reactively by the redirect useLayoutEffect in src/ValueNodeWrapper.tsx:199-217. When a node finds itself flagged isEditing but isn't (isVisible && canEdit) and isn't a forced edit, it bounces the target onward — getNextOrPreviousAtPath(tabDirection) → open(next), else previouslyEditedElement, else cancel(). Each hop re-runs the same check on the new node, chaining until it finds a viable node. useLayoutEffect (not useEffect) is load-bearing: it runs before paint so the open→eject→reopen sequence collapses into a single frame with no visible flicker.

Why it's worth removing

• It's a setState-during-useLayoutEffect (setState-after-render) pattern living in every value node.
• The open→eject→reopen path fires spurious startEdit/cancelEdit events on each hop — an onEditEvent observer sees churn for a Tab that's just passing through dead-end nodes.
• Decision-by-correction is harder to reason about than decision-up-front.

Note: the effect is only in ValueNodeWrapper. CollectionNode has no equivalent (it only uses isVisible for an early return null), so there's no duplication to untangle.

Proposed approach

Do not precompute visibility/editability for the whole tree. getNextOrPrevious already walks node-by-node, so instead pass it a single predicate and have it skip non-viable candidates as it walks, continuing in the same direction:

isViableTarget(nodeData) => boolean   // visible && editable

• visible: filterNode('value', nodeData, searchFilter, searchText) — already a pure call.
• editable: the canEdit derivation, currently per-node in useCommon. This is the one real lift — extract the restrictEdit → canEdit logic into something callable for an arbitrary nodeData. It's a pure function of nodeData + props, so extractable.
• force does not enter into it — Tab is never a forced edit.

getNextOrPrevious already extracts parentData and has key/value/path/index as it walks, so it can synthesise the nodeData each predicate call needs. The recursion's "skip and continue" structure already exists (it recurses to the parent when a destination is null), so adding "skip when non-viable" fits the existing shape.

The one caveat that stops it being a clean swap

The effect catches two triggers; Tab-awareness only covers the first:

1. Tab lands on a non-viable node — the main case; viability-skipping in getNextOrPrevious handles this fully.
2. An already-open node becomes non-viable reactively — e.g. typing in the search box filters out the node currently being edited.

After the recent canEdit gating of the custom-node setIsEditing props (see Related), direct entry can no longer target non-viable nodes (filtered-out nodes render null so aren't clickable; read-only nodes get a NOOP opener). So trigger 2 shrinks to just "a live search change hides the open node". That can be handled by a far smaller guard than the current redirect — a "if the active node goes invisible, cancel()" one-liner (no neighbour-walking, no setState-reopen) — or pushed into the editing store. Decide this as part of the work.

Suggested steps

1. Extract the canEdit derivation into a reusable isEditable(nodeData); compose isViableTarget = visible && editable.
2. Thread the predicate into getNextOrPrevious; skip non-viable candidates, continuing in tabDirection.
3. Replace the redirect useLayoutEffect with either nothing or a minimal "cancel-if-active-goes-invisible" guard (resolves trigger 2).
4. Keep the forced/imperative edit path (editorRef.startEdit → open(..., { force: true })) as-is.

Risk areas to verify

• Recursion edges: wrap-around at the document boundary, empty collections, and whether Tab navigates key edits too.
• The synthesised nodeData must match what restrictEdit / filterNode expect.
• getNextOrPrevious is already unit-tested (test/keyboard.test.ts) — add viability cases (Tab across filtered-out and read-only nodes; Tab off the end), regression-test first.

Related

Groundwork already landed on this branch: the custom-node setIsEditing props in ValueNodeWrapper and CollectionNode are now canEdit-gated, so a read-only custom node double-clicks to a clean no-op instead of relying on the redirect effect to eject it. That narrows trigger 2 above and is a prerequisite for removing the effect cleanly.

[CarlosNZ]

Update after #351 lands: the visibility half of the proposed isViableTarget predicate is now a precomputed Set<pathString> lookup, not a per-candidate filterNode call. The FilterStateProvider exposes visiblePaths (built once per (data, searchText, searchFilter) change at the JsonEditor level), so step 2 simplifies to threading that Set into getNextOrPrevious as a small predicate:

const isViable = (path) =>
  (filterState === null || filterState.visiblePaths.has(toPathString(path))) &&
  isEditable(path)

It also removes the risk of the Tab-walker's visibility check diverging from the renderer's (both now read from the same precomputed Set), and inherits the empty-collection / custom-filter bug fix landed in #351.

Trigger 2 ("active node becomes non-viable reactively") gets cleaner too — a useEffect watching useNodeVisible(activePath) that fires cancel() on flip, instead of the current redirect-and-re-run chain.

Step 1 (extract canEdit from useCommon) is unchanged — still the real lift.

[CarlosNZ]

Closed by #352