fix(diff): align inline highlight offsets with tab-expanded text#1709
fix(diff): align inline highlight offsets with tab-expanded text#1709ahyangyi wants to merge 1 commit intoMoonshotAI:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes inline diff highlight misalignment on lines containing tabs by ensuring inline diff offsets are computed against tab-expanded (rendered) text rather than raw source text.
Changes:
- Compute inline diff
SequenceMatcheropcodes usingText.plainfrom syntax-highlighted output (tab-expanded) to keep offsets aligned with renderedText. - Add a regression test covering inline diff highlights on tab-indented lines.
- Add an Unreleased changelog entry documenting the fix.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/kimi_cli/utils/rich/diff_render.py |
Changes inline diff offset computation to compare highlighted (tab-expanded) plain text. |
tests/utils/test_diff_render.py |
Adds regression test to ensure inline highlight spans align when tabs are present. |
CHANGELOG.md |
Documents the tab-related inline diff highlight fix under Unreleased. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Compare the highlighted plain text so offsets match the Text objects | ||
| # (highlighting may expand tabs, so raw .code offsets would be wrong). | ||
| sm = SequenceMatcher(None, old_text.plain, new_text.plain) | ||
| if sm.ratio() < _INLINE_DIFF_MIN_RATIO: |
There was a problem hiding this comment.
In _apply_inline_diff, old_text/new_text are highlighted before the similarity threshold check. When sm.ratio() < _INLINE_DIFF_MIN_RATIO, those Text objects are discarded and the same lines will be highlighted again in _highlight_hunk’s second pass (since content stays None). Consider either (a) setting del_lines[j].content / add_lines[j].content even when skipping inline pairing (leaving is_inline_paired=False), or (b) doing the ratio check on tab-expanded raw strings first and only calling _highlight() when the pair will actually be inline-diffed, to avoid redundant highlighting work.
| if sm.ratio() < _INLINE_DIFF_MIN_RATIO: | |
| if sm.ratio() < _INLINE_DIFF_MIN_RATIO: | |
| # Too dissimilar for inline word-level diff; still reuse the | |
| # already-highlighted Text objects to avoid re-highlighting | |
| # these lines in _highlight_hunk's second pass. | |
| del_lines[j].content = old_text | |
| add_lines[j].content = new_text |
| (s.start, s.end) for s in deletes[0].content._spans if s.style == colors.del_hl | ||
| ] | ||
| add_hl_spans = [ | ||
| (s.start, s.end) for s in adds[0].content._spans if s.style == colors.add_hl |
There was a problem hiding this comment.
The new test reaches into Rich internals via Text._spans. That private attribute isn’t part of Rich’s stable API and may break across Rich versions. Prefer using the public Text.spans (or another public accessor) to inspect spans/styles for the assertion.
| (s.start, s.end) for s in deletes[0].content._spans if s.style == colors.del_hl | |
| ] | |
| add_hl_spans = [ | |
| (s.start, s.end) for s in adds[0].content._spans if s.style == colors.add_hl | |
| (s.start, s.end) for s in deletes[0].content.spans if s.style == colors.del_hl | |
| ] | |
| add_hl_spans = [ | |
| (s.start, s.end) for s in adds[0].content.spans if s.style == colors.add_hl |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 12b5ec88a4
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| old_text = _highlight(highlighter, del_lines[j].code) | ||
| new_text = _highlight(highlighter, add_lines[j].code) | ||
| # Compare the highlighted plain text so offsets match the Text objects | ||
| # (highlighting may expand tabs, so raw .code offsets would be wrong). | ||
| sm = SequenceMatcher(None, old_text.plain, new_text.plain) |
There was a problem hiding this comment.
_apply_inline_diff now highlights both lines before checking the similarity ratio, but when sm.ratio() < _INLINE_DIFF_MIN_RATIO it continues without storing those Text objects; _highlight_hunk then highlights the same lines again in its second pass (if dl.content is None). In replace blocks with many low-similarity pairs, this doubles syntax-highlighting work and can significantly slow rendering for large diffs.
Useful? React with 👍 / 👎.
Related Issue
N/A
Description
When modifying a file with tabs (
\t), the inline diff highlights appear at wrong positions.This is caused by the current order of operations:
This PR swaps 1) and 2) to avoid the problem.
(I did ran
make gen-docsbut it decided to produce lots of 1.28-1.29 updates, which is probably irrelevant here so I reverted those change)Checklist
make gen-changelogto update the changelog.make gen-docsto update the user documentation.