fix(plugin-list-index): number list items nested in containers#2804
Open
christianhg wants to merge 2 commits into
Open
fix(plugin-list-index): number list items nested in containers#2804christianhg wants to merge 2 commits into
christianhg wants to merge 2 commits into
Conversation
🦋 Changeset detectedLatest commit: 805f62c The changes in this PR will be included in the next version bump. This PR includes changesets to release 14 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📦 Bundle Stats —
|
| Metric | Value | vs main (f587123) |
|---|---|---|
| Internal (raw) | 786.3 KB | - |
| Internal (gzip) | 150.0 KB | - |
| Bundled (raw) | 1.39 MB | - |
| Bundled (gzip) | 311.9 KB | - |
| Import time | 99ms | +2ms, +2.0% |
@portabletext/editor/behaviors
| Metric | Value | vs main (f587123) |
|---|---|---|
| Internal (raw) | 467 B | - |
| Internal (gzip) | 207 B | - |
| Bundled (raw) | 424 B | - |
| Bundled (gzip) | 171 B | - |
| Import time | 2ms | +0ms, +5.1% |
@portabletext/editor/plugins
| Metric | Value | vs main (f587123) |
|---|---|---|
| Internal (raw) | 2.7 KB | - |
| Internal (gzip) | 894 B | - |
| Bundled (raw) | 2.5 KB | - |
| Bundled (gzip) | 827 B | - |
| Import time | 7ms | +0ms, +3.8% |
@portabletext/editor/selectors
| Metric | Value | vs main (f587123) |
|---|---|---|
| Internal (raw) | 78.8 KB | - |
| Internal (gzip) | 14.4 KB | - |
| Bundled (raw) | 74.3 KB | - |
| Bundled (gzip) | 13.3 KB | - |
| Import time | 8ms | -0ms, -0.3% |
@portabletext/editor/traversal
| Metric | Value | vs main (f587123) |
|---|---|---|
| Internal (raw) | 24.9 KB | +36 B, +0.1% |
| Internal (gzip) | 5.0 KB | - |
| Bundled (raw) | 24.9 KB | +17 B, +0.1% |
| Bundled (gzip) | 4.9 KB | +4 B, +0.1% |
| Import time | 6ms | +0ms, +2.2% |
@portabletext/editor/utils
| Metric | Value | vs main (f587123) |
|---|---|---|
| Internal (raw) | 29.1 KB | - |
| Internal (gzip) | 6.1 KB | - |
| Bundled (raw) | 26.7 KB | - |
| Bundled (gzip) | 5.7 KB | - |
| Import time | 6ms | +0ms, +0.7% |
🗺️ . · ./behaviors · ./plugins · ./selectors · ./traversal · ./utils · Artifacts
Details
- Import time regressions over 10% are flagged with
⚠️ - Sizes shown as raw / gzip 🗜️. Internal bytes = own code only. Total bytes = with all dependencies. Import time = Node.js cold-start median.
📦 Bundle Stats — @portabletext/markdown
Compared against main (f5871232)
| Metric | Value | vs main (f587123) |
|---|---|---|
| Internal (raw) | 53.0 KB | - |
| Internal (gzip) | 9.6 KB | - |
| Bundled (raw) | 347.6 KB | - |
| Bundled (gzip) | 96.0 KB | - |
| Import time | 39ms | -1ms, -1.3% |
🗺️ View treemap · Artifacts
Details
- Import time regressions over 10% are flagged with
⚠️ - Sizes shown as raw / gzip 🗜️. Internal bytes = own code only. Total bytes = with all dependencies. Import time = Node.js cold-start median.
christianhg
force-pushed
the
fix/plugin-list-index-containers
branch
from
f70ed7d to
68e3220
Compare
christianhg
force-pushed
the
fix/plugin-list-index-containers
branch
from
68e3220 to
5aefefc
Compare
christianhg
force-pushed
the
fix/plugin-list-index-containers
branch
from
5aefefc to
2a999e5
Compare
christianhg
force-pushed
the
fix/plugin-list-index-containers
branch
from
2a999e5 to
6372303
Compare
christianhg
force-pushed
the
fix/plugin-list-index-containers
branch
from
6372303 to
6984a39
Compare
christianhg
force-pushed
the
fix/plugin-list-index-containers
branch
from
6984a39 to
e624fe6
Compare
`plugin-list-index`, and any consumer that walks editable containers,
needs to descend a node's children without re-deriving the path and
re-walking from the root at each level, which is O(depth^2) for
recursive descent. The public `getChildren` is path-based and re-walks;
the node-based resolver that threads the resolved parent was internal.
Exports `getNodeChildren(context, node, parent?)` from
`@portabletext/editor/traversal` (`@beta`). It resolves a node's
editable child array in one step, returning `{children, fieldName,
parent}`, so a caller already holding the node descends in O(1) per
level. The function already existed and is used internally; this only
widens the public surface, no behavior change.
`buildListIndexMap` mirrored the engine's flat walk and treated a
container block as a sequence-breaking non-text block, so list items in
a cell were never numbered and `useListIndex` returned `undefined`.
Recurse into each container's child array via the now-public
`getNodeChildren`, scope list state per array (so each cell numbers
independently and restarts at 1), and key by the full block path; the
top-level path collapses to `[{_key}]`, so flat numbering is unchanged.
Descent is node-based, O(depth). The engine's own `listIndexMap` is left
top-level-only on purpose: container content renders through this plugin
in the pt-native pipeline (a `defineTextBlock` reading `useListIndex`),
not the engine's default text-block render, so the engine map is never
read for a nested block.
The rebuild was gated on `path.length > 2`, skipping deep edits inside a
container so a cell's numbering never updated. Drop the path gate:
rebuild on every operation except text edits (the only
`editor.on('operation')` events that cannot change list structure).
`containers` is optional on the entry point; production passes the full
snapshot context.
christianhg
force-pushed
the
fix/plugin-list-index-containers
branch
from
e624fe6 to
805f62c
Compare
christianhg
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
List items inside an editable container (e.g. a table cell) render unnumbered:
useListIndexreturnsundefinedfor them.plugin-list-index'sbuildListIndexMapmirrored the engine's flat walk and treated a container block as a sequence-breaking non-text block, so it never descended into a cell's child array.The fix lives entirely in
plugin-list-index, because that is the path Studio's container rendering actually uses: the pt-native pipeline registers adefineTextBlockthat readsuseListIndex, so cell list numbering comes from this plugin, not the engine.buildListIndexMapnow recurses into each container's child array, scopes list state per array (each cell numbers independently and restarts at 1), and keys entries by the full block path; the top-level path collapses to[{_key}], so flat numbering is byte-for-byte unchanged.The descent is node-based via a newly-exported primitive,
getNodeChildren(@portabletext/editor/traversal,@beta). It's the node-based companion to the path-basedgetChildren: given a node already in hand it resolves the editable child array in one step, so recursive descent stays O(depth) rather than re-walking from the root each level. The function already existed internally; this only widens the public surface.The engine's own
listIndexMapis deliberately left top-level-only. Its sole reader is the engine's default text-block render (data-list-index), which feeds the legacy pipeline, and the legacy pipeline does not render containers inline. Making it container-aware would add engine surface that nothing currently observes; that can come later (or under the container-resolution cleanup, EDEX-1321). The broader container-resolution util sprawl that this export brushes against is tracked there too.A second bug in the plugin: the rebuild was gated on
path.length > 2, which skipped deep edits inside a container, so a structural edit in a cell left that cell's numbering stale until a shallow op fired. The gate is dropped; the plugin rebuilds on every operation exceptinsert.text/remove.text(the onlyeditor.on('operation')events that cannot change list structure).Tests
data-list-index.test.tsx: end-to-end through a real editor with a registeredcalloutcontainer, assertingdata-list-indexon the rendered nested list items, plus a deepinsert.breakinside a cell that renumbers the sibling (pins the gate removal).buildListIndexMapunit suite passes unchanged (top-level numbering is unaffected).