Use CodeMirror on Admin Submissions Diff tab #2204
Conversation
WalkthroughThe changes involve significant modifications to the code rendering system, transitioning from an in-house solution to utilizing CodeMirror for displaying code and diffs. Enhancements include improved visual presentation of unchanged sections, support for dark mode, and the introduction of a new helper for converting diff strings into document objects. The updates also refine the testing framework for diff views, focusing on the interaction with collapsed lines and the rendering of changed files. Changes
Possibly related issues
Possibly related PRs
Suggested reviewers
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 (
|
Test Results 1 files ±0 1 suites ±0 6m 25s ⏱️ -2s Results for commit 9fe2f7b. ± Comparison against base commit 5580ca6. This pull request removes 5 and adds 4 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
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
use-codemirror-on-admin-submissions-diff
branch
from
cf3d748
to
6445e8c
Compare
VasylMarchuk
force-pushed
the
use-codemirror-on-admin-submissions-diff
branch
from
6445e8c
to
68961e7
Compare
Bundle ReportChanges will increase total bundle size by 13.63kB (0.04%) ⬆️. This is within the configured threshold ✅ Detailed changes
|
|
@VasylMarchuk design-wise this is good enough to proceed! Let's ask CodeMirror about whether it's possible to get the "Unchanged" lines element attached to the left and not have to do the super-large padding hack. Doesn't have to block this though. Mind requesting a review when this is ready to take a look? |
VasylMarchuk
force-pushed
the
use-codemirror-on-admin-submissions-diff
branch
2 times, most recently
from
55cf80d
to
ab455e5
Compare
|
@rohitpaulk Just fixing the remaining broken tests right now and will request review once done! |
VasylMarchuk
force-pushed
the
use-codemirror-on-admin-submissions-diff
branch
from
ab455e5
to
68cb6d4
Compare
VasylMarchuk
force-pushed
the
use-codemirror-on-admin-submissions-diff
branch
from
09577a5
to
9fe2f7b
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: 5
🧹 Outside diff range and nitpick comments (15)
app/utils/blend-colors.ts (2)
4-4: LGTM: Clear function signature with flexible defaults.The function signature is well-defined with meaningful parameter names and sensible default values.
Consider adding explicit type annotations for the parameters to improve code readability and type safety:
export default function blendColors(fgColor: string = '#ffffff', opacity: number = 1, bgColor: string = '#ffffff'): string {
5-8: LGTM: Correct color conversion and blending logic.The color conversion and blending logic are implemented correctly. The use of
slice(0, 3)ensures that only RGB values are used in the blending process.To improve readability, consider breaking down the blending logic into smaller steps:
const blendedRgb = fgRgb.slice(0, 3).map((colFg, idx) => { const colBg = bgRgb[idx] || 255; return opacity * colFg + (1 - opacity) * colBg; }) as [number, number, number];This approach makes the blending formula more explicit and easier to understand.
app/helpers/diff-to-document.ts (3)
4-18: LGTM: Core logic is sound, with a minor suggestion for improvement.The core logic of the function correctly processes the diff string, categorizing lines into 'current' and 'original' arrays based on their prefixes. The handling of unchanged lines (adding to both arrays) is appropriate.
Consider adding a guard clause at the beginning of the function to handle empty or undefined input more explicitly:
if (!diff) { return { current: '', original: '' }; }This would make the function more robust against edge cases.
26-32: LGTM: Export and type declaration are correct, with a minor suggestion.The helper function is correctly exported, and the type declaration for the Ember Glint environment is properly implemented. This ensures type safety when using the helper in Ember templates.
For consistency with the function name, consider updating the interface property name to use camelCase:
export default interface Registry { 'diffToDocument': typeof diffToDocument; }This aligns with the function name and follows common JavaScript naming conventions.
1-32: Overall implementation aligns well with PR objectives.The
diffToDocumenthelper function is a well-implemented addition that supports the PR's goal of enhancing diff handling for the Admin Submissions Diff page. It provides a clean way to process diff strings into separate 'current' and 'original' documents, which should integrate seamlessly with the CodeMirror implementation mentioned in the PR summary.The function's logic is sound, with appropriate handling of added, removed, and unchanged lines. The type declarations ensure proper integration with the Ember Glint environment, enhancing type safety in the application.
While the implementation is solid, consider the minor suggestions provided in the previous comments to further improve robustness and consistency.
As you continue to develop this feature, consider the following:
- Ensure that this helper is used consistently across all diff-related components for maintainability.
- If performance becomes a concern with large diffs, consider implementing lazy loading or virtualization techniques in conjunction with this helper.
- Document the expected input format and output structure for other developers who might use this helper in the future.
app/components/course-admin/submissions-page/submission-details/diff-container.ts (2)
17-19: LGTM: Computed property for theme selection is well-implemented.The
codeMirrorThemecomputed property correctly determines the theme based on the dark mode state. It's concise and effective.Consider using an arrow function for slightly improved readability:
get codeMirrorTheme() { return this.darkMode.isEnabled ? codeCraftersDark : codeCraftersLight; }
Line range hint
1-28: Summary: Changes align well with PR objectives.The modifications to
DiffContainerComponentsuccessfully implement dark mode support for the CodeMirror-based diff view. The changes are well-structured, properly typed, and consistent with Ember.js and TypeScript best practices. These updates directly contribute to the PR's goal of enhancing the Admin Submissions Diff tab with improved theming capabilities.As the project continues to evolve, consider creating a dedicated theme service that could manage both light and dark themes across the application. This would centralize theme management and make it easier to extend or modify theming in the future.
tests/unit/utils/blend-colors-test.ts (1)
5-15: LGTM: Comprehensive test case with a suggestion for improvement.The test case is well-structured and covers various scenarios for the
blendColorsfunction, including edge cases. It checks for function existence, type, and multiple color blending scenarios with different opacities and background colors.Consider adding a test case for invalid input handling, such as:
assert.throws(() => blendColors('invalid-color', 0.5), /Invalid color format/);This would ensure the function properly handles error cases.
tests/integration/helpers/diff-to-document-test.ts (1)
21-31: LGTM with a minor suggestion: Add a comment explaining the importance of this edge caseThis test case effectively checks the robustness of the
diff-to-documenthelper when handling undefined input. It's a crucial edge case to test.Consider adding a brief comment explaining why testing this edge case is important. For example:
// This test ensures that the helper gracefully handles undefined input, // which could occur in real-world scenarios where diff data might be missing. test('it does not break if passed an undefined diff', async function (assert) { // ... (existing test code) ... });This comment would provide context for future developers about the importance of this test case.
tests/pages/course-admin/submissions-page.js (2)
10-10: Approved: Renaming tochangedFilesimproves clarity.The renaming from
expandableChunkstochangedFilesis a good change that better describes the collected elements. It aligns well with the PR objectives of focusing on changed files rather than expandable chunks.Consider updating the data attribute in the selector from
[data-test-changed-file]to[data-test-changed-files]to match the plural form of the property name. This would improve consistency:- changedFiles: collection('[data-test-changed-file]', { + changedFiles: collection('[data-test-changed-files]', {
Line range hint
1-58: Summary: Successful integration of CodeMirror for diff testingThe changes in this file successfully integrate CodeMirror into the page object for testing the Admin Submissions Diff tab. The modifications align well with the PR objectives, shifting focus from expandable chunks to changed files and incorporating CodeMirror for improved diff viewing.
Key points:
- The new import for the CodeMirror component is correctly added.
- Renaming
expandableChunkstochangedFilesimproves clarity.- The addition of the
codeMirrorproperty tochangedFilesenables interaction with CodeMirror instances in tests.These changes enhance the testing capabilities for the new CodeMirror-based diff view, supporting the overall goals of the pull request.
As you continue to implement CodeMirror throughout the application, consider creating a dedicated CodeMirror page object that can be reused across different test files. This would promote consistency and reduce duplication in your test code.
package.json (1)
170-170: New color conversion dependencies look appropriate.The addition of
hex-rgbandrgb-hexlibraries aligns with the PR objectives, particularly the improvements to CodeMirror themes and Dark Mode support. These libraries will facilitate color manipulation required for these features.Consider updating the project's documentation to mention the use of these libraries for color conversion, if not already done.
Also applies to: 183-183
app/components/code-mirror.ts (1)
314-316: LGTM! Consider updating documentation for new options.The changes to the
unifiedMergeViewconfiguration look good:
- Increasing the
collapseUnchangedmargin from 2 to 3 may improve readability.- Adding
highlightChanges: falsedisables change highlighting, which might be intentional for your use case.- Setting
syntaxHighlightDeletions: trueenables syntax highlighting for deleted code, potentially enhancing visibility.These adjustments seem to fine-tune the diff view behavior, likely improving the user experience.
Consider updating the component's documentation or README to reflect these new options, especially
highlightChangesandsyntaxHighlightDeletions, to ensure other developers are aware of these configuration possibilities.app/templates/demo/code-mirror.hbs (1)
298-298: Improved flexibility and accessibility of CodeMirror component.The changes to the CodeMirror component's class attribute enhance its usability and appearance:
- Replacing
h-72withmax-h-96allows for more flexible height, accommodating larger code snippets.- Adding
dark:outline-gray-700improves dark mode support, enhancing accessibility.- Changing the outline style from dashed to dotted is a minor visual improvement.
These updates are beneficial for the overall user experience.
Consider adding a minimum height (e.g.,
min-h-48) to ensure the component maintains a reasonable size for smaller code snippets:- class="block overflow-auto mt-4 max-h-96 {{if this.outline 'outline outline-dotted outline-gray-200 dark:outline-gray-700'}}" + class="block overflow-auto mt-4 min-h-48 max-h-96 {{if this.outline 'outline outline-dotted outline-gray-200 dark:outline-gray-700'}}"app/utils/code-mirror-themes.ts (1)
47-47: Use Tailwind color variables instead of hardcoded hex valuesAt lines 47 and 58, the
colorproperty is set to'#94a3b8'. For consistency and maintainability, consider usingtailwindColorsvariables instead of hardcoded hex values. This aligns with the rest of the code and helps keep color definitions consistent.Apply this diff to use Tailwind color variables:
- color: '#94a3b8', + color: tailwindColors.slate['400'],Also applies to: 58-58
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (7)
package-lock.jsonis excluded by!**/package-lock.jsonpublic/assets/images/codemirror/expand-diff-bottom-dark.svgis excluded by!**/*.svgpublic/assets/images/codemirror/expand-diff-bottom.svgis excluded by!**/*.svgpublic/assets/images/codemirror/expand-diff-middle-dark.svgis excluded by!**/*.svgpublic/assets/images/codemirror/expand-diff-middle.svgis excluded by!**/*.svgpublic/assets/images/codemirror/expand-diff-top-dark.svgis excluded by!**/*.svgpublic/assets/images/codemirror/expand-diff-top.svgis excluded by!**/*.svg
📒 Files selected for processing (15)
- app/components/code-mirror.ts (1 hunks)
- app/components/course-admin/submissions-page/submission-details/diff-container.hbs (1 hunks)
- app/components/course-admin/submissions-page/submission-details/diff-container.ts (2 hunks)
- app/helpers/diff-to-document.ts (1 hunks)
- app/styles/app.css (1 hunks)
- app/styles/pages/course-admin-submission-diff.scss (1 hunks)
- app/templates/demo/code-mirror.hbs (1 hunks)
- app/utils/blend-colors.ts (1 hunks)
- app/utils/code-mirror-themes.ts (1 hunks)
- package.json (2 hunks)
- tests/acceptance/course-admin/view-diffs-test.js (2 hunks)
- tests/integration/helpers/diff-to-document-test.ts (1 hunks)
- tests/pages/components/code-mirror.ts (2 hunks)
- tests/pages/course-admin/submissions-page.js (1 hunks)
- tests/unit/utils/blend-colors-test.ts (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/styles/pages/course-admin-submission-diff.scss
🧰 Additional context used
🔇 Additional comments (26)
app/utils/blend-colors.ts (3)
1-2: LGTM: Appropriate use of color conversion libraries.The imports of
hexRgbandrgbHexare well-chosen for handling color conversions in this utility function.
10-10: LGTM: Correct conversion and formatting of the result.The return statement correctly converts the blended RGB values back to a hex string and formats it with a leading '#'.
1-11: Overall assessment: Well-implemented color blending utility.The
blendColorsfunction is a well-implemented utility for color blending. It makes good use of external libraries for color conversions and implements the blending algorithm correctly. The function is concise and serves its purpose effectively.Minor suggestions for improvement:
- Add explicit type annotations to the function parameters.
- Consider breaking down the blending logic for improved readability.
These suggestions are optional and do not impact the functionality of the code.
app/helpers/diff-to-document.ts (2)
1-3: LGTM: Import and function declaration are well-structured.The import statement and function declaration are correctly implemented. The use of a default parameter for the
diffargument is a good practice to handle potential undefined inputs.
20-23: LGTM: Return statement is correctly implemented.The return statement properly formats the output object with 'current' and 'original' properties. The use of
join('\n')ensures that the original line structure is maintained in the output strings.app/components/course-admin/submissions-page/submission-details/diff-container.ts (2)
2-3: LGTM: Import statements are correct and necessary.The new import statements for the
servicedecorator,DarkModeServicetype, and CodeMirror themes are correctly added and essential for the new dark mode functionality.Also applies to: 5-5
15-15: LGTM: DarkModeService is correctly injected.The
darkModeservice is properly injected using the@servicedecorator and correctly typed asDarkModeService. This allows the component to access the dark mode state, which is essential for the new theme switching functionality.tests/unit/utils/blend-colors-test.ts (2)
1-2: LGTM: Imports are correct and appropriate.The imports for the
blendColorsfunction and QUnit testing utilities are correctly implemented and necessary for this test file.
4-4: LGTM: Module structure is well-defined.The QUnit module is correctly structured and named appropriately, following best practices for organizing unit tests.
tests/integration/helpers/diff-to-document-test.ts (4)
1-4: LGTM: Appropriate imports for Ember.js integration testsThe import statements are correct and include all necessary testing utilities for Ember.js integration tests.
6-8: LGTM: Proper module setup for integration testsThe module setup follows Ember.js testing best practices, correctly using
setupRenderingTestfor integration tests.
9-19: LGTM: Effective test for diff-to-document helperThis test case thoroughly checks the
diff-to-documenthelper's functionality by rendering a template and asserting the content of both the current and original documents. The use of regular expressions in assertions is appropriate for this scenario.
1-32: Overall: Well-structured and comprehensive integration testsThis file provides thorough and well-implemented integration tests for the
diff-to-documenthelper. It covers both the main functionality and an important edge case (handling undefined input). The testing strategy is effective, using appropriate Ember.js testing utilities and conventions.The only minor suggestion is to add a brief comment explaining the importance of the edge case test, which would enhance the file's documentation for future developers.
Great job on writing these tests!
tests/pages/course-admin/submissions-page.js (1)
4-4: LGTM: New import for CodeMirror component.The import statement for the
codeMirrorcomponent is correctly added and aligns with the PR objective of implementing CodeMirror for the Admin Submissions Diff tab.tests/pages/components/code-mirror.ts (5)
27-30: LGTM:changeGuttercollection added correctly.The
changeGuttercollection is well-defined and consistent with other gutter collections. This addition enables interaction with change indicators in the gutter, which is crucial for diff views in tests.
39-42: LGTM:collapsedLinesPlaceholderscollection added appropriately.The
collapsedLinesPlaceholderscollection is well-defined with necessarytextandclickactions. This addition enables testing of collapsed line functionality in diff views, which aligns with the PR objectives of improving diff rendering and interaction.
50-54: Verify the need forfillInaction inchangedLinescollection.The
changedLinescollection is well-defined and includestextandclickactions, which are appropriate for interacting with changed lines in a diff view. However, the inclusion of thefillInaction suggests that changed lines are editable.Could you please confirm if this is the intended behavior for diff views? Typically, diff views are read-only, so the
fillInaction might not be necessary unless there's a specific use case for editing changed lines during tests.
56-58: LGTM:deletedChunkscollection added correctly.The
deletedChunkscollection is well-defined with atextaction, which is appropriate for representing deleted content in a diff view. The absence of interactive actions likeclickis consistent with the expected behavior of deleted chunks in a diff.
Line range hint
26-58: Overall assessment: Comprehensive improvements to CodeMirror page object for diff testing.The additions to the
code-mirror.tspage object significantly enhance its capabilities for testing CodeMirror-based diff views. The new collections (changeGutter,collapsedLinesPlaceholders,changedLines, anddeletedChunks) provide a robust set of selectors and actions for interacting with diff-specific elements.These changes align well with the PR objectives of transitioning to CodeMirror for diff rendering and improving the testing framework. The new structure will allow for more precise and comprehensive testing of diff views, including interactions with change indicators, collapsed lines, and changed content.
One minor point to verify is the inclusion of the
fillInaction forchangedLines, as mentioned in a previous comment.Overall, these modifications to the page object will greatly facilitate the testing of the new CodeMirror implementation in the Admin Submissions Diff page.
app/styles/app.css (1)
9-9: LGTM! Verify the existence of the imported file.The addition of this import statement is consistent with the existing structure and aligns with the PR objectives for updating the Admin Submissions Diff page.
Let's verify the existence of the imported file:
✅ Verification successful
Verified: The imported SCSS file exists and contains the expected styles.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence of the imported SCSS file # Test: Check if the file exists if fd -t f "course-admin-submission-diff.scss" app/styles/pages; then echo "File exists" else echo "File does not exist" fi # Test: Display the content of the file if it exists fd -t f "course-admin-submission-diff.scss" app/styles/pages -x catLength of output: 309
package.json (1)
Line range hint
3-3: Version update looks appropriate.The version increment to "0.0.3" aligns with the significant updates mentioned in the PR objectives, particularly the changes to the Admin Submissions Diff page.
app/components/course-admin/submissions-page/submission-details/diff-container.hbs (1)
1-1: Confirm the necessity of...attributes.At line 1,
...attributesis used to pass down additional HTML attributes. Ensure that the parent component provides meaningful attributes to this element; otherwise,...attributesmay be unnecessary.tests/acceptance/course-admin/view-diffs-test.js (1)
7-51: Efficient Handling of Multiline Diff StringDefining
EXAMPLE_DIFFas an array and joining it with'\n'is an effective solution to prevent VSCode from stripping trailing spaces in blank lines. This approach ensures that the diff content remains accurate for the test cases.app/utils/code-mirror-themes.ts (3)
3-4: Imports are appropriately added for Tailwind colors and blendColors utilityThe imports at lines 3-4 for
tailwindColorsandblendColorsare correct and necessary for the new styling enhancements.
178-234: Ensure that theme ordering correctly overrides styles as intendedIn the
codeCraftersDarkexport (lines 178-234), themes are applied in the following order:
- Custom
EditorView.themewith dark mode styles.EditorView.theme(BASE_STYLE, { dark: true }).githubDark.Since the last theme in the array can override previous styles, please verify that the imported
githubDarktheme does not unintentionally override your custom styles inBASE_STYLEor the prior custom theme. If necessary, adjust the order or increase the specificity of your custom styles to ensure the desired styling is applied.
72-72: Confirm that 'Montserrat' font is available and appropriateThe
fontFamilyproperty at line 72 is set to'Montserrat, sans-serif'. Please verify that 'Montserrat' is loaded and available in the application, and that using it here aligns with the overall design guidelines. If 'Montserrat' is not guaranteed to be available, consider using a default font stack or ensuring the font is properly imported.Run the following script to check if 'Montserrat' font is being imported or referenced in the codebase:
✅ Verification successful
Montserrat font is properly imported and available
The 'Montserrat' font is successfully imported via
<link>tags intests/index.htmlandapp/index.html, and is appropriately referenced intailwind.config.jsandapp/utils/code-mirror-themes.ts.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify if 'Montserrat' font is imported or referenced. # Search for 'Montserrat' in CSS files. rg 'Montserrat' --type css # Search for font imports in JavaScript or TypeScript files. rg 'Montserrat' --type js --type tsLength of output: 193
Script:
#!/bin/bash # Description: Verify if 'Montserrat' font is imported via CSS or HTML. # Search for '@import' or 'font-face' declarations in CSS files. rg '@import.*Montserrat|font-face.*Montserrat' --type css # Search for '<link>' tags importing 'Montserrat' in HTML files. rg '<link[^>]*href="[^"]*Montserrat[^"]*"' --type html # Optionally, search for dynamic imports in JS/TS files. rg 'import.*Montserrat' --type js --type tsLength of output: 444
app/components/course-admin/submissions-page/submission-details/diff-container.hbs
Show resolved
Hide resolved
app/components/course-admin/submissions-page/submission-details/diff-container.hbs
Show resolved
Hide resolved
Related to #1231
Brief
This switches the
DiffContainercomponent (which appears on the Admin Submissions Diff page) from usingSyntaxHighlightedDiffto using CodeMirror.Details
SyntaxHighlightedDiffdiff-to-documenthelper for parsing diffs into document objects, with testsObject.assignAcceptance | course-admin | view-diffstests to test CodeMirror's collapsed lines placeholders instead of "expandable chunks", and combined them all into one for more efficient testing.codeMirrorpage objectsubmissionsPagepage object to usecodeMirrorpage objectblend-colorsfor blendingfgColorof a givenopacitywithbgColorhex-rgbandrgb-hexScreenshots
Checklist
[percy]in the message to trigger)Summary by CodeRabbit
Release Notes
New Features
CodeMirrorcomponent for displaying code diffs, improving interactivity and allowing for better handling of different code styles.Bug Fixes
Tests
diff-to-documenthelper to ensure accurate conversion and error handling.