Skip to content

V2 §2: Path identity — drop dot-joined strings - #260

Merged
CarlosNZ merged 4 commits into
v2.0-devfrom
246-fix-string-based-paths-problem
May 29, 2026
Merged

V2 §2: Path identity — drop dot-joined strings#260
CarlosNZ merged 4 commits into
v2.0-devfrom
246-fix-string-based-paths-problem

Conversation

@CarlosNZ

@CarlosNZ CarlosNZ commented May 29, 2026

Copy link
Copy Markdown
Owner

Closes #246. Part of the V2.0 roadmap, §2.

Summary

  • Internal node identity is now CollectionKey[] everywhere instead of a dot-joined string.
  • areChildrenBeingEdited, the drag-onto-self guard, and the editing-state comparisons all use array predicates (pathsEqual, isDescendantOf) — no more string .includes() / .startsWith() substring checks.
  • toPathString survives as a public utility but its encoding changes to / + encodeURIComponent (injective by construction). The key?: 'key_' second arg is removed — mode is now a field on the editing-state object, not a string prefix.

Bugs fixed

This kills three related bugs that were all rooted in the same string-encoding shortcut. Paste this into the demo to see them on v2.0-dev (and watch them vanish here):

```json
{
"foo": { "name": "A" },
"foobar": { "name": "B" },
"with.dot": "I collide with 'with > dot'",
"with": { "dot": "I collide with 'with.dot'" }
}
```

  1. Dotted-key collision. Editing the root `"with.dot"` value used to put both it and the nested `with > dot` value into edit mode simultaneously — they serialised to the same path string.
  2. Substring match in `areChildrenBeingEdited`. Editing `foobar > name` would force the unrelated `foo` collection to open (because `"foobar.name".includes("foo")`).
  3. Substring match in drag-onto-self guard. Dragging `foo` over `foobar` would hide the drop indicator (`"foobar".startsWith("foo")`).

Also fixes a related latent bug in `handleDrop` (useDragNDrop.tsx:122–123) where `sourceBase` was joined with `.` but `thisBase` was joined with `''` — so `KEY_EXISTS` fired spuriously on legitimate same-parent drags at depth ≥ 3.

Implementation shape

  • Editing state is now `{ path: CollectionKey[]; mode: 'value' | 'key' } | null`, bundled so transitions are atomic (the §4 reducer work in V2 §4: TreeStateProvider refactor #247 can grow this into a discriminated union naturally).
  • `previouslyEditedElement` stays path-only (tab nav only ever targets value-mode).
  • `dragSource` drops the mirrored `pathString` field.
  • New helpers in `src/helpers.ts`: `pathsEqual`, `isDescendantOf` (reflexive — a path is a trivial descendant of itself; both callers want that), `editingStatesEqual`.

Bundle size impact

Essentially a wash — adding new symbol names slightly reduces gzip efficiency:

Raw Gzipped
ESM 53,421 → 53,418 (−3 bytes) 18,054 → 18,072 (+18 bytes, +0.10%)
CJS 54,589 → 54,587 (−2 bytes) 18,104 → 18,115 (+11 bytes, +0.06%)

Test plan

  • New unit tests in `test/pathHelpers.test.ts` cover `pathsEqual`, `isDescendantOf` (including reflexive case + substring-lookalike regression), and `toPathString` (encoding injectivity, slashes, percents, empties, unicode).
  • `pnpm lint && pnpm compile && pnpm test` — all green (126 tests).
  • `pnpm -r build` — core + themes + components all build cleanly.
  • Demo verification: paste the JSON above, confirm all three bugs are gone.
  • Tab navigation through the tree (exercises `previouslyEditedElement`).
  • External `edit` trigger fires correctly on the matching path.

🤖 Generated with Claude Code

CarlosNZ and others added 2 commits May 29, 2026 15:09
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@github-actions

github-actions Bot commented May 29, 2026

Copy link
Copy Markdown

Bundle size impact

json-edit-react

Format Base raw PR raw Δ raw Base gzip PR gzip Δ gzip
esm 52.17 KB 52.20 KB 🔺 +29 B (+0.05%) 17.69 KB 17.72 KB 🔺 +34 B (+0.19%)
cjs 53.31 KB 53.34 KB 🔺 +30 B (+0.05%) 17.74 KB 17.77 KB 🔺 +31 B (+0.17%)

Measured from build/index.{cjs,esm}.js. Gzip at level 9.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Replaces dot-joined string path identity with CollectionKey[] arrays throughout the editing/drag/match flow, eliminating ambiguity and substring-matching bugs in areChildrenBeingEdited and drag guards. Editing state becomes a structured { path, mode } object instead of a key_-prefixed string. The exported toPathString survives but switches to a /-joined encodeURIComponent encoding (breaking change, documented in migration guide).

Changes:

  • Introduce array path helpers (pathsEqual, isDescendantOf, editingStatesEqual) and the EditingState type; refactor TreeStateProvider, useCommon, useDragNDrop, useTriggers, CollectionNode, and ValueNodeWrapper to use them.
  • Re-encode toPathString to /+encodeURIComponent, drop the 'key_' second arg, and update README/migration guide/changeset accordingly.
  • Add test/pathHelpers.test.ts covering the new helpers and encoding semantics.

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src/types.ts Adds EditingState interface.
src/helpers.ts New array path helpers; rewrites toPathString encoding.
src/contexts/TreeStateProvider.tsx Editing state stored as EditingState, drag source loses pathString, areChildrenBeingEdited uses isDescendantOf.
src/CollectionNode.tsx Stops reading currentlyEditingElement; uses areChildrenBeingEdited(path).
src/hooks/useCommon.ts Computes isEditing/isEditingKey via pathsEqual + mode.
src/hooks/useDragNDrop.tsx Drag source becomes {path}; reflexive descendant guard; parent comparison via pathsEqual; fixes the join('.')/join('') mismatch in handleDrop.
src/hooks/useTriggers.ts External edit-trigger path matching via pathsEqual.
src/ValueNodeWrapper.tsx Stores path array (not string) into previouslyEditedElement.
test/pathHelpers.test.ts New unit coverage for path helpers and encoding.
README.md / migration-guide.md / .changeset Document the encoding change and removal of the 'key_' arg.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/helpers.ts Outdated
Comment on lines +130 to +137
* Converts a path expressed as an array of CollectionKeys to a string suitable
* for HTML `name`/`id` attributes. The encoding is injective: keys are
* URL-encoded (so any literal `/` becomes `%2F`) and joined with `/`. Because
* `encodeURIComponent` never emits `/`, the separator can only appear between
* keys, never inside one — so distinct paths always produce distinct strings.
*/
export const toPathString = (path: Array<string | number>, key?: 'key_') =>
(key ?? '') +
path
// An empty string in a part will "disappear", so replace it with a
// non-printable char
.map((part) => (part === '' ? String.fromCharCode(0) : part))
.join('.')
export const toPathString = (path: CollectionKey[]) =>
path.map((part) => encodeURIComponent(String(part))).join('/')

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch — the injectivity claim was overstated. Fixed in f25c49b:

  • [''] now maps to '\0' via a single-case short-circuit before the join-based encoding runs. Safe because encodeURIComponent never emits a literal null char (it produces '%00' for the null byte).
  • All other empty-key positions already disambiguate via separator structure, so they're unchanged: ['a', '']'a/', ['', 'a']'/a', ['', '']'/', ['', '', '']'//', ['a', '', 'b']'a//b'.
  • Added explicit tests for each empty-key position plus a covering injectivity check over the edge-case paths (including ['\0'] to verify the sentinel doesn't collide with a literal null-byte key).

Docstring updated to document the sentinel rather than overstate the guarantee.

`toPathString([''])` previously collided with `toPathString([])` — both
produced `''`. Sentinel `'\0'` for the single-empty case fixes the only
remaining collision; all other empty-key positions already disambiguate
via separator structure. Caught by Copilot review of #260.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@CarlosNZ
CarlosNZ merged commit 941a1cd into v2.0-dev May 29, 2026
2 checks passed
@CarlosNZ
CarlosNZ deleted the 246-fix-string-based-paths-problem branch May 29, 2026 04:10
@CarlosNZ CarlosNZ linked an issue May 29, 2026 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

V2 §2: Path identity — drop dot-joined strings

2 participants