Skip to content

fixes 27953: migrate CodeMirror v5 to v6 (@codemirror/* packages)#28438

Open
satender-kumar-collate wants to merge 31 commits into
mainfrom
upgrade/cmV6
Open

fixes 27953: migrate CodeMirror v5 to v6 (@codemirror/* packages)#28438
satender-kumar-collate wants to merge 31 commits into
mainfrom
upgrade/cmV6

Conversation

@satender-kumar-collate
Copy link
Copy Markdown
Contributor

@satender-kumar-collate satender-kumar-collate commented May 26, 2026

Replaces react-codemirror2 / codemirror@5 with the modular @codemirror v6 packages. Introduces a useCodeMirror hook that mounts a single EditorView, syncs external value changes via an Annotation (suppressing spurious onChange calls), and reconfigures language/options via a Compartment. All CSS selectors, Playwright locators, and Jest mocks are updated to use CM6 class names (.cm-editor, .cm-scroller, .cm-gutters, etc.).

Fixes #27953
Screenshot 2026-06-02 at 10 17 21 AM
Screenshot 2026-06-02 at 10 18 11 AM
Screenshot 2026-06-02 at 10 18 48 AM
Screenshot 2026-06-02 at 10 20 39 AM
Screenshot 2026-06-02 at 10 22 02 AM
Screenshot 2026-06-02 at 10 40 34 AM
Screenshot 2026-06-02 at 10 41 50 AM
Screenshot 2026-06-02 at 10 41 58 AM

Type of change:

  • Bug fix
  • Improvement
  • New feature
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation

High-level design:

N/A — small change.

Tests:

Use cases covered

Unit tests

Backend integration tests

Ingestion integration tests

Playwright (UI) tests

Manual testing performed

UI screen recording / screenshots:

Not applicable.

Checklist:

  • I have read the CONTRIBUTING document.
  • My PR title is Fixes <issue-number>: <short explanation>
  • My PR is linked to a GitHub issue via Fixes #<issue-number> above.
  • I have commented on my code, particularly in hard-to-understand areas.
  • For JSON Schema changes: I updated the migration scripts or explained why it is not needed.
  • For UI changes: I attached a screen recording and/or screenshots above.
  • I have added tests (unit / integration / Playwright as applicable) and listed them above.

Summary by Gitar

  • Dependency updates:
    • Added react-markdown version 10.1.0 to package.json dependencies.
  • Refactored CodeMirror props:
    • Simplified readOnly boolean logic in CodeEditor and SchemaEditor components.
  • Updated Playwright selectors:
    • Replaced deprecated pre[role='presentation'] locators with .cm-content class to support CodeMirror v6 DOM structure.
    • Switched from keyboard.type to keyboard.insertText for more reliable CM6 editor input handling in Playwright tests.
  • Maintenance:
    • Cleaned up redundant logic in SchemaEditor.test.tsx.
    • Added visibility assertions before interacting with CodeMirror editors in test suites.

This will update automatically on new commits.

Replaces react-codemirror2 / codemirror@5 with the modular @codemirror v6
packages. Introduces a useCodeMirror hook that mounts a single EditorView,
syncs external value changes via an Annotation (suppressing spurious onChange
calls), and reconfigures language/options via a Compartment. All CSS selectors,
Playwright locators, and Jest mocks are updated to use CM6 class names
(.cm-editor, .cm-scroller, .cm-gutters, etc.).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@satender-kumar-collate satender-kumar-collate requested a review from a team as a code owner May 26, 2026 11:55
@satender-kumar-collate satender-kumar-collate added UI UI specific issues safe to test Add this label to run secure Github workflows on PRs labels May 26, 2026
Comment thread openmetadata-ui/src/main/resources/ui/src/hooks/useCodeMirror.ts Outdated
Comment thread openmetadata-ui/src/main/resources/ui/src/hooks/useCodeMirror.ts Outdated
@satender-kumar-collate satender-kumar-collate changed the title feat(ui): migrate CodeMirror v5 to v6 (@codemirror/* packages) fixes 27953: migrate CodeMirror v5 to v6 (@codemirror/* packages) May 26, 2026
@siddhant1 siddhant1 requested a review from Copilot May 26, 2026 12:01
@gitar-bot
Copy link
Copy Markdown

gitar-bot Bot commented May 26, 2026

Code Review ⚠️ Changes requested 0 resolved / 3 findings

Migrates CodeMirror v5 to v6 using a custom hook for EditorView management, but introduces instability in opts.mode dependency tracking, missing height constraints in the ManifestJsonWidget, and incomplete component cleanup.

⚠️ Performance: opts.mode object reference instability triggers reconfigure on every render

📄 openmetadata-ui/src/main/resources/ui/src/hooks/useCodeMirror.ts:220-221 📄 openmetadata-ui/src/main/resources/ui/src/components/Database/SchemaEditor/SchemaEditor.tsx:32-35

The useEffect at line 210-230 in useCodeMirror.ts includes opts.mode as a dependency. Since mode has an inline default value { name: CSMode.JAVASCRIPT, json: true } in both SchemaEditor.tsx and CodeEditor.tsx, a new object reference is created on every render, causing dynamicCompartment.current.reconfigure(...) to fire on every re-render. This dispatches a transaction to the editor unnecessarily and could cause performance issues (especially with frequent parent re-renders).

The hook should either compare mode by value (e.g., use opts.mode?.name and opts.mode?.json as separate deps) or the components should stabilize the default with useMemo.

Replace opts.mode with its primitive constituents in the dependency array to avoid reference comparison issues
], [
  opts.mode?.name,
  opts.mode?.json,
  opts.readOnly,
  opts.showLineNumbers,
  opts.lineWrapping,
  opts.showFoldGutter,
  opts.styleActiveLine,
  opts.matchBrackets,
  opts.autoCloseBrackets,
  opts.tabSize,
]);
⚠️ Bug: ManifestJsonWidget editor height broken: missing height:100% on wrapper

📄 openmetadata-ui/src/main/resources/ui/src/components/common/Form/JSONSchema/JsonSchemaWidgets/ManifestJsonWidget/manifest-json-widget.less:26-28

In manifest-json-widget.less, the old CSS applied height: 100% !important to three selectors: .manifest-json-widget-editor, .manifest-json-widget-editor > div, and .CodeMirror. The new CSS only applies it to .manifest-json-widget-editor > div (which targets the .cm-editor). However, the .manifest-json-widget-editor element itself (the editorRef div in SchemaEditor) no longer gets height: 100%, breaking the height chain from .manifest-json-widget-resize-wrapper.manifest-json-widget-editor.cm-editor. The editor will collapse to its content height instead of filling the resizable wrapper.

Restore height: 100% on the .manifest-json-widget-editor wrapper to maintain the height chain
.manifest-json-widget-editor,
.manifest-json-widget-editor > div {
  height: 100% !important;
}
💡 Edge Case: useCodeMirror never cleans up EditorView on editorRef detach

📄 openmetadata-ui/src/main/resources/ui/src/hooks/useCodeMirror.ts:145-159

The useEffect that creates the EditorView (line 145-193) has an empty dependency array and only runs once. If editorRef.current is null at mount time (e.g., the component conditionally renders the container div), the editor is never created. Conversely, if the ref div is replaced later, the old view remains attached to a detached DOM node. This is a minor edge case since the current callers always render the div unconditionally, but it's worth noting as a fragility if the hook is reused elsewhere.

🤖 Prompt for agents
Code Review: Migrates CodeMirror v5 to v6 using a custom hook for EditorView management, but introduces instability in `opts.mode` dependency tracking, missing height constraints in the ManifestJsonWidget, and incomplete component cleanup.

1. ⚠️ Performance: opts.mode object reference instability triggers reconfigure on every render
   Files: openmetadata-ui/src/main/resources/ui/src/hooks/useCodeMirror.ts:220-221, openmetadata-ui/src/main/resources/ui/src/components/Database/SchemaEditor/SchemaEditor.tsx:32-35

   The `useEffect` at line 210-230 in `useCodeMirror.ts` includes `opts.mode` as a dependency. Since `mode` has an inline default value `{ name: CSMode.JAVASCRIPT, json: true }` in both `SchemaEditor.tsx` and `CodeEditor.tsx`, a new object reference is created on every render, causing `dynamicCompartment.current.reconfigure(...)` to fire on every re-render. This dispatches a transaction to the editor unnecessarily and could cause performance issues (especially with frequent parent re-renders).
   
   The hook should either compare `mode` by value (e.g., use `opts.mode?.name` and `opts.mode?.json` as separate deps) or the components should stabilize the default with `useMemo`.

   Fix (Replace opts.mode with its primitive constituents in the dependency array to avoid reference comparison issues):
   ], [
     opts.mode?.name,
     opts.mode?.json,
     opts.readOnly,
     opts.showLineNumbers,
     opts.lineWrapping,
     opts.showFoldGutter,
     opts.styleActiveLine,
     opts.matchBrackets,
     opts.autoCloseBrackets,
     opts.tabSize,
   ]);

2. ⚠️ Bug: ManifestJsonWidget editor height broken: missing height:100% on wrapper
   Files: openmetadata-ui/src/main/resources/ui/src/components/common/Form/JSONSchema/JsonSchemaWidgets/ManifestJsonWidget/manifest-json-widget.less:26-28

   In `manifest-json-widget.less`, the old CSS applied `height: 100% !important` to three selectors: `.manifest-json-widget-editor`, `.manifest-json-widget-editor > div`, and `.CodeMirror`. The new CSS only applies it to `.manifest-json-widget-editor > div` (which targets the `.cm-editor`). However, the `.manifest-json-widget-editor` element itself (the `editorRef` div in SchemaEditor) no longer gets `height: 100%`, breaking the height chain from `.manifest-json-widget-resize-wrapper` → `.manifest-json-widget-editor` → `.cm-editor`. The editor will collapse to its content height instead of filling the resizable wrapper.

   Fix (Restore height: 100% on the .manifest-json-widget-editor wrapper to maintain the height chain):
   .manifest-json-widget-editor,
   .manifest-json-widget-editor > div {
     height: 100% !important;
   }

3. 💡 Edge Case: useCodeMirror never cleans up EditorView on editorRef detach
   Files: openmetadata-ui/src/main/resources/ui/src/hooks/useCodeMirror.ts:145-159

   The `useEffect` that creates the `EditorView` (line 145-193) has an empty dependency array and only runs once. If `editorRef.current` is null at mount time (e.g., the component conditionally renders the container div), the editor is never created. Conversely, if the ref div is replaced later, the old view remains attached to a detached DOM node. This is a minor edge case since the current callers always render the div unconditionally, but it's worth noting as a fragility if the hook is reused elsewhere.

Options

Display: compact → Showing less information.

Comment with these commands to change:

Compact
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Gitar

Satender and others added 3 commits May 26, 2026 17:33
…widget

- Expand opts.mode into primitive constituents (opts.mode?.name, opts.mode?.json)
  in the dynamic compartment useEffect dep array to avoid unnecessary
  reconfigures on every render when an inline default object is passed
- Change editorRef type from useRef<HTMLDivElement>(null!) to
  useRef<HTMLDivElement | null>(null) to remove the non-null assertion and
  correctly type the null case
- Restore .manifest-json-widget-editor to the height:100% selector group so
  the height chain from the resize wrapper through to .cm-editor is maintained

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Adds a comment on the mount useEffect documenting that the hook assumes
the host <div ref={editorRef} /> is rendered unconditionally by the
caller, and noting the edge cases (null at mount / ref replacement) that
are acceptable for current SchemaEditor / CodeEditor usage. Fixes #1 and
#2 (primitive mode deps + manifest-json-widget height chain) were already
applied in the previous commit.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Issue #1 (root cause): Replace inline mode default objects in SchemaEditor
and CodeEditor with a module-level DEFAULT_MODE constant so the same
reference is reused across renders, eliminating unnecessary reconfigures
regardless of how the hook tracks deps.

Issue #3 (edge case): Replace the empty-dep useEffect + editorRef useRef
pattern with a callback ref (useCallback with no deps). React calls the
callback whenever the DOM node mounts, unmounts, or is replaced, so the
EditorView is always destroyed on detach — including when the host element
is conditionally rendered or swapped without the parent unmounting.
Update test mocks to return editorRef: jest.fn() to match the new
(node: HTMLDivElement | null) => void type.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 26, 2026

Jest test Coverage

UI tests summary

Lines Statements Branches Functions
Coverage: 63%
63.39% (67308/106167) 44.34% (37067/83591) 46.73% (11136/23829)

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 26, 2026

🟡 Playwright Results — all passed (17 flaky)

✅ 4265 passed · ❌ 0 failed · 🟡 17 flaky · ⏭️ 88 skipped

Shard Passed Failed Flaky Skipped
🟡 Shard 1 300 0 1 4
🟡 Shard 2 801 0 3 9
🟡 Shard 3 804 0 3 8
🟡 Shard 4 850 0 5 12
🟡 Shard 5 720 0 1 47
🟡 Shard 6 790 0 4 8
🟡 17 flaky test(s) (passed on retry)
  • Pages/AuditLogs.spec.ts › should apply both User and EntityType filters simultaneously (shard 1, 1 retry)
  • Features/BulkImport.spec.ts › Database service (shard 2, 1 retry)
  • Features/DataQuality/TestCaseImportExportE2eFlow.spec.ts › Admin: Complete export-import-validate flow (shard 2, 1 retry)
  • Features/DataQuality/TestCaseResultPermissions.spec.ts › User with only VIEW cannot PATCH results (shard 2, 1 retry)
  • Features/Table.spec.ts › Table pagination with sorting should works (shard 3, 1 retry)
  • Features/Tasks/DomainFiltering.spec.ts › selecting All Domains removes domain filter from feed API call (shard 3, 2 retries)
  • Flow/ExploreAggregationCountsMatching.spec.ts › should verify left panel counts and tab search results for normal search (shard 3, 1 retry)
  • Flow/PersonaDeletionUserProfile.spec.ts › User profile loads correctly before and after persona deletion (shard 4, 1 retry)
  • Flow/PersonaFlow.spec.ts › Set default persona for team should work properly (shard 4, 1 retry)
  • Pages/CustomProperties.spec.ts › Number (shard 4, 1 retry)
  • Pages/CustomProperties.spec.ts › Duration (shard 4, 1 retry)
  • Pages/DataContractsSemanticRules.spec.ts › Validate DataProduct Rule Any_In (shard 4, 1 retry)
  • Pages/ExplorePageRightPanel_KnowledgeCenter.spec.ts › Should remove user owner for knowledgeCenter (shard 5, 1 retry)
  • Pages/Lineage/DataAssetLineage.spec.ts › Column lineage for topic -> mlModel (shard 6, 1 retry)
  • Pages/Lineage/LineageFilters.spec.ts › Verify Impact Analysis service filter selection (shard 6, 1 retry)
  • Pages/TasksUIFlow.spec.ts › Create and reject tag task for Pipeline via UI (shard 6, 1 retry)
  • Pages/UserDetails.spec.ts › Admin user can assign and remove domain from a user (shard 6, 1 retry)

📦 Download artifacts

How to debug locally
# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip    # view trace

Satender and others added 2 commits May 27, 2026 08:52
…CM6 .cm-content

CM6 does not render `pre[role='presentation']` elements — those were CM5-specific
line nodes. Tests were timing out because the locator never resolved. Replace all
six occurrences with `.cm-content`, the CM6 contenteditable div that accepts input.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Drop redundant `?? false` from readOnly expression (no-constant-binary-expression):
  `readOnly ?? options?.readOnly === true` already yields a boolean
- Remove unused `capturedOnFocus` variable from SchemaEditor.test.tsx

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Comment thread openmetadata-ui/src/main/resources/ui/src/hooks/useCodeMirror.ts
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 5, 2026

❌ PR checklist incomplete

This 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 skip-pr-checks label.

@gitar-bot
Copy link
Copy Markdown

gitar-bot Bot commented Jun 5, 2026

Code Review ✅ Approved 3 resolved / 3 findings

Migrates CodeMirror from v5 to v6 with a new useCodeMirror hook and updated component configurations. Resolves re-render instability, editor height issues, and missing cleanup logic.

✅ 3 resolved
Performance: opts.mode object reference instability triggers reconfigure on every render

📄 openmetadata-ui/src/main/resources/ui/src/hooks/useCodeMirror.ts:220-221 📄 openmetadata-ui/src/main/resources/ui/src/components/Database/SchemaEditor/SchemaEditor.tsx:32-35
The useEffect at line 210-230 in useCodeMirror.ts includes opts.mode as a dependency. Since mode has an inline default value { name: CSMode.JAVASCRIPT, json: true } in both SchemaEditor.tsx and CodeEditor.tsx, a new object reference is created on every render, causing dynamicCompartment.current.reconfigure(...) to fire on every re-render. This dispatches a transaction to the editor unnecessarily and could cause performance issues (especially with frequent parent re-renders).

The hook should either compare mode by value (e.g., use opts.mode?.name and opts.mode?.json as separate deps) or the components should stabilize the default with useMemo.

Bug: ManifestJsonWidget editor height broken: missing height:100% on wrapper

📄 openmetadata-ui/src/main/resources/ui/src/components/common/Form/JSONSchema/JsonSchemaWidgets/ManifestJsonWidget/manifest-json-widget.less:26-28
In manifest-json-widget.less, the old CSS applied height: 100% !important to three selectors: .manifest-json-widget-editor, .manifest-json-widget-editor > div, and .CodeMirror. The new CSS only applies it to .manifest-json-widget-editor > div (which targets the .cm-editor). However, the .manifest-json-widget-editor element itself (the editorRef div in SchemaEditor) no longer gets height: 100%, breaking the height chain from .manifest-json-widget-resize-wrapper.manifest-json-widget-editor.cm-editor. The editor will collapse to its content height instead of filling the resizable wrapper.

Edge Case: useCodeMirror never cleans up EditorView on editorRef detach

📄 openmetadata-ui/src/main/resources/ui/src/hooks/useCodeMirror.ts:145-159
The useEffect that creates the EditorView (line 145-193) has an empty dependency array and only runs once. If editorRef.current is null at mount time (e.g., the component conditionally renders the container div), the editor is never created. Conversely, if the ref div is replaced later, the old view remains attached to a detached DOM node. This is a minor edge case since the current callers always render the div unconditionally, but it's worth noting as a fragility if the hook is reused elsewhere.

Options

Display: compact → Showing less information.

Comment with these commands to change:

Compact
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Gitar

@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud Bot commented Jun 5, 2026

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

safe to test Add this label to run secure Github workflows on PRs UI UI specific issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Codemirror version migration

3 participants