Tab viability: skip non-viable targets, retire redirect useLayoutEffect (#334)

.changeset/editing-store-dead-state-cleanup.md

@@ -0,0 +1,5 @@
+---
+'json-edit-react': patch
+---
+
+Internal cleanup: remove the now-unused `previouslyEditedElement` / `recordPreviousEdit` and `tabDirection` / `setTabDirection` plumbing from the editing store. These were only ever consumed by the redirect `useLayoutEffect` retired in the previous Tab-viability work; with the redirect gone, the state was write-only and the actions had no readers. `EditingStore` shrinks to `{ open, cancel, submit, areChildrenBeingEdited }` (plus the subscribe/getSnapshot pair). No user-visible change.

.changeset/tab-viability-skip-non-viable-targets.md

@@ -0,0 +1,11 @@
+---
+'json-edit-react': patch
+---
+
+Tab navigation now skips filtered-out and non-editable nodes up front instead of opening them and bouncing reactively. The redirect `useLayoutEffect` in `ValueNodeWrapper` that previously fired transient `startEdit` / `cancelEdit` observer events on dead-end nodes is gone — `onEditEvent` consumers no longer see those spurious pairs.
+
+The change is in `getNextOrPrevious`, which gains an optional 5th `isViable?: (nodeData) => boolean` predicate. `useCommon` composes the predicate from the precomputed `filterState.visiblePaths` Set and the existing `allowEditFilter`, and threads it through. Tab navigation from both value edits and key edits (via `KeyDisplay`) now benefits.
+
+Behaviour change worth noting: when no viable Tab target exists, the editor cancels cleanly. The previous "fall back to `previouslyEditedElement`" hop is gone.
+
+When live search hides the actively-edited node, the input now unmounts cleanly and the editing record sits inactive in the store; clearing the search later resumes the edit. No off-screen commit footgun because there's no input element to commit through.

src/JsonEditor.tsx

@@ -7,6 +7,7 @@ import React, {
   useState,
 } from 'react'
 import { assign, type AssignOptions, type AssignInput } from './utils/assign'
+import { buildNodeData } from './utils/buildNodeData'
 import { extract } from './utils/extract'
 import { CollectionNode } from './CollectionNode'
 import { NativeSelect } from './NativeSelect'
@@ -825,57 +826,6 @@ const updateDataObject = (
   }
 }
 
-// Canonical `NodeData` builder: construct the data for any node from the live
-// tree given its path. Used for the root node and by the `editorRef` handle to
-// run a node's `allowEdit` filter at call time (the filter takes the full
-// NodeData, not just a path). `index`/`size` mirror the child construction in
-// CollectionNode so a filter keying off them sees the same input the rendered
-// node would; `sort` (the `sortKeys` comparator) is only needed to resolve the
-// `index` of an object child, so it's optional.
-const buildNodeData = (
-  fullData: JsonData,
-  path: CollectionKey[],
-  rootName: string,
-  sort?: <T>(arr: T[], nodeMap: (input: T) => [string | number, unknown]) => void
-): NodeData => {
-  if (path.length === 0) {
-    return {
-      key: rootName,
-      path: [],
-      level: 0,
-      index: 0,
-      value: fullData,
-      size: isCollection(fullData) ? Object.keys(fullData).length : 1,
-      parentData: null,
-      fullData,
-    }
-  }
-
-  const key = path[path.length - 1]
-  const value = extract(fullData, path) as JsonData
-  const parentData = (extract(fullData, path.slice(0, -1)) ?? null) as object | null
-
-  let index = 0
-  if (Array.isArray(parentData)) {
-    index = typeof key === 'number' ? key : Number(key)
-  } else if (parentData && typeof parentData === 'object') {
-    const entries = Object.entries(parentData) as Array<[string | number, unknown]>
-    sort?.(entries, (entry) => entry)
-    index = entries.findIndex(([k]) => k === key)
-  }
-
-  return {
-    key,
-    path,
-    level: path.length,
-    index,
-    value,
-    size: isCollection(value) ? Object.keys(value as object).length : null,
-    parentData,
-    fullData,
-  }
-}
-
 const getFilterFunction = (
   propValue: boolean | number | FilterFunction
 ): FilterFunction => {

src/ValueNodeWrapper.tsx

@@ -1,4 +1,4 @@
-import React, { useEffect, useLayoutEffect, useState, useMemo, useCallback, useRef } from 'react'
+import React, { useEffect, useState, useMemo, useCallback, useRef } from 'react'
 import {
   StringValue,
   NumberValue,
@@ -60,14 +60,11 @@ const ValueNodeWrapperBase: React.FC<ValueNodeProps> = (props) => {
     getLatestData,
   } = props
   const { getStyles } = useTheme()
-  // Actions + a getSnapshot for imperative reads. The editing *state* this node
-  // needs (active, tabDirection, previouslyEditedElement) is only read
-  // inside event handlers / the Tab-redirect effect — never during render — so
-  // it's read from the snapshot at use-time rather than subscribed to. The only
-  // editing state that drives this node's render (`isEditing`) comes from
-  // `useCommon`'s per-node selector.
-  const { open, cancel, submit, recordPreviousEdit, setTabDirection, getSnapshot } =
-    useEditingStore()
+  // Actions + a getSnapshot for imperative reads. The editing `active` state
+  // this node reads (via `getSnapshot`) is only consulted inside event
+  // handlers, never during render. The only editing state that drives this
+  // node's render (`isEditing`) comes from `useCommon`'s per-node selector.
+  const { open, cancel, submit, getSnapshot } = useEditingStore()
   const { setCollapseState } = useCollapse()
   const [value, setValue] = useState<typeof data | CollectionData>(
     // Bad things happen when you put a function into useState
@@ -184,39 +181,6 @@ const ValueNodeWrapperBase: React.FC<ValueNodeProps> = (props) => {
   // Early return if this node is filtered out
   const isVisible = useNodeVisible(path)
 
-  // Skip hidden or uneditable nodes that Tab navigation has landed on by
-  // advancing the editing target to the next viable node.
-  //
-  // `useLayoutEffect` (not `useEffect`) is load-bearing: it fires synchronously
-  // before the browser paints, so the redirect state update batches into the
-  // same paint as the commit that flagged this node as `isEditing`. With plain
-  // `useEffect` the user would see a flicker — the current node briefly
-  // closes its editor (commit 1 with the filtered-out target "editing"),
-  // then reopens (commit 2 after the redirect). See V2-roadmap §16 for the
-  // followup: hoisting filter-awareness into `getNextOrPrevious` so the Tab
-  // handler can pick a viable target up front, eliminating the redirect
-  // entirely and dropping the setState-after-render pattern.
-  useLayoutEffect(() => {
-    if (!isEditing) return
-    if (isVisible && canEdit) return
-    // A forced (imperative `editorRef.startEdit`) edit overrides `allowEdit`,
-    // so don't bounce off this node just because it's normally uneditable. A
-    // search-filtered-out node (`!isVisible`) still redirects — it can't
-    // render.
-    if (isVisible && getSnapshot().active?.force) return
-    const { tabDirection, previouslyEditedElement } = getSnapshot()
-    const next = getNextOrPreviousAtPath(tabDirection)
-    if (next) open(next)
-    else if (previouslyEditedElement) open(previouslyEditedElement)
-    else cancel()
-    // The three booleans gate the redirect; `open`/`cancel` are included for
-    // hygiene (store-stable, so they almost never flip). The remaining reads
-    // (`tabDirection`, `previouslyEditedElement`, `path`, `sort`) are
-    // intentionally excluded — they change every render / edit transition and
-    // would cause spurious re-fires when no redirect is needed.
-    // eslint-disable-next-line react-hooks/exhaustive-deps
-  }, [isEditing, isVisible, canEdit, open, cancel])
-
   if (!isVisible) return null
 
   // A local (deferred) type-switch is in progress when the editing `dataType`
@@ -436,14 +400,13 @@ const ValueNodeWrapperBase: React.FC<ValueNodeProps> = (props) => {
   const setIsEditing = canEdit ? startEdit : NOOP
 
   // Commit this field's edit, then open the next/previous node in the given Tab
-  // direction. Not pure — it orchestrates store actions (`setTabDirection`,
-  // `recordPreviousEdit`, `open`) with this node's `handleEdit`/`path`, so it
-  // stays local rather than moving to keyboard utils (cf. the pure
-  // `getNextOrPrevious`). `handleEdit`'s `onCommit` defers `open` to the commit
-  // moment, so Tab advances only once this field's edit has landed.
+  // direction. Not pure — it orchestrates store actions (`open`) with this
+  // node's `handleEdit`/`path`, so it stays local rather than moving to
+  // keyboard utils (cf. the pure `getNextOrPrevious`). `handleEdit`'s
+  // `onCommit` defers `open` to the commit moment, so Tab advances only once
+  // this field's edit has landed. `getNextOrPreviousAtPath` already skips
+  // non-viable targets (the viability predicate lives in `useCommon`).
   const tabTo = (dir: TabDirection) => () => {
-    setTabDirection(dir)
-    recordPreviousEdit(path)
     const target = getNextOrPreviousAtPath(dir)
     if (target) handleEdit(undefined, () => open(target))
   }

src/contexts/EditingProvider.tsx

@@ -34,7 +34,6 @@
 
 import React, { createContext, useContext, useMemo, useRef, useSyncExternalStore } from 'react'
 import {
-  type TabDirection,
   type CollectionKey,
   type OnEditEventFunction,
   type EditEvent,
@@ -65,8 +64,6 @@ export interface EditingStateBundle {
   active: EditingState | null
   /** In-flight optimistic commits: path-string → the commit's token. */
   settling: Record<PathString, Token>
-  previouslyEditedElement: CollectionKey[] | null
-  tabDirection: TabDirection
 }
 
 /** A commit to perform, discriminated by `op`. `path` is the target/source. */
@@ -187,17 +184,13 @@ export interface EditingStore {
    *  `onUpdate`) so the calling node can report errors via its own `onError`.
    */
   submit: (args: SubmitArgs) => Promise<UpdateOutcome | undefined>
-  setTabDirection: (dir: TabDirection) => void
-  recordPreviousEdit: (path: CollectionKey[]) => void
   /** Imperative read for event handlers — does not subscribe. */
   areChildrenBeingEdited: (path: CollectionKey[]) => boolean
 }
 
 const initialState: EditingStateBundle = {
   active: null,
   settling: {},
-  previouslyEditedElement: null,
-  tabDirection: 'next',
 }
 
 const createEditingStore = (
@@ -485,17 +478,6 @@ const createEditingStore = (
     return outcome
   }
 
-  const setTabDirection = (dir: TabDirection) => {
-    if (state.tabDirection !== dir) commit({ ...state, tabDirection: dir })
-  }
-
-  const recordPreviousEdit = (path: CollectionKey[]) => {
-    const prev = state.previouslyEditedElement
-    if (prev === null || !pathsEqual(prev, path)) {
-      commit({ ...state, previouslyEditedElement: path })
-    }
-  }
-
   return {
     subscribe: (onChange) => {
       listeners.add(onChange)
@@ -506,8 +488,6 @@ const createEditingStore = (
     open,
     cancel,
     submit,
-    setTabDirection,
-    recordPreviousEdit,
     areChildrenBeingEdited: (path) =>
       state.active !== null && isDescendantOf(state.active.path, path),
   }
@@ -591,8 +571,6 @@ export const useEditing = () => {
       open: store.open,
       cancel: store.cancel,
       submit: store.submit,
-      setTabDirection: store.setTabDirection,
-      recordPreviousEdit: store.recordPreviousEdit,
       areChildrenBeingEdited: store.areChildrenBeingEdited,
     }),
     [bundle, store]

src/contexts/FilterStateProvider.tsx

@@ -4,7 +4,7 @@
  * visibility/count is an O(1) Set/Map lookup instead of the per-node
  * subtree walk the old `filterNode` did.
  *
- * Three consumer hooks:
+ * Four consumer hooks:
  *
  *   useFilterActive()           — boolean: "is search active right now?"
  *                                 Drives the n-of-m branch (no point
@@ -20,6 +20,14 @@
  *                                 filter is active (caller falls back to
  *                                 `nodeData.size`).
  *
+ *   useRawFilterState()         — FilterState | null: the whole pre-
+ *                                 computed bundle. For callers that need
+ *                                 to check many candidate paths from a
+ *                                 closure (e.g. the Tab-viability
+ *                                 predicate in ValueNodeWrapper, where
+ *                                 hooks-per-candidate would violate the
+ *                                 rules of hooks).
+ *
  * Pattern note: this is a dumb provider — `JsonEditor` owns the
  * `useMemo` that builds the state, and passes it as a `value` prop.
  * Keeps the algorithm out of React-land for testability (see
@@ -56,6 +64,8 @@ const useFilterStateContext = (): FilterState | null => {
 
 export const useFilterActive = (): boolean => useFilterStateContext() !== null
 
+export const useRawFilterState = (): FilterState | null => useFilterStateContext()
+
 export const useNodeVisible = (path: CollectionKey[]): boolean => {
   const fs = useFilterStateContext()
   if (fs === null) return true

src/contexts/index.ts

@@ -8,4 +8,5 @@ export {
   useFilterActive,
   useNodeVisible,
   useVisibleChildCount,
+  useRawFilterState,
 } from './FilterStateProvider'

src/hooks/useCommon.ts

@@ -4,11 +4,17 @@
  */
 
 import React, { useCallback, useRef, useState } from 'react'
-import { useEditingSelector, useEditingStore, useVisibleChildCount } from '../contexts'
+import {
+  useEditingSelector,
+  useEditingStore,
+  useRawFilterState,
+  useVisibleChildCount,
+} from '../contexts'
 import {
   type CollectionNodeProps,
   type ErrorString,
   type JerError,
+  type NodeData,
   type ThemeableElement,
   type ValueData,
   type ValueNodeProps,
@@ -113,11 +119,35 @@ export const useCommon = ({ props, collapsed }: CommonProps) => {
     [onErrorCallback, showErrorMessages, getLatestData, errorDisplayTime]
   )
 
+  // Tab-viability predicate: a candidate leaf is a valid Tab target only
+  // if it would be visible AND editable. Visibility comes from the
+  // precomputed `filterState.visiblePaths` Set; editability is the
+  // existing `allowEditFilter(nodeData)`. Closures over the live filter
+  // state, so a search keystroke that re-builds the state implicitly
+  // re-builds the predicate (via useCallback identity), but only when
+  // those inputs actually change — the §16 memo invariants hold.
+  //
+  // Hooks-rules note: we read the WHOLE filter state via
+  // `useRawFilterState` (not per-path `useNodeVisible`) because the
+  // predicate is invoked for many candidate paths during a single Tab,
+  // and we can't call hooks inside a loop.
+  const filterState = useRawFilterState()
+  const isViableTarget = useCallback(
+    (candidate: NodeData) => {
+      const visible =
+        filterState === null || filterState.visiblePaths.has(toPathString(candidate.path))
+      return visible && allowEditFilter(candidate)
+    },
+    [filterState, allowEditFilter]
+  )
+
   // Stable wrapper around `getNextOrPrevious` against the LIVE document for
   // this node's `path`, so callers (`KeyDisplay`, value-node `tabForward` /
   // `tabBack`) don't need to re-thread `getLatestData` / `path` / `sort`.
+  // Threads `isViableTarget` so Tab skips filtered-out / non-editable
+  // leaves up front instead of landing on them and bouncing.
   const getNextOrPreviousAtPath = (type: 'next' | 'prev') =>
-    getNextOrPrevious(getLatestData(), path, type, sort)
+    getNextOrPrevious(getLatestData(), path, type, sort, isViableTarget)
 
   // Commits a key rename through the store's commit engine. The no-op /
   // duplicate-key checks stay client-side and close the session as a cancel.

src/utils/buildNodeData.ts

@@ -0,0 +1,62 @@
+import { type CollectionKey, type JsonData, type NodeData } from '../types'
+import { extract } from './extract'
+import { isCollection } from './misc'
+
+/**
+ * Builds a `NodeData` snapshot for an arbitrary path against `fullData`.
+ *
+ * Reused by:
+ *   - `JsonEditor` (root and bridge construction — onCollapse / onEditEvent /
+ *     editorRef handle reads).
+ *   - `getNextOrPrevious` (synthesising the candidate `NodeData` the
+ *     viability predicate is called with during Tab navigation).
+ *
+ * `index` derivation respects the same sort the renderer uses, so a custom
+ * `searchFilter` / `allowEdit` callback sees the same `index` it would during
+ * render. `rootName` is the editor's `rootName` prop value — only used when
+ * `path` is empty; defaults to `''` for callers that never target the root
+ * (Tab leaves, internal walk steps).
+ */
+export const buildNodeData = (
+  fullData: JsonData,
+  path: CollectionKey[],
+  rootName = '',
+  sort?: <T>(arr: T[], nodeMap: (input: T) => [string | number, unknown]) => void
+): NodeData => {
+  if (path.length === 0) {
+    return {
+      key: rootName,
+      path: [],
+      level: 0,
+      index: 0,
+      value: fullData,
+      size: isCollection(fullData) ? Object.keys(fullData).length : 1,
+      parentData: null,
+      fullData,
+    }
+  }
+
+  const key = path[path.length - 1]
+  const value = extract(fullData, path) as JsonData
+  const parentData = (extract(fullData, path.slice(0, -1)) ?? null) as object | null
+
+  let index = 0
+  if (Array.isArray(parentData)) {
+    index = typeof key === 'number' ? key : Number(key)
+  } else if (parentData && typeof parentData === 'object') {
+    const entries = Object.entries(parentData) as Array<[string | number, unknown]>
+    sort?.(entries, (entry) => entry)
+    index = entries.findIndex(([k]) => k === key)
+  }
+
+  return {
+    key,
+    path,
+    level: path.length,
+    index,
+    value,
+    size: isCollection(value) ? Object.keys(value as object).length : null,
+    parentData,
+    fullData,
+  }
+}