feat(DesignerV2): Added merge/diff view to code editor component#8819
feat(DesignerV2): Added merge/diff view to code editor component#8819
Conversation
🤖 AI PR Validation ReportPR Review ResultsThank you for your submission! Here's detailed feedback on your PR title and body compliance:✅ PR Title
✅ Commit Type
❌ Risk Level
✅ What & Why
|
| Section | Status | Recommendation |
|---|---|---|
| Title | ✅ | Keep as-is. |
| Commit Type | ✅ | Keep as-is. |
| Risk Level | ❌ | Update to Medium or provide justification for Low and change label accordingly. |
| What & Why | ✅ | Consider one additional sentence about usage toggle. |
| Impact of Change | Expand to mention bundle/dependency and runtime impact. | |
| Test Plan | ❌ | Update checkboxes: mark Unit tests added/updated; explain E2E omission if applicable. |
| Contributors | Add contributors if applicable; otherwise optional. | |
| Screenshots/Videos | ✅ | Screenshot provided; consider additional screenshots showing both panels. |
Final Message
Please update the PR body with the following before removing the needs-pr-update label:
-
Test Plan: check the
Unit tests added/updatedbox (you added unit tests in libs/designer-ui/tests). If you did manual testing as well, keep that checked. If you intentionally omitted E2E tests, add a short explanation. -
Risk Level: either change the selected risk to Medium (and update the label to
risk:medium) or keep Low but add a short justification explaining why adding@codemirror/mergeand changing editor rendering/theming is still considered Low risk (e.g., limited scope, covered by tests, no prod API changes, validated CI build size/perf). -
Impact of Change: expand slightly to mention the new props (showMerge, originalValue), the new dependency, and potential bundle/theming impact so reviewers can focus testing.
-
Contributors: add any reviewers/designers who contributed or confirm none to improve attribution.
-
Run CI and confirm there are no build or size regressions — mention the CI status in the PR once green.
Once these updates are made, re-request review and remove the needs-pr-update label. Thanks for the thorough implementation and added unit tests — once the PR body is aligned with the changes and the risk justification is clarified, this should be ready for merge.
Last updated: Tue, 17 Feb 2026 22:37:47 GMT
There was a problem hiding this comment.
Pull request overview
This PR adds merge/diff view functionality to the CodeMirrorEditor component in the designer-ui library. The feature enables a split-screen view comparing an original (read-only) version of code against a modified (editable) version, intended for displaying differences between draft and production workflow code in the Azure Portal.
Changes:
- Added @codemirror/merge dependency to enable split diff/merge view functionality
- Extended CodeMirrorEditor with originalValue and showMerge props to support merge view mode
- Fixed ESLint warning in CodeViewV2 component
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| pnpm-lock.yaml | Added @codemirror/merge@6.12.0 dependency and updated lock file snapshots |
| libs/designer-ui/package.json | Added @codemirror/merge dependency; reorganized @lezer packages; reformatted files array |
| libs/designer-ui/src/lib/editor/codemirror/types.ts | Added originalValue and showMerge props to CodeMirrorEditorProps interface |
| libs/designer-ui/src/lib/editor/codemirror/CodeMirrorEditor.tsx | Implemented merge view functionality with split-screen diff display; refactored theme configuration |
| apps/Standalone/src/designer/app/AzureLogicAppsDesigner/CodeViewV2.tsx | Added ESLint disable comment for react/display-name rule |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
| themeCompartment.of(createFluentTheme(isInverted)), | ||
| languageCompartment.of(getLanguageExtension(language)), |
There was a problem hiding this comment.
The MergeView uses separate compartments for panel A's theme and language (lines 250-251), but these compartments are the same shared instances used by panel B. The existing theme and language update effects (outside the changed region) only update viewRef.current, which points to panel B in merge mode. This means panel A won't receive theme or language updates after initialization, causing visual inconsistency between the two panels when these props change.
| value?: string; | ||
| originalValue?: string; |
There was a problem hiding this comment.
The new properties originalValue and showMerge are not documented with JSDoc comments. Consider adding documentation to explain:
- originalValue: The original/left-side content for merge/diff view
- showMerge: Whether to enable split merge/diff view mode (shows originalValue on left, value on right)
| value?: string; | |
| originalValue?: string; | |
| value?: string; | |
| /** | |
| * Original/left-side content for merge/diff view. | |
| */ | |
| originalValue?: string; | |
| /** | |
| * Whether to enable split merge/diff view mode (shows originalValue on the left and value on the right). | |
| */ |
| if (showMerge) { | ||
| const mergeView = new MergeView({ | ||
| a: { | ||
| doc: originalValue ?? '', | ||
| extensions: [ | ||
| EditorView.editable.of(false), | ||
| EditorState.readOnly.of(true), | ||
| themeCompartment.of(createFluentTheme(isInverted)), | ||
| languageCompartment.of(getLanguageExtension(language)), | ||
| EditorView.theme({ | ||
| ...baseThemeSpec, | ||
| '.cm-changedLine': { | ||
| backgroundColor: 'rgba(255, 128, 100, .12) !important', | ||
| }, | ||
| '.cm-content': { | ||
| backgroundColor: isInverted ? '#252423' : '#f3f3f3', | ||
| }, | ||
| }), | ||
| ...(lineNumbers === 'on' ? [lineNumbersExtension()] : []), | ||
| ], | ||
| }, | ||
| b: { | ||
| doc: value ?? defaultValue, | ||
| extensions, | ||
| }, | ||
| parent: containerRef.current, | ||
| }); | ||
|
|
||
| mergeViewRef.current = mergeView; | ||
| viewRef.current = mergeView.b; | ||
| onEditorRef?.(editorRef); | ||
| onEditorLoaded?.(); | ||
|
|
||
| return () => { | ||
| mergeView.destroy(); | ||
| mergeViewRef.current = null; | ||
| viewRef.current = null; | ||
| isInitializedRef.current = false; | ||
| }; | ||
| } |
There was a problem hiding this comment.
The new merge view functionality (showMerge and originalValue props) lacks test coverage. The existing tests in tests/CodeMirrorEditor.test.tsx don't cover the merge view mode. Consider adding tests for:
- Rendering with showMerge=true and originalValue set
- Verifying both panels are rendered
- Ensuring panel A is read-only
- Testing that getValue() returns the value from panel B (not panel A)
| const baseThemeSpec = { | ||
| '&': { | ||
| fontSize: `${fontSize}px`, | ||
| height: '100%', | ||
| minHeight: '100px', | ||
| boxSizing: 'border-box', | ||
| }, | ||
| '.cm-scroller': { | ||
| overflow: 'auto', | ||
| fontFamily: '"SF Mono", Menlo, Monaco, Consolas, "Liberation Mono", "Courier New", monospace', | ||
| fontWeight: '500', | ||
| letterSpacing: '0.5px', | ||
| lineHeight: '1.4', | ||
| }, | ||
| '.cm-content': { | ||
| textAlign: 'left', | ||
| padding: '4px 0', | ||
| fontVariantLigatures: 'none', | ||
| }, | ||
| '.cm-line': { | ||
| padding: '0 4px', | ||
| }, | ||
| '.cm-gutterElement': { | ||
| fontFamily: '"SF Mono", Menlo, Monaco, Consolas, "Liberation Mono", "Courier New", monospace', | ||
| fontWeight: '500', | ||
| }, | ||
| '.cm-changedLine': { | ||
| backgroundColor: 'rgba(100, 255, 128, .12) !important', | ||
| }, | ||
| }; |
There was a problem hiding this comment.
The baseThemeSpec object is defined inside the useEffect but is used to construct both the editorTheme and the theme for merge view panel A. This is fine functionally, but note that changes to fontSize prop won't update the merge view since it's only created once on mount. Consider whether fontSize updates should be supported for merge views.
| "@lezer/common": "^1.2.0", | ||
| "@lezer/highlight": "^1.2.0", | ||
| "@lezer/lr": "^1.4.0", |
There was a problem hiding this comment.
The dependency order has been changed to move @lezer packages after @lexical packages. While this doesn't affect functionality (dependencies are resolved alphabetically by package managers), it's inconsistent with typical alphabetical ordering conventions. The original position (after @codemirror packages) was more appropriate since @lezer is related to @codemirror.
| "files": [ | ||
| "build/lib/**/*", | ||
| "src" | ||
| ], |
There was a problem hiding this comment.
The files array formatting has been changed from a single-line array to a multi-line array. This is a stylistic change that improves readability. Consider applying this formatting consistently to other single-line arrays in the file for uniformity, though this is optional.
| // eslint-disable-next-line react/display-name | ||
| const CodeViewEditor = forwardRef(({ workflowKind, isConsumption }: CodeViewProps, ref) => { |
There was a problem hiding this comment.
Instead of disabling the react/display-name ESLint rule, consider adding a displayName property to the component after the forwardRef, following the pattern used in CodeMirrorEditor.tsx (line 373 in that file). For example: CodeViewEditor.displayName = 'CodeViewEditor'. This provides better debugging information in React DevTools and follows the established codebase pattern.
| if (showMerge) { | ||
| const mergeView = new MergeView({ | ||
| a: { | ||
| doc: originalValue ?? '', | ||
| extensions: [ | ||
| EditorView.editable.of(false), | ||
| EditorState.readOnly.of(true), | ||
| themeCompartment.of(createFluentTheme(isInverted)), | ||
| languageCompartment.of(getLanguageExtension(language)), | ||
| EditorView.theme({ | ||
| ...baseThemeSpec, | ||
| '.cm-changedLine': { | ||
| backgroundColor: 'rgba(255, 128, 100, .12) !important', | ||
| }, | ||
| '.cm-content': { | ||
| backgroundColor: isInverted ? '#252423' : '#f3f3f3', | ||
| }, | ||
| }), | ||
| ...(lineNumbers === 'on' ? [lineNumbersExtension()] : []), | ||
| ], | ||
| }, | ||
| b: { | ||
| doc: value ?? defaultValue, | ||
| extensions, | ||
| }, | ||
| parent: containerRef.current, | ||
| }); | ||
|
|
||
| mergeViewRef.current = mergeView; | ||
| viewRef.current = mergeView.b; | ||
| onEditorRef?.(editorRef); | ||
| onEditorLoaded?.(); | ||
|
|
||
| return () => { | ||
| mergeView.destroy(); | ||
| mergeViewRef.current = null; | ||
| viewRef.current = null; | ||
| isInitializedRef.current = false; | ||
| }; | ||
| } |
There was a problem hiding this comment.
The merge view is only created during component initialization (in a useEffect with empty dependency array), which means changes to showMerge, originalValue, language, or theme will not recreate or update the merge view. This creates several issues:
- If showMerge changes from false to true after mount, the merge view won't appear
- If originalValue changes, the left panel (panel A) won't update to show the new original content
- Theme changes (via isInverted) will only update panel B, not panel A which uses its own theme compartment
- Language changes will only update panel B, not panel A which uses its own language compartment
Consider adding useEffect hooks to handle updates to these props, similar to how theme and language updates are handled for the regular editor view (lines 306-321). For showMerge toggling, you may need to conditionally render different components or reinitialize the editor.
📊 Coverage CheckThe following changed files need attention:
Please add tests for the uncovered files before merging. |
1 participant
Commit Type
Risk Level
// Reason: No change to any production functionality
What & Why
Added merge view to code editor component.
This will be used for a diff view between draft and prod workflow code in portal.
Impact of Change
Test Plan
Contributors
Screenshots/Videos