feat(joint-react): generic cellVisibility ownership latch#3383
Open
kumilingus wants to merge 9 commits into
Open
feat(joint-react): generic cellVisibility ownership latch#3383kumilingus wants to merge 9 commits into
kumilingus wants to merge 9 commits into
Conversation
Let a paper feature take ownership of `paper.options.cellVisibility` so the Paper component stops writing it directly. This lets a virtual-rendering PaperScroller (in @joint/react-plus) compose the Paper's `cellVisibility` with its viewport culling instead of throwing "Paper cellVisibility is already set". The latch is generic — core has no knowledge of PaperScroller or virtual rendering; any feature can claim it via the existing feature system. - PaperStore: `nativeCellVisibility`, `claim`/`releaseCellVisibility`, `isCellVisibilityOwned`, and a change listener. - use-create-portal-paper: expose the resolved callback; skip writing `cellVisibility` when owned and push refreshed values to the owner. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Author
|
Consumer PR: clientIO/joint-plus#715 |
- Ignore `cellVisibility` supplied via the `<Paper options>` escape hatch so it can't clobber an owning feature's callback (it is managed via the dedicated prop / feature ownership only). - Route owner notifications through the registered feature's `onCellVisibilityChange` hook instead of a separate listener registry (drops the listener field + register/notify methods on PaperStore). - Self-heal the latch: `removePaperFeature` releases cellVisibility ownership held by the removed feature. - Only re-notify the owner when the resolved callback actually changes (dedicated effect keyed on the callback), avoiding redundant re-wraps / viewport recomputes on unrelated prop changes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Drop the sanitized-escape-hatch useMemo. The only place a stray `options.cellVisibility` could clobber an owning feature's wrapper is the update effect, so just omit `cellVisibility` from the assigned options when owned (covers both the prop and the escape hatch). Creation is already handled by the feature's claim clearing the option. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… hatch `cellVisibility` has a dedicated prop and is managed by feature ownership, so it must not be settable via the `options` escape hatch. Omit it from `PaperOptions` (TypeScript-enforced) instead of stripping it at runtime; the update effect then just omits the dedicated callback when a feature owns it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ape hatch The type-level Omit only guards TS callers; add a runtime guard so plain-JS callers can't smuggle `cellVisibility` through `options` and override the dedicated prop / clobber an owning feature's wrapper. Fail loud with a clear message pointing to the dedicated prop. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…uard Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… guard Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR introduces a generic “ownership latch” in @joint/react for paper.options.cellVisibility, allowing a feature (e.g. virtual rendering) to temporarily own the option without React <Paper> repeatedly overwriting it on re-renders.
Changes:
- Add
PaperStoreAPIs to claim/releasecellVisibilityownership and to notify the owning feature when the native callback changes. - Update paper creation/update flow to track
nativeCellVisibility, omit writingcellVisibilitywhile owned, and add a runtime guard preventingcellVisibilityvia theoptionsescape hatch. - Add/extend Jest tests covering the ownership latch behavior and feature removal self-healing.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/joint-react/src/types/feature.types.ts | Adds optional onCellVisibilityChange hook for feature owners. |
| packages/joint-react/src/store/paper-store.ts | Implements cellVisibility ownership latch + owner notification API. |
| packages/joint-react/src/store/graph-store.ts | Ensures removing a paper feature releases cellVisibility ownership if held. |
| packages/joint-react/src/store/tests/paper-store.test.ts | Adds unit tests for claim/release/notify behavior. |
| packages/joint-react/src/store/tests/graph-store-features.test.ts | Adds test ensuring ownership is released on feature removal. |
| packages/joint-react/src/hooks/use-create-portal-paper.tsx | Tracks nativeCellVisibility, omits writes while owned, and guards escape-hatch usage. |
| packages/joint-react/src/components/paper/paper.types.ts | Excludes cellVisibility from PaperOptions escape hatch type. |
| packages/joint-react/src/components/paper/tests/paper.test.tsx | Adds test for the runtime escape-hatch guard. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…pe comment Address PR review: - Initialize `PaperStore.nativeCellVisibility` from the paper's initial `cellVisibility` option in the constructor, so `releaseCellVisibility` restores a correct value even before a prop-update effect runs or when the store is used without the hook (also covers an adopted external paper). - Collapse the duplicated escape-hatch guard comment in use-create-portal-paper. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Guard on the actual value, not key presence, so an explicit
`options: { cellVisibility: undefined }` (a harmless no-op) doesn't throw.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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
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.
Why
In
@joint/react-plus, using<Paper cellVisibility>together with<PaperScroller virtualRendering>throws:The
VirtualRenderingControllerneeds to ownpaper.options.cellVisibility(it composes the user callback with a viewport check), but the React<Paper>writes that option on creation and re-asserts it on every relevant re-render (a passive effect that always runs after the scroller's layout effect). So the scroller's wrapper gets clobbered.This can't be fixed from the plus layer alone — core must stop writing
cellVisibilitywhen a feature owns it.What
A generic ownership latch on
PaperStore. Core has no knowledge of PaperScroller or virtual rendering — any feature can claim the option through the existing feature system.PaperStore:nativeCellVisibility— the resolved native callback (kept current regardless of ownership).claimCellVisibility(ownerId)/releaseCellVisibility(ownerId)/isCellVisibilityOwned.registerCellVisibilityListener/notifyCellVisibilityChange— push refreshed callbacks to the owner without depending on the owning component re-rendering.use-create-portal-paper: exposenativeCellVisibility; when owned, skip writingcellVisibilityonto the paper and notify the owner instead.Tests
paper-store.test.ts— 7 new cases (claim clears, release restores, non-owner no-op, listener fired/cleared). Full suite: 946 passing.Coordinated change
Consumed by a matching PR in
clientIO/joint-plus(PaperScroller + VirtualRenderingController). The plus side feature-detects this latch and degrades gracefully against an older core.🤖 Generated with Claude Code