Update @codemirror/merge to version 6.8.0
#2513
Conversation
WalkthroughThis pull request introduces enhancements to the CodeMirror diff editor, focusing on improving the handling and visualization of unchanged lines. Key changes include the addition of a new gutter widget class for collapsing unchanged lines, updates to CSS styles for better visual representation, and modifications to the text displayed for collapsed lines. The changes span multiple files, including component logic, utility functions, and styling, aiming to provide a more intuitive user interface for code diffs. Changes
Sequence DiagramsequenceDiagram
participant User
participant Editor
participant GutterWidget
participant CollapsedLines
User->>GutterWidget: Click collapse gutter
GutterWidget->>Editor: Find corresponding collapsed area
Editor->>CollapsedLines: Programmatically expand lines
CollapsedLines-->>User: Display expanded content
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
VasylMarchuk
changed the title
@codemirror/merge version 6.8.0@codemirror/merge to version 6.8.0
Codecov ReportAttention: Patch coverage is ✅ All tests successful. No failed tests found.
Additional details and impacted files📢 Thoughts on this report? Let us know! |
VasylMarchuk
force-pushed
the
code-mirror/update-cm-merge
branch
from
51999fd to
22c7ca8
Compare
VasylMarchuk
force-pushed
the
code-mirror/update-cm-merge
branch
from
22c7ca8 to
7b038f4
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
app/utils/code-mirror-collapse-unchanged-gutter-widget-class.ts (1)
15-17: Validate element references before usage.A null or undefined DOM element could cause runtime errors. For instance,
editor,gutter,gutterElement, orgutterElementSiblingsmight be null. Currently, you safely return if they are missing, but coverage can be improved by testing these edge cases.Would you like me to propose a test that confirms the code gracefully handles these null scenarios?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 16-16: app/utils/code-mirror-collapse-unchanged-gutter-widget-class.ts#L16
Added line #L16 was not covered by teststests/acceptance/course-admin/view-diffs-test.js (1)
105-105: Ensure test coverage matches dynamic line counts.Changing "⦚ 7 unchanged lines ⦚" to "Expand 7 unchanged lines" is consistent. If there's any logic that dynamically adjusts the displayed text, ensure test coverage remains updated for that logic.
app/utils/code-mirror-themes.ts (1)
195-225: Prudent approach for differentiating changed lines in Dark Mode.Blending colors is a neat technique to maintain readability. Just confirm that the chosen colors pass accessibility guidelines (contrast ratio).
Would you like me to generate a script to help verify color contrast compliance?
app/components/code-mirror.ts (1)
402-404: Phrase addition for collapsed lines is user-friendly.“Expand $ unchanged lines” conveys the action precisely. Great job providing a direct explanation instead of cryptic symbols.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (8)
app/components/code-mirror.ts(3 hunks)app/styles/app.css(0 hunks)app/styles/pages/course-admin-submission-diff.css(0 hunks)app/styles/pages/course-admin-updates-diff.css(0 hunks)app/utils/code-mirror-collapse-unchanged-gutter-widget-class.ts(1 hunks)app/utils/code-mirror-themes.ts(6 hunks)package.json(1 hunks)tests/acceptance/course-admin/view-diffs-test.js(1 hunks)
💤 Files with no reviewable changes (3)
- app/styles/app.css
- app/styles/pages/course-admin-submission-diff.css
- app/styles/pages/course-admin-updates-diff.css
🧰 Additional context used
🪛 GitHub Check: codecov/patch
app/utils/code-mirror-collapse-unchanged-gutter-widget-class.ts
[warning] 10-13: app/utils/code-mirror-collapse-unchanged-gutter-widget-class.ts#L10-L13
Added lines #L10 - L13 were not covered by tests
[warning] 16-16: app/utils/code-mirror-collapse-unchanged-gutter-widget-class.ts#L16
Added line #L16 was not covered by tests
[warning] 20-20: app/utils/code-mirror-collapse-unchanged-gutter-widget-class.ts#L20
Added line #L20 was not covered by tests
[warning] 23-23: app/utils/code-mirror-collapse-unchanged-gutter-widget-class.ts#L23
Added line #L23 was not covered by tests
[warning] 25-25: app/utils/code-mirror-collapse-unchanged-gutter-widget-class.ts#L25
Added line #L25 was not covered by tests
🔇 Additional comments (11)
app/utils/code-mirror-collapse-unchanged-gutter-widget-class.ts (2)
9-13: Consider test coverage for the click event.Currently, there's no test verifying that the click listener correctly locates and interacts with the corresponding expand bar in the content. Adding an acceptance or integration test can prevent regressions and confirm that this event-driven behavior works as intended.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 10-13: app/utils/code-mirror-collapse-unchanged-gutter-widget-class.ts#L10-L13
Added lines #L10 - L13 were not covered by tests
32-40: Nice abstraction for creating a specialized gutter widget.Returning
nullif the widget type doesn't match'collapsed-unchanged-code'is a clean approach that avoids overhead. Just ensure test coverage exists to confirm that the gutter class is only assigned when needed.tests/acceptance/course-admin/view-diffs-test.js (2)
93-93: Kudos on updating the placeholder text.Switching to “Expand X unchanged lines” aligns with the new CodeMirror phrases. This ensures clarity for end-users.
99-99: Maintain consistent placeholders.All placeholders now use the same “Expand X unchanged lines” format. Great move for consistency. If future placeholders differ, consider a constant or translation approach to maintain uniform text.
app/utils/code-mirror-themes.ts (4)
67-106: Great usage of Tailwind colors for branding consistency.The newly introduced
.cm-collapseUnchangedBarNeighborstyle and its hover states use Tailwind color tokens, aligning well with the rest of the app. Ensure you account for the UI across different screen resolutions and color schemes.
Line range hint
113-128: Hover style improvements enhance discoverability.Removing pseudo-elements and focusing on background/color changes is simpler and user-friendly. Good approach for maintaining a minimal and consistent design system.
Line range hint
227-242: Dark theme alignment with the new collapsed lines workflow.Ensuring that
.cm-collapsedLinesand.cm-collapseUnchangedBarNeighborare styled appropriately in dark mode prevents jarring user experiences. Looks well-structured.
130-136: Pseudo-elements removal looks tidy.By removing
:beforeand:aftercontent, the code is more concise. Verify that related classes aren't depending on these pseudo-elements in older style sheets or code references.✅ Verification successful
The previous search might have been too specific. Let's broaden it to find any usage of the class and any CSS files with pseudo-elements to ensure we don't miss any related styling.
Pseudo-element removal is safe to proceed
The search results show that all pseudo-element styles for the
cm-collapsedLinesclass are contained within the same file (app/utils/code-mirror-themes.ts). The other pseudo-elements found in the file are for different styling purposes (showing "+" and "-" symbols) and are unrelated to the removed ones. There are no other dependencies on these pseudo-elements in other files or stylesheets.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Searching older references to cm-collapsedLines pseudo-elements rg -A 3 "cm-collapsedLines:before|cm-collapsedLines:after"Length of output: 60
Script:
#!/bin/bash # Let's search for any references to cm-collapsedLines class in the codebase rg -A 3 "cm-collapsedLines" # Also search for any CSS/SCSS files that might contain pseudo-element styles fd -e css -e scss -x rg -l "::before|::after|\:before|\:after" {}Length of output: 2550
Script:
#!/bin/bash # Let's try a different approach to find pseudo-element styles # Search in the specific file and its directory for any pseudo-element references rg -A 3 "(:before|:after|::before|::after)" "app/utils/code-mirror-themes.ts" rg -A 3 "(:before|:after|::before|::after)" "app/utils/"Length of output: 1308
app/components/code-mirror.ts (2)
40-40: ImportingcollapseUnchangedGutterWidgetClass.This import cleanly integrates the new gutter widget extension. Make sure consumers of this component are aware of the new functionality to avoid confusion.
139-139: Optional extension pattern is tidy.Wrapping
collapseUnchangedGutterWidgetClass()in a conditional iforiginalDocumentis provided ensures that the widget is only applied when relevant.package.json (1)
42-42: LGTM! Verify test coverage for the new CodeMirror features.The upgrade to
@codemirror/merge@6.8.0aligns with the PR objectives and maintains compatibility with other CodeMirror packages. The caret range (^6.8.0) appropriately allows for future patch updates while preventing breaking changes.Run the test suite to ensure the new features work as expected:
app/utils/code-mirror-collapse-unchanged-gutter-widget-class.ts
Outdated
Show resolved
Hide resolved
VasylMarchuk
force-pushed
the
code-mirror/update-cm-merge
branch
from
7b038f4 to
92d897d
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/utils/code-mirror-collapse-unchanged-gutter-widget-class.ts (1)
32-40: LGTM! Consider adding type safety improvements.The utility function implementation is clean and follows CodeMirror's extension patterns.
Consider adding a type guard for better type safety:
+ interface CollapsedUnchangedWidget { + type: 'collapsed-unchanged-code'; + } + + function isCollapsedUnchangedWidget(widget: unknown): widget is CollapsedUnchangedWidget { + return typeof widget === 'object' && widget !== null && 'type' in widget && + widget.type === 'collapsed-unchanged-code'; + } + export function collapseUnchangedGutterWidgetClass() { return gutterWidgetClass.of(function (_view, widget, _block): GutterMarker | null { - if ('type' in widget && widget.type === 'collapsed-unchanged-code') { + if (isCollapsedUnchangedWidget(widget)) { return new CollapseUnchangedGutterMarker(); } return null; }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
app/components/code-mirror.ts(3 hunks)app/styles/app.css(0 hunks)app/styles/pages/course-admin-submission-diff.css(0 hunks)app/styles/pages/course-admin-updates-diff.css(0 hunks)app/utils/code-mirror-collapse-unchanged-gutter-widget-class.ts(1 hunks)app/utils/code-mirror-themes.ts(6 hunks)tests/acceptance/course-admin/view-diffs-test.js(1 hunks)
💤 Files with no reviewable changes (3)
- app/styles/pages/course-admin-submission-diff.css
- app/styles/pages/course-admin-updates-diff.css
- app/styles/app.css
🚧 Files skipped from review as they are similar to previous changes (3)
- app/components/code-mirror.ts
- tests/acceptance/course-admin/view-diffs-test.js
- app/utils/code-mirror-themes.ts
🧰 Additional context used
🪛 GitHub Check: codecov/patch
app/utils/code-mirror-collapse-unchanged-gutter-widget-class.ts
[warning] 10-13: app/utils/code-mirror-collapse-unchanged-gutter-widget-class.ts#L10-L13
Added lines #L10 - L13 were not covered by tests
[warning] 16-16: app/utils/code-mirror-collapse-unchanged-gutter-widget-class.ts#L16
Added line #L16 was not covered by tests
[warning] 20-20: app/utils/code-mirror-collapse-unchanged-gutter-widget-class.ts#L20
Added line #L20 was not covered by tests
[warning] 23-23: app/utils/code-mirror-collapse-unchanged-gutter-widget-class.ts#L23
Added line #L23 was not covered by tests
[warning] 25-25: app/utils/code-mirror-collapse-unchanged-gutter-widget-class.ts#L25
Added line #L25 was not covered by tests
🔇 Additional comments (2)
app/utils/code-mirror-collapse-unchanged-gutter-widget-class.ts (2)
1-4: LGTM! Clean class declaration with appropriate imports.The class extends GutterMarker appropriately and follows consistent naming conventions.
6-29: Add unit tests for the click handler logic.The click handler contains complex DOM traversal and event handling logic that should be tested. Static analysis indicates missing test coverage.
Would you like me to help create unit tests for this component? The tests should cover:
- Element creation and class assignment
- Click handler behavior with various DOM structures
- Error cases when elements are not found
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 10-13: app/utils/code-mirror-collapse-unchanged-gutter-widget-class.ts#L10-L13
Added lines #L10 - L13 were not covered by tests
[warning] 16-16: app/utils/code-mirror-collapse-unchanged-gutter-widget-class.ts#L16
Added line #L16 was not covered by tests
[warning] 20-20: app/utils/code-mirror-collapse-unchanged-gutter-widget-class.ts#L20
Added line #L20 was not covered by tests
[warning] 23-23: app/utils/code-mirror-collapse-unchanged-gutter-widget-class.ts#L23
Added line #L23 was not covered by tests
[warning] 25-25: app/utils/code-mirror-collapse-unchanged-gutter-widget-class.ts#L25
Added line #L25 was not covered by tests
| toDOM(_view: EditorView): Node { | ||
| const el = document.createElement('div'); | ||
| el.className = 'cm-collapseUnchangedBarGutterElement'; | ||
| el.addEventListener('click', (_e) => { | ||
| const editor = el.closest('.cm-editor'); | ||
| const gutter = el.closest('.cm-gutter'); | ||
| const gutterElement = el.closest('.cm-collapseUnchangedBarNeighbor'); | ||
| const gutterElementSiblings = gutter?.querySelectorAll('.cm-collapseUnchangedBarNeighbor'); | ||
|
|
||
| if (!editor || !gutter || !gutterElement || !gutterElementSiblings) { | ||
| return; | ||
| } | ||
|
|
||
| // Find the index of the clicked gutter element | ||
| const gutterElementIndex = [...gutterElementSiblings.values()].indexOf(gutterElement); | ||
|
|
||
| // Find the corresponding expand unchanged bar in the content | ||
| const expandCollapsedBar = editor.querySelectorAll<HTMLElement>('.cm-content .cm-collapsedLines').item(gutterElementIndex); | ||
|
|
||
| expandCollapsedBar?.click(); | ||
| }); | ||
|
|
||
| return el; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using type-safe DOM queries and error handling.
The DOM manipulation code could be improved for better type safety and error handling:
- Replace
querySelectorwith more specific type assertions - Add error logging for failed DOM traversals
- Consider extracting the click handler logic for better testability
Here's a suggested improvement:
toDOM(_view: EditorView): Node {
const el = document.createElement('div');
el.className = 'cm-collapseUnchangedBarGutterElement';
el.addEventListener('click', (_e) => {
- const editor = el.closest('.cm-editor');
- const gutter = el.closest('.cm-gutter');
- const gutterElement = el.closest('.cm-collapseUnchangedBarNeighbor');
- const gutterElementSiblings = gutter?.querySelectorAll('.cm-collapseUnchangedBarNeighbor');
+ const editor = el.closest<HTMLElement>('.cm-editor');
+ const gutter = el.closest<HTMLElement>('.cm-gutter');
+ const gutterElement = el.closest<HTMLElement>('.cm-collapseUnchangedBarNeighbor');
if (!editor || !gutter || !gutterElement) {
+ console.warn('Required DOM elements not found');
return;
}
+ const gutterElementSiblings = Array.from(
+ gutter.querySelectorAll<HTMLElement>('.cm-collapseUnchangedBarNeighbor')
+ );
+
// Find the index of the clicked gutter element
- const gutterElementIndex = [...gutterElementSiblings.values()].indexOf(gutterElement);
+ const gutterElementIndex = gutterElementSiblings.indexOf(gutterElement);
+ if (gutterElementIndex === -1) {
+ console.warn('Gutter element not found in siblings');
+ return;
+ }
// Find the corresponding expand unchanged bar in the content
- const expandCollapsedBar = editor.querySelectorAll<HTMLElement>('.cm-content .cm-collapsedLines').item(gutterElementIndex);
+ const expandCollapsedBars = editor.querySelectorAll<HTMLElement>(
+ '.cm-content .cm-collapsedLines'
+ );
+ const expandCollapsedBar = expandCollapsedBars[gutterElementIndex];
- expandCollapsedBar?.click();
+ if (expandCollapsedBar) {
+ expandCollapsedBar.click();
+ } else {
+ console.warn('Corresponding collapsed bar not found');
+ }
});
return el;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| toDOM(_view: EditorView): Node { | |
| const el = document.createElement('div'); | |
| el.className = 'cm-collapseUnchangedBarGutterElement'; | |
| el.addEventListener('click', (_e) => { | |
| const editor = el.closest('.cm-editor'); | |
| const gutter = el.closest('.cm-gutter'); | |
| const gutterElement = el.closest('.cm-collapseUnchangedBarNeighbor'); | |
| const gutterElementSiblings = gutter?.querySelectorAll('.cm-collapseUnchangedBarNeighbor'); | |
| if (!editor || !gutter || !gutterElement || !gutterElementSiblings) { | |
| return; | |
| } | |
| // Find the index of the clicked gutter element | |
| const gutterElementIndex = [...gutterElementSiblings.values()].indexOf(gutterElement); | |
| // Find the corresponding expand unchanged bar in the content | |
| const expandCollapsedBar = editor.querySelectorAll<HTMLElement>('.cm-content .cm-collapsedLines').item(gutterElementIndex); | |
| expandCollapsedBar?.click(); | |
| }); | |
| return el; | |
| } | |
| toDOM(_view: EditorView): Node { | |
| const el = document.createElement('div'); | |
| el.className = 'cm-collapseUnchangedBarGutterElement'; | |
| el.addEventListener('click', (_e) => { | |
| const editor = el.closest<HTMLElement>('.cm-editor'); | |
| const gutter = el.closest<HTMLElement>('.cm-gutter'); | |
| const gutterElement = el.closest<HTMLElement>('.cm-collapseUnchangedBarNeighbor'); | |
| if (!editor || !gutter || !gutterElement) { | |
| console.warn('Required DOM elements not found'); | |
| return; | |
| } | |
| const gutterElementSiblings = Array.from( | |
| gutter.querySelectorAll<HTMLElement>('.cm-collapseUnchangedBarNeighbor') | |
| ); | |
| // Find the index of the clicked gutter element | |
| const gutterElementIndex = gutterElementSiblings.indexOf(gutterElement); | |
| if (gutterElementIndex === -1) { | |
| console.warn('Gutter element not found in siblings'); | |
| return; | |
| } | |
| // Find the corresponding expand unchanged bar in the content | |
| const expandCollapsedBars = editor.querySelectorAll<HTMLElement>( | |
| '.cm-content .cm-collapsedLines' | |
| ); | |
| const expandCollapsedBar = expandCollapsedBars[gutterElementIndex]; | |
| if (expandCollapsedBar) { | |
| expandCollapsedBar.click(); | |
| } else { | |
| console.warn('Corresponding collapsed bar not found'); | |
| } | |
| }); | |
| return el; | |
| } |
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 10-13: app/utils/code-mirror-collapse-unchanged-gutter-widget-class.ts#L10-L13
Added lines #L10 - L13 were not covered by tests
[warning] 16-16: app/utils/code-mirror-collapse-unchanged-gutter-widget-class.ts#L16
Added line #L16 was not covered by tests
[warning] 20-20: app/utils/code-mirror-collapse-unchanged-gutter-widget-class.ts#L20
Added line #L20 was not covered by tests
[warning] 23-23: app/utils/code-mirror-collapse-unchanged-gutter-widget-class.ts#L23
Added line #L23 was not covered by tests
[warning] 25-25: app/utils/code-mirror-collapse-unchanged-gutter-widget-class.ts#L25
Added line #L25 was not covered by tests
VasylMarchuk
changed the title
@codemirror/merge to version 6.8.0@codemirror/merge to version 6.8.0
Related to #1231
Brief
This updates
@codemirror/mergefrom version6.7.4to version6.8.0and adjusts all the necessary styles.Details
@codemirror/mergeplugin to version6.8.0, changelog herecode-mirror-collapse-unchanged-gutter-widget-class— a custom extension used wheneveroriginalDocumentis passed:.cm-collapseUnchangedBarNeighborclass to all gutter elements that are next to the "Expand X unchanged lines" bar in the document.cm-collapseUnchangedBarGutterElementchild elements inside the above gutter elements.cm-changeGutterare:Demo
2025-01-03.18.49.11.mov
2025-01-03.18.52.47.mov
Checklist
[percy]in the message to trigger)Summary by CodeRabbit
Release Notes
New Features
Styling Updates
Dependency Update
@codemirror/mergeto version 6.8.0.Performance