Programming exercises: Show whitespace changes in diff viewer
#9962
Conversation
github-actions
bot
added
the
client
magaupp
temporarily deployed
to
artemis-test2.artemis.cit.tum.de
GitHub Actions
Inactive
WalkthroughThe pull request modifies the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/main/webapp/app/shared/monaco-editor/monaco-editor.service.ts (2)
Line range hint
92-95: Consider enhancing the method documentation.The JSDoc comment should mention the whitespace handling behavior since it's a significant aspect of the diff viewer's functionality.
Consider updating the documentation like this:
/** * Creates a standalone diff editor (see {@link MonacoDiffEditorComponent}) with sensible default settings and inserts it into the given DOM element. + * The diff editor is configured to show whitespace changes to ensure accurate comparison of indentation and spacing. * @param domElement The DOM element to insert the editor into. */
Line range hint
92-104: Consider the performance impact on large diffs.While showing whitespace changes is valuable, it might affect the rendering performance of large diffs. The current configuration looks good, but consider monitoring the performance impact, especially in programming exercises with substantial whitespace changes.
Some suggestions:
- Consider adding telemetry to monitor diff rendering times
- If performance issues arise, we could add a toggle button to enable/disable whitespace diff visibility
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
src/main/webapp/app/shared/monaco-editor/monaco-editor.service.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/main/webapp/app/shared/monaco-editor/monaco-editor.service.ts (1)
🔇 Additional comments (1)
src/main/webapp/app/shared/monaco-editor/monaco-editor.service.ts (1)
103-103: LGTM! The change correctly implements whitespace diff visibility.
The addition of ignoreTrimWhitespace: false directly addresses the PR objective and will help solve the issue of incorrect diff counts in programming exercises.
Let's verify that this is the only instance where we need to configure whitespace handling:
✅ Verification successful
The search results show only two matches:
- The line we're reviewing in
monaco-editor.service.ts - A string manipulation in
short-answer-question-util.service.tswhich is unrelated as it's just text processing logic, not editor configuration
Let me verify if there are any other Monaco editor configurations that might be affected:
Let me check one final thing to ensure there are no other diff editor configurations:
Based on the comprehensive search results, I can now provide the final response:
✅ The whitespace configuration change is correctly isolated to the diff editor configuration
The change to ignoreTrimWhitespace: false in monaco-editor.service.ts is the only place where whitespace handling needs to be configured for the Monaco diff editor. This is verified by:
- The diff editor is only created in one place via
createStandaloneDiffEditor - No other whitespace trimming configurations exist in the codebase
- The configuration is properly scoped within the diff editor initialization
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other potential locations where whitespace trimming might be configured
rg -i "trimwhitespace|whitespace.*trim" --type ts
Length of output: 380
Script:
#!/bin/bash
# Search for Monaco editor configuration patterns
rg -i "monaco.*config|editor.*config" --type ts -A 5
Length of output: 2285
Script:
#!/bin/bash
# Search specifically for diff editor configurations
rg -i "diffeditor|diff.*editor" --type ts -A 3
Length of output: 25418
HawKhiem
added
deploy:artemis-test3
deploy:artemis-test2
and removed
deploy:artemis-test3
labels
HawKhiem
temporarily deployed
to
artemis-test2.artemis.cit.tum.de
GitHub Actions
Inactive
ItsaaaMeMario
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
There was a problem hiding this comment.
I am typically not interested in whitespace changes as an instructor, so the setting in the client was already correct. The numbers should then also take this setting into account, regardless of whether they are calculated on the server or client.
We could offer an option similar to Github "Files changed" tab in the future in case other instructors are interested in whitespace changes.
|
We'll open another PR for a whitespace option in diffs |
Checklist
General
Client
Changes affecting Programming Exercises
Motivation and Context
Whitespace-only changes are currently hidden in the diff viewer. This, for example, hides tabs vs spaces inconsistencies and causes inconsistencies with the displayed line counts.
Description
This PR disables the
ignoreTrimWhitespaceoption of the monaco DiffEditor.Closes #9928.
Steps for Testing
Testserver States
Note
These badges show the state of the test servers.
Green = Currently available, Red = Currently locked
Click on the badges to get to the test servers.
Review Progress
Code Review
Manual Tests
Test Coverage
Screenshots
Before
After
Summary by CodeRabbit