Merge dev into main - #1
Merged
Merged
Conversation
… case merges with score 120345 > threshold 74184, need to verify why VSCode doesn't merge
…e 5 diffs, VSCode produces 7 diffs
… - produces 5 diffs instead of VSCode's 7 diffs. This causes downstream merging in removeVeryShortText that VSCode avoids. BUG IS IN: src/char_level.c line 465 extend_diffs_to_entire_word function
… position discrepancies
…lumn offset issue
When merging overlapping diffs, we must update both start and end positions for both sequences. Previously only end positions were updated, causing incorrect character offsets in the merged diffs. This fixes many column position mismatches in character-level diffs. Reduces test mismatches from 37 to 18 out of 100 tests.
- Add UTF-8 character counting functions - Convert between character positions (from JS/VSCode) and byte offsets - Store column positions in characters, not bytes - Fixes mismatches in files with multi-byte UTF-8 characters (e.g., →) - Reduces mismatches from 64 to 18
- Convert byte offsets to character offsets when calculating columns - This fixes most UTF-8 related mismatches (18 -> 3 remaining) - The elements array stores bytes but VSCode stores characters - Need to count UTF-8 characters, not bytes, for column positions
- Created include/utf8_utils.h and src/utf8_utils.c - Moved utf8_strlen, utf8_char_to_byte_offset, utf8_byte_to_char_offset from sequence.c - All existing UTF-8 functions now in one place for better maintainability - Updated sequence.c to include utf8_utils.h - Build and tests passing
- Track where diffs are being merged in remove_very_short_text - Identified snowball effect bug: once a diff becomes large, everything merges - Need to fix scoring calculation to match VSCode behavior
- Clean up fprintf debug statements - Back to 42 mismatches (21%) - Identified but not yet fixed: snowball merging issue in remove_very_short_text
- Added utf8_decode_char() to convert UTF-8 bytes to Unicode code points - Modified CharSequence to store UTF-16 code units instead of UTF-8 bytes - This matches JavaScript's charCodeAt() which VSCode uses internally - For BMP characters (U+0000 to U+FFFF), code point equals UTF-16 code unit - Still have 1 known issue with inner change merging logic
- JavaScript uses UTF-16 code units for string indexing (string.length, charCodeAt) - Our C code was using UTF-8 byte/code point indexing - This caused massive position/column mismatches Fixed by: 1. Added utf8_to_utf16_length() and utf8_to_utf16() functions 2. Added utf16_pos_to_utf8_byte() for position conversion 3. Updated CharSequence creation to use UTF-16 code units throughout 4. Store non-BMP characters (e.g., emoji) as surrogate pairs (2 UTF-16 units) Results: Reduced mismatches from 200/200 to 42/200 tests!
- Replace custom UTF-8 helpers with utf8proc library - Properly handle UTF-16 surrogate pairs for JS parity - Fix column position calculations matching VSCode behavior - Reduced mismatches from 200 to 42
- Move build-vscode-diff.sh and test_diff_comparison.sh to scripts/ - Update test_diff_comparison.sh to use relative paths from script location - Update documentation references to new script locations - Clean up temporary backup files and test results
…fference with my cases, so need more observation) This fix doesn't make much difference from my observation, but seems valid since VSCode has it, so need more observation
![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.
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+803
to
+808
| // Language conversion: Count UTF-8 characters in elements array | ||
| // JavaScript Note: In JS, offset is directly a character index (automatic) | ||
| // C Note: We must count UTF-8 characters manually because elements stores bytes | ||
| int line_offset_chars = count_utf8_chars_in_byte_range(seq->elements, | ||
| seq->line_start_offsets[line_idx], | ||
| offset); |
There was a problem hiding this comment.
The new sequence builder stores seq->elements as UTF‑16 code units, but char_sequence_translate_offset still calls count_utf8_chars_in_byte_range to derive the column offset. That helper decodes each value as a UTF‑8 lead byte and advances by 2–4 when the low byte has the high-bit pattern, so BMP characters such as é cause the loop to skip subsequent code units and return a smaller column than VSCode would. As a result, offsets inside lines containing non‑ASCII BMP characters are mapped to the wrong columns. The column calculation should use offset - seq->line_start_offsets[line_idx] (one per UTF‑16 unit) or a helper that counts UTF‑16 units instead of interpreting the array as UTF‑8 bytes.
Useful? React with 👍 / 👎.
Contributor
|
@esmuellert I've opened a new pull request, #2, to work on those changes. Once the pull request is ready, I'll request review from you. |
The code was incorrectly treating UTF-16 code units as UTF-8 bytes when calculating column offsets in char_sequence_translate_offset(). Since seq->elements stores UTF-16 code units (as populated by write_utf8_as_utf16_units), and offset is already an index into UTF-16 code units, the column offset should simply be: offset - seq->line_start_offsets[line_idx] This fixes the P1 issue where BMP characters like 'é' would cause incorrect column mapping because count_utf8_chars_in_byte_range() was treating each UTF-16 code unit as a UTF-8 lead byte and advancing by 2-4 bytes when the high bit pattern matched, skipping subsequent code units. Also updated Makefile to include utf8_utils.c and -lutf8proc for all test targets so tests can build correctly. Co-authored-by: esmuellert <41042490+esmuellert@users.noreply.github.com>
Fix UTF-16 column offset calculation in char_sequence_translate_offset
esmuellert
added a commit
that referenced
this pull request
May 29, 2026
…files (#401) (#402) ## Summary Fixes #401 — inline layout flickered and reset the cursor when viewing staged newly-added files. Side-by-side was unaffected. ## Root cause `inline_view.show_single_file` allocated a fresh scratch buffer via `nvim_create_buf(false, true)` on every call for virtual files (revision-based content). Each git fs_event refresh tick (~500ms) re-invoked it, swapped a new bufnr into the modified window, and reset the cursor. The side-by-side equivalent (`load_virtual_file` in `side_by_side.lua:790`) already used `bufadd(virtual_file.create_url(...))` — which returns a stable bufnr keyed by `(git_root, revision, path)`. Inline simply missed the trick. This PR brings inline in line with the existing convention. ## Why only staged newly-added inline showed it | Path | Bufnr stability | |---|---| | Inline + virtual revision (`A`/`D` with rev) | **broken** — `nvim_create_buf` churns | | Inline + real file (`??`, unstaged) | OK — `bufadd(file_path)` reuses | | Side-by-side + any single-file virtual | OK — `load_virtual_file` already uses `bufadd(create_url)` | | Tracked-file diff (status `M`) | OK — `view.update` pins buffers | Status `??` in inline didn't flicker because it hit the real-file branch. `D` inline had the same latent flicker but rarely surfaced (viewing a deleted file while git is changing). Fixed as a side effect. ## Empirical confirmation Probe driving the explorer with simulated refresh ticks, observing `session.modified_bufnr` and cursor line: **Before:** ``` INITIAL: bufnr=10 cursor.line=5 REFRESH #1: bufnr=10 cursor.line=5 REFRESH #2: bufnr=12 cursor.line=1 ← new buf, cursor reset REFRESH #3: bufnr=12 cursor.line=1 REFRESH #4: bufnr=14 cursor.line=1 REFRESH #5: bufnr=16 cursor.line=1 ``` **After:** ``` INITIAL: bufnr=6 cursor.line=5 REFRESH #1: bufnr=6 cursor.line=5 REFRESH #2: bufnr=6 cursor.line=5 REFRESH #3: bufnr=6 cursor.line=5 REFRESH #4: bufnr=6 cursor.line=5 REFRESH #5: bufnr=6 cursor.line=5 ``` ## Changes ### `lua/codediff/ui/view/inline_view.lua` Replace the `nvim_create_buf` virtual-file branch with `bufadd(virtual_file.create_url(opts.git_root, opts.revision, opts.rel_path or file_path))` + `bufload`. Mirrors `side_by_side.load_virtual_file` exactly. `BufReadCmd` in `core/virtual_file.lua` handles content loading and intentionally avoids setting `filetype` to prevent LSP attach crashes on the `codediff://` URI scheme. ### `tests/ui/view/inline_explorer_spec.lua` +2 tests: - Test 5: Repeated `show_single_file` for a real file keeps `modified_bufnr` stable. - Test 6: Repeated `show_single_file` for a staged virtual file (`:0`) keeps `modified_bufnr` stable. Direct regression test for #401. ## Testing All 19 regression-relevant spec files pass (172 tests, 0 failures), including: - `inline_explorer_spec.lua` (6) — includes new tests - `inline_standalone_spec.lua` (14) - `inline_history_spec.lua` (4) - `inline_interaction_spec.lua` (12) - `layout_toggle_spec.lua` (13) - `conflict_dedup_spec.lua` (4) - `view_spec.lua` (14) - `explorer_staging_spec.lua` (3) - `virtual_file_lsp_spec.lua` (2) - + others ## Benefits - Stops user-visible flicker and cursor reset reported in #401 - Mirrors the existing `side_by_side.load_virtual_file` convention — no new patterns introduced - Minimal diff (~9 LoC change + tests) — easy to review - No public API changes - Fixes the latent same-class flicker in deleted-file inline view ## Follow-up (not in this PR) `on_file_select` still has no dedup for the status `A`/`??`/`D` early-return branches in either layout. With this fix, that's a wasted-CPU issue (calls to `inline.clear`, `auto_refresh.disable`, `lifecycle.update_*`, `keymaps.setup_all_keymaps`, `layout.arrange` on every ~500ms tick) but no longer a visible bug, since bufnr is now stable. The recurring "missing dedup branch" bug class (#308 staged-rename, #320 conflict, #401 staged-add) suggests `on_file_select` should be refactored to a `resolve_view → render` pipeline with a single dedup site shared across layouts. Worth a follow-up issue to capture the technical debt.
Copilot AI
added a commit
that referenced
this pull request
Jul 29, 2026
…lows Addresses code scanning alerts #1-#5 (actions/missing-workflow-permissions). Adds `permissions: contents: read` at workflow level to: - .github/workflows/build-and-test.yml (alert #1) - .github/workflows/pr.yml (alerts #2, #3, #4) - .github/workflows/regression-check.yml (alert #5) Co-authored-by: yanuoma <220247883+yanuoma@users.noreply.github.com>
esmuellert
pushed a commit
that referenced
this pull request
Jul 29, 2026
…lows Addresses code scanning alerts #1-#5 (actions/missing-workflow-permissions). Adds `permissions: contents: read` at workflow level to: - .github/workflows/build-and-test.yml (alert #1) - .github/workflows/pr.yml (alerts #2, #3, #4) - .github/workflows/regression-check.yml (alert #5) Co-authored-by: yanuoma <220247883+yanuoma@users.noreply.github.com>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
No description provided.