Fixes 31156: release TipTap editors on unmount and load the ELK layout engine on demand - #31157
Open
siddhant1 wants to merge 2 commits into
Open
Fixes 31156: release TipTap editors on unmount and load the ELK layout engine on demand#31157siddhant1 wants to merge 2 commits into
siddhant1 wants to merge 2 commits into
Conversation
…ulating `useCustomEditor` creates a ProseMirror `Editor` in an effect but its cleanup only flipped an `isMounted` flag — `instance.destroy()` was never called (grep finds it only in test files). The editor's view, plugin state, contenteditable DOM and MutationObserver therefore outlive the component. This compounds because the editor is mounted per table row: the description column renders `RichTextEditorPreviewerNew` for every row, so a page of descriptions is a page of editors. Measured live on release-2-0: 16 live `.ProseMirror` instances for 16 rows on `/tags/General`, 7 for 7 on a glossary terms table. Every pagination click, search keystroke or tab switch abandoned that many editors for the rest of the session. The `isDestroyed` guard mirrors `@tiptap/react`'s own `useEditor` cleanup (node_modules/@tiptap/react/dist/index.js:1013 and :1039) — StrictMode double-invokes effects, so the cleanup can run twice. Co-Authored-By: Claude <noreply@anthropic.com>
Contributor
❌ PR checklist incompleteThis PR cannot be merged until the following are addressed on its linked issue:
The fields live on the linked issue in the Shipping project (open the issue → right sidebar → Projects). After you set them, re-run this check (or push a commit) — issue/project changes do not re-trigger it automatically. Maintainers can bypass this check by adding the |
Contributor
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
Contributor
🔴 Playwright Results — workflow failedValidated commit ✅ 0 passed · ❌ 0 failed · 🟡 0 flaky · ⏭️ 0 skipped · 🧰 0 lifecycle flaky Pipeline and setup failures (6)
Performance⚪ Performance metrics unavailable; see the CI and reporting failures above.
How to debug locally# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip # view trace |
siddhant1
changed the title
Contributor
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
siddhant1
added
safe to test
…demand `EntityLineageLayoutUtils` mixed two concerns: five pure geometry helpers (`centerNodePosition`, `getNodesBoundsReactFlow`, `getViewportForBoundsReactFlow`, ...) and two ELK-backed layout functions. Because `ELKUtil` was imported at module scope, importing any of the pure helpers dragged elkjs — 1.37MB built. `LineageControlButtons` imports only `centerNodePosition`, and `LineageProvider` is statically imported by eight lineage components. That was enough to land elkjs in the shared chunk which 476 route chunks statically import, so every authenticated page downloaded and parsed the lineage layout engine. Glossary pages paid for it while rendering with G6/antv-dagre, never calling ELK at all. `getELKLayoutedElements` and `positionNodesUsingElk` now live in `utils/Lineage/Layout/ElkLayoutUtils.ts`, alongside a named `loadElkLayout()` lazy boundary. `EntityLineageLayoutUtils` no longer references ELK in any form, so the pure helpers cannot re-acquire the dependency by accident. A plain file split would not have been enough on its own: `LineageProvider` is the sole caller of the ELK functions and is itself reachable from the shared cluster, so the engine needs the dynamic boundary regardless. Making it a named function in an ELK-specific module keeps that explicit rather than burying an `await import()` in the middle of layout logic. Measured on a production build: ELKUtil chunk importers 1 static -> 0 static, 1 dynamic glossary terms tab 12.57 MB -> 11.20 MB (-1.37 MB, runtime) Verified against a live backend: the glossary terms tab makes zero elk-related requests and still renders, and /lineage fetches ELKUtil + vendor-elk on demand and draws 83 nodes with no console errors. Co-Authored-By: Claude <noreply@anthropic.com>
siddhant1
force-pushed
the
long-animation-frame-block
branch
from
August 7, 2026 10:28
1def900 to
9a9143e
Compare
Code Review ✅ ApprovedReleases TipTap editors on component unmount and lazy-loads the ELK layout engine to reduce bundle size and prevent memory leaks. No issues found. OptionsDisplay: compact → Showing less information. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Powered by Gitar — free for open source |
Contributor
❌ UI Checkstyle Failed❌ ESLint + Prettier + Organise Imports (src)One or more source files have linting or formatting issues. Affected files
❌ Core Components - I18n SyncCore-components 🔍 ESLint findings in this PR's files — 0 error(s), 48 warning(s)Errors block the build. Warnings do not yet — they are rules whose backlog is still 0 error(s), 48 warning(s) across 2 changed file(s).
All findings
Fix locally (fast - only checks files changed in this branch): make ui-checkstyle-changed |
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.
Fixes #31156
Two independent main-thread fixes found while investigating Sentry
ui.long-animation-framespans ("Main UI thread blocked") — 826 spans over two weeks, p99 1461ms, max 4304ms, 153.4 s of total blocking.1. TipTap editors were never destroyed
useCustomEditorcreates a ProseMirrorEditorin an effect, but cleanup only flipped a localisMountedflag —instance.destroy()was never called (a grep acrosssrc/matches only test files). The view, plugin stack, contenteditable DOM andMutationObserveroutlived the component.It compounds because the editor mounts once per table row — the shared description column (
utils/TableColumn.util.tsx) rendersRichTextEditorPreviewerNew, which mounts aBlockEditor.The
isDestroyedguard mirrors@tiptap/react's ownuseEditorcleanup (node_modules/@tiptap/react/dist/index.js:1013,:1039) —<StrictMode>double-invokes effects, so cleanup can run twice.Verified at runtime
Production build served against a live backend, driving SPA navigation (
pushState+popstate) so components unmount without a page reload — a full reload resets the count regardless and proves nothing:.ProseMirrorcount returns to exactly the row count every round and drops 16 → 9 on leaving. No compounding.2. ELK layout engine loaded on every authenticated page
EntityLineageLayoutUtilsimportedELKUtilat module scope. That module also exports five pure geometry helpers, so importing any one of them dragged elkjs — 1.37 MB built.LineageControlButtonsimports onlycenterNodePosition, and is reached statically fromLineage.component.tsx. That was enough to pull elkjs into the shared chunk that 476 route chunks statically import. Every authenticated page downloaded and parsed the lineage layout engine — including glossary, which uses G6/antv-dagre and never calls ELK (OntologyExplorerchunk: 0 occurrences oforg.eclipse.elk).ELKLayoutis referenced in exactly one place, insidegetELKLayoutedElements, which is alreadyasyncand already awaited at every call site. Moving the import inside it is behaviour-preserving; the ELK engine itself is untouched.Measured on a production build
ServicesPagemust-load closureELKUtilchunk importers−1.37 MB on every authenticated route.
Verified against a live backend
/lineage: fetchesELKUtil+vendor-elkon demand, renders 83 nodes, 0 console errorsType of change
Tests
312 tests pass across the affected areas (
EntityLineageLayoutUtils,Lineage,BlockEditor,EntityLineage,LineageProvider,Lineagecomponents).14 suites fail to run with "Jest encountered an unexpected token" — a pre-existing environment issue with the
ui-core-componentsprebuilt dist. Control run on pristinemainshows 15 such failures, i.e. this branch has strictly fewer. I also A/B'd the three lineage suites with and without the ELK change: identical failures both ways, so none are caused by this PR.tsc --noEmit: 0 errors in both changed files.prettier --check: clean.Not verified: ESLint could not run locally — this branch's
eslint.config.mjsrequireseslint-plugin-sonarjs, absent from the checkouts available here. Relying on CI.Manual test steps
Editor leak: open a table of descriptions (
/tags/General, a glossary terms tab), rundocument.querySelectorAll('.ProseMirror').length, navigate away and back within the app (don't reload), re-run. Before: grows. After: returns to the row count.ELK: open a glossary page with DevTools Network filtered to
elk— no requests. Then open/lineage—ELKUtilandvendor-elkload, and the graph renders.Checklist
🤖 Generated with Claude Code
Summary by Gitar
ELKUtilinEntityLineageLayoutUtilsto prevent bundlingelkjs(~1.37MB) across all authenticated pagesThis will update automatically on new commits.