[8.16] [Security Solution] Fix incorrect changes highlighting in diff view (#205138) #205612
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…lastic#205138) **Resolves: elastic#202016 ## Summary This PR resolves an issue where the diff view incorrectly marked certain characters as changed (using bold font) in some cases. ## Root Cause The issue arises from a bug in the `diff` library (v5). The library is used to compute two-way diffs between strings (old field value and new field value), producing an array of change objects that is then used for rendering. Conditions for the bug: - `diff` v5 library is in use (fixed in v6 and above) and `DiffMethod.WORDS` is passed as a parameter to it. - The old field value contains a line with an addition separated by a space (see example below). - The next line contains some changes (additions, deletions, or updates). For example, for these input strings: ``` foo bar spring ``` ``` foo sprint ``` | You would expect to see this diff | But you actually see this | |----------|----------| | <img width="119" alt="expected" src="https://github.com/user-attachments/assets/c41b3dec-e578-4a12-8eb8-91fbb60d7247" /> | <img width="118" alt="actual" src="https://github.com/user-attachments/assets/f2a33fee-5de2-4291-876a-e7575ea07079" /> | **A more real-life example** <img width="1661" alt="more_real_life" src="https://github.com/user-attachments/assets/91ebfe93-81ad-45c8-8f9b-e173c2cf940b" /> ## Solution Switching to `DiffMethod.WORDS_WITH_SPACE` avoids this issue. Screenshot showing the difference between `DiffMethod.WORDS` and `DiffMethod.WORDS_WITH_SPACE`: <img width="675" alt="words_vs_words_with_space" src="https://github.com/user-attachments/assets/3c91e1d2-63fc-4fcd-a762-a905878bfc3a" /> ## Other changes - Removed `DiffMethod.TRIMMED_LINES` since it's now [deprecated](kpdecker/jsdiff#486) in the `diff` library and we are not using it anyways. - Stopped using the "zip" option since I believe it produces a less readable diff, especially for cases when there's a different number of lines in the original value vs updated value. <details> <summary><strong>Screenshots: with and without "zip" (click to expand)</strong></summary> <strong>With the "zip" option (how it was before)</strong> <img width="1918" alt="zip" src="https://github.com/user-attachments/assets/272ed849-47d6-4fef-8acc-ab1b22c9f42e" /> <strong>No "zip" (this branch)</strong> <img width="1919" alt="no_zip" src="https://github.com/user-attachments/assets/417303bf-9570-4ee1-98c5-8a78f59c7956" /> </details> ## Testing I thoroughly tested with `DiffMethod.WORDS_WITH_SPACE` across various inputs and scenarios, including: - Single-line and multi-line strings. - Numbers, arrays, and objects. - Additions, deletions, and updates at different positions (start, middle, and end) within and across lines. I also validated diffs against real prebuilt rules by installing an older Fleet package version and observed no issues. You can test by trying different input strings and settings in Storybook. **Run Storybook**: `yarn storybook security_solution`. https://github.com/user-attachments/assets/0440b73c-a4d7-40cf-9cee-e632146d292e You can notice that `ComparisonSide` stories are broken, but that's unrelated to these changes and needs to be handled separately. ## Compatibility with future upgrades There's an open [PR](elastic#202622) that will upgrade the `diff` library from v5 to v7. I verified the behavior of `DiffMethod.WORDS_WITH_SPACE` on v7 and found no differences compared to v5, so it should be safe to upgrade to v7 without any changes on our end. Work started on 23-Dec-2024. (cherry picked from commit 140c2e0) # Conflicts: # x-pack/plugins/security_solution/public/detection_engine/rule_management/components/rule_details/json_diff/diff_view.stories.tsx
dplumlee
approved these changes
Jan 6, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Backport
This will backport the following commits from
mainto8.16:Questions ?
Please refer to the Backport tool documentation
\n\n### Questions ?\nPlease refer to the [Backport tool\ndocumentation](https://github.com/sqren/backport)\n\n\n\nCo-authored-by: Nikita Indik "}}]}] BACKPORT-->