fix(win): add EOL padding to compensate for path_shorten concealing (…#2613
Open
fix(win): add EOL padding to compensate for path_shorten concealing (…#2613
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaced extmark/conceal-based path shortening with overlay virtual-text rendering. PathShortener now stores per-window WinData, accepts formatting options, supports filename/dirname-first modes, ANSI stripping, and applies virt_text overlays per line instead of extmarks; attach/detach signatures updated accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant FzfWin
participant PathShortener
participant NeovimAPI
participant Buffer
FzfWin->>PathShortener: attach(winid, bufnr, shorten_len, fmt_opts)
PathShortener->>NeovimAPI: subscribe to win/line events (on_win/on_line)
NeovimAPI->>PathShortener: on_win(winid, topline)
PathShortener->>PathShortener: store WinData (bufnr, topline, fmt_opts)
NeovimAPI->>PathShortener: on_line(buf, row, text)
PathShortener->>PathShortener: compute shortened segments, widths, hl spans
PathShortener->>Buffer: apply virt_text overlay (virt_text, hl_mode="combine", virt_text_pos="overlay")
Buffer-->>NeovimAPI: render overlayed line
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lua/fzf-lua/win.lua (1)
108-126:⚠️ Potential issue | 🟠 MajorGuard the 0.11-only string-index APIs with a version check.
Lines 110 and 125 use
vim.str_utfindex(component, "utf-32")andvim.str_byteindex(component, "utf-32", keep_chars, false). These function signatures with the "utf-32" parameter are 0.11+. The repository explicitly supports Neovim 0.9+ (per README.md) and maintains version gates throughout the codebase (seeutils.__HAS_NVIM_011). The fallback logic (or component_len,or keep_chars) only helps if the function returns nil—it won't prevent a runtime error if the function signature is incompatible. Wrap this code block with anif utils.__HAS_NVIM_011 then ... else ... endguard and provide a 0.10-compatible fallback path for the character-length calculation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lua/fzf-lua/win.lua` around lines 108 - 126, Wrap the current 0.11-only calls in a version guard using utils.__HAS_NVIM_011: if true, keep using vim.str_utfindex(...) and vim.str_byteindex(...) as written (assigning component_charlen and keep_bytes); else compute character count with vim.fn.strchars(component) (fallback for component_charlen) and compute keep_bytes by taking the character-based substring via vim.fn.strcharpart(component, 0, keep_chars) and using its byte length (`#substring`) as the byte offset (assign to keep_bytes); preserve the existing bounds/fallback logic (component_len, keep_chars) and special-dot handling (DOT_BYTE, shorten_len) inside the guarded block so behavior is identical on both branches.
🧹 Nitpick comments (1)
tests/win_spec.lua (1)
417-561: Add one rendered regression for the extmark application path.These checks validate
_compute_line()well, but the original bug was in the rendered width afterconcealplus inlinevirt_text. One screen-level assertion here would catch Neovim rendering regressions that the pure-function suite cannot.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/win_spec.lua` around lines 417 - 561, Add a new integration test that exercises the full extmark application path rather than only the pure _compute_line result: call win._PathShortener._compute_line(...) to get specs, create a real buffer/line and apply each spec with vim.api.nvim_buf_set_extmark (using the conceal and virt_text/extmark fields produced), redraw the UI (or use the test screen helper) and assert the rendered screen columns/widths match the expected padding.width and padding.col from parse_specs; reference PS._compute_line, parse_specs and the extmark application via nvim_buf_set_extmark to locate where to hook this check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lua/fzf-lua/win.lua`:
- Around line 1526-1527: Add a test-only field to the module's type so assigning
M._PathShortener = PathShortener is type-safe: update the module annotation for
M (typed as fzf-lua.win.api) to include an optional _PathShortener field (e.g.,
by using an intersection or augmented type like fzf-lua.win.api & {
_PathShortener: typeof(PathShortener) } or marking it optional) so the
`_PathShortener` export is declared on M's type while keeping it clearly
test-only.
---
Outside diff comments:
In `@lua/fzf-lua/win.lua`:
- Around line 108-126: Wrap the current 0.11-only calls in a version guard using
utils.__HAS_NVIM_011: if true, keep using vim.str_utfindex(...) and
vim.str_byteindex(...) as written (assigning component_charlen and keep_bytes);
else compute character count with vim.fn.strchars(component) (fallback for
component_charlen) and compute keep_bytes by taking the character-based
substring via vim.fn.strcharpart(component, 0, keep_chars) and using its byte
length (`#substring`) as the byte offset (assign to keep_bytes); preserve the
existing bounds/fallback logic (component_len, keep_chars) and special-dot
handling (DOT_BYTE, shorten_len) inside the guarded block so behavior is
identical on both branches.
---
Nitpick comments:
In `@tests/win_spec.lua`:
- Around line 417-561: Add a new integration test that exercises the full
extmark application path rather than only the pure _compute_line result: call
win._PathShortener._compute_line(...) to get specs, create a real buffer/line
and apply each spec with vim.api.nvim_buf_set_extmark (using the conceal and
virt_text/extmark fields produced), redraw the UI (or use the test screen
helper) and assert the rendered screen columns/widths match the expected
padding.width and padding.col from parse_specs; reference PS._compute_line,
parse_specs and the extmark application via nvim_buf_set_extmark to locate where
to hook this check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3badb89d-ad93-49b6-add1-b4e991729e4b
📒 Files selected for processing (2)
lua/fzf-lua/win.luatests/win_spec.lua
ibhagwan
force-pushed
the
fix/path_shorten_padding
branch
from
5f43b39 to
1663a9e
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lua/fzf-lua/win.lua (1)
65-71:⚠️ Potential issue | 🟠 MajorHandle UTF-8-leading paths in the no-icon fallback.
Line 68 uses
%w, which is ASCII-only in Lua patterns and does not match UTF-8 multibyte characters. A line like"> こんにちは/longdir/file.lua"will havepath_startset to the/instead of the first CJK byte, so the first directory component is never concealed and the padding width calculation becomes incorrect for exactly the multibyte case this patch is trying to fix.The test at
tests/win_spec.lua:469covers multibyte paths but only for paths starting with ASCII (e.g.,"dこんにちは"), so the issue is not caught.Suggested fix
- local first_path_char = line:find("[%w/~%.]") + local first_path_char = line:find("[%w/~%.]") + if not first_path_char then + -- `%w` does not match UTF-8 letters, so fall back to the first + -- non-prefix byte for paths whose first component starts non-ASCII. + first_path_char = line:find("[^%s>]") + end if first_path_char then path_start = first_path_char end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lua/fzf-lua/win.lua` around lines 65 - 71, The current detection uses line:find("[%w/~%.]") which is ASCII-only and misses UTF-8 multibyte path starts; replace this with a UTF-8 aware scan: use utf8.offset to iterate characters from the line start, skip leading whitespace and any pointer characters, and stop when you hit an ASCII path char (match '/', '~', '.', drive-letter pattern or ASCII alnum) or any UTF-8 multibyte character (byte >= 0x80) — set path_start to that character's byte index. Update the code that computes first_path_char (the place currently using line:find("[%w/~%.]")) to use utf8.offset/utf8.byte in a small loop so CJK and other multibyte names are treated as path starts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@lua/fzf-lua/win.lua`:
- Around line 65-71: The current detection uses line:find("[%w/~%.]") which is
ASCII-only and misses UTF-8 multibyte path starts; replace this with a UTF-8
aware scan: use utf8.offset to iterate characters from the line start, skip
leading whitespace and any pointer characters, and stop when you hit an ASCII
path char (match '/', '~', '.', drive-letter pattern or ASCII alnum) or any
UTF-8 multibyte character (byte >= 0x80) — set path_start to that character's
byte index. Update the code that computes first_path_char (the place currently
using line:find("[%w/~%.]")) to use utf8.offset/utf8.byte in a small loop so CJK
and other multibyte names are treated as path starts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: efe38e00-da10-4ec9-a199-e728e42f8889
📒 Files selected for processing (2)
lua/fzf-lua/win.luatests/win_spec.lua
ibhagwan
force-pushed
the
fix/path_shorten_padding
branch
from
1663a9e to
a49c583
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lua/fzf-lua/win.lua (2)
113-145:⚠️ Potential issue | 🟡 MinorDon't shorten already-minimal hidden components.
With the current Line 116 check,
.a/.zstill shortens to.whenshorten_len = 1, because concealment is decided fromcomponent_charlen > shorten_leninstead of the final dot-awarekeep_chars. Computekeep_charsfirst, then only conceal when the component is longer than that adjusted limit.💡 Suggested fix
- if component_charlen > shorten_len then - -- Handle special case: component starts with '.' (hidden files/dirs) - local keep_chars = shorten_len - if string.byte(component, 1) == DOT_BYTE and component_charlen > shorten_len + 1 then - -- Keep the dot plus shorten_len characters - keep_chars = shorten_len + 1 - end - - -- Bounds check to prevent errors - keep_chars = math.min(keep_chars, component_charlen) + local keep_chars = shorten_len + if string.byte(component, 1) == DOT_BYTE then + keep_chars = keep_chars + 1 + end + keep_chars = math.min(keep_chars, component_charlen) + + if component_charlen > keep_chars then🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lua/fzf-lua/win.lua` around lines 113 - 145, Compute keep_chars (respecting DOT_BYTE and the hidden-file special case) before deciding to conceal and replace the outer condition that uses component_charlen > shorten_len with component_charlen > keep_chars; specifically, always calculate keep_chars (using component, shorten_len, DOT_BYTE and the bounds clamp), then only enter the concealment block when component_charlen > keep_chars so `.a`/`.z` won't be shortened to `.`; keep existing calculations for keep_bytes, conceal_start/end, and total_concealed_width unchanged.
108-126:⚠️ Potential issue | 🔴 CriticalGuard the Neovim 0.11-only UTF helper overloads with version-gating.
Lines 110 and 125 use the
encodingparameter forvim.str_utfindex()andvim.str_byteindex(), which was introduced in Neovim 0.11. Before 0.11, these functions had different signatures and returned tuples instead of single values. Since fzf-lua documents support for Neovim >= 0.9, thepath_shortenfeature will break on Neovim 0.9/0.10 installs. The present fallbacks (or component_lenandor keep_chars) are insufficient because they only catchnilreturns, not the malformed results from calling with an incompatible signature. Wrap lines 108–126 with a version check (e.g.,if vim.fn.has("nvim-0.11") == 1 then) and provide a fallback implementation for older versions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lua/fzf-lua/win.lua` around lines 108 - 126, The code calls the Neovim 0.11 overloads vim.str_utfindex(..., "utf-32") and vim.str_byteindex(..., "utf-32") (affecting variables component_charlen and keep_bytes), which will break on older Neovim; wrap the block that computes component_charlen/keep_chars/keep_bytes with a version check (if vim.fn.has("nvim-0.11") == 1 then ...) and keep the current logic inside that branch, and in the else branch implement the fallback using the older APIs (e.g., component_charlen = vim.fn.strchars(component) or utf8.len(component) and compute byte offsets with vim.fn.byteidx(component, keep_chars) or equivalent) so component_charlen, keep_chars and keep_bytes are always valid on Neovim >=0.9 and <0.11.
🧹 Nitpick comments (1)
tests/win_spec.lua (1)
417-561: Add one rendered-width regression test.These cases lock down
_compute_line, but the bug surface here is the actual screen width after_apply_linesets ephemeral extmarks and inline virtual text. One screen-level picker test withpath_shortenenabled would protect the original alignment regression.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/win_spec.lua` around lines 417 - 561, Add a screen-level regression test that exercises path_shorten end-to-end: create a picker window with path_shorten enabled, feed a line requiring shortening, call PathShortener._compute_line then win._apply_line (or trigger the codepath that does both), and assert the final screen cell alignment/virtual-text width matches the expected rendered display width (not just extmark specs) to catch alignment regressions; reference PathShortener._compute_line and win._apply_line and verify using the same parsing/assert helpers (e.g., parse_specs or screen comparisons) as other tests so the test fails if ephemeral extmarks/inline virtual text produce incorrect screen width.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@lua/fzf-lua/win.lua`:
- Around line 113-145: Compute keep_chars (respecting DOT_BYTE and the
hidden-file special case) before deciding to conceal and replace the outer
condition that uses component_charlen > shorten_len with component_charlen >
keep_chars; specifically, always calculate keep_chars (using component,
shorten_len, DOT_BYTE and the bounds clamp), then only enter the concealment
block when component_charlen > keep_chars so `.a`/`.z` won't be shortened to
`.`; keep existing calculations for keep_bytes, conceal_start/end, and
total_concealed_width unchanged.
- Around line 108-126: The code calls the Neovim 0.11 overloads
vim.str_utfindex(..., "utf-32") and vim.str_byteindex(..., "utf-32") (affecting
variables component_charlen and keep_bytes), which will break on older Neovim;
wrap the block that computes component_charlen/keep_chars/keep_bytes with a
version check (if vim.fn.has("nvim-0.11") == 1 then ...) and keep the current
logic inside that branch, and in the else branch implement the fallback using
the older APIs (e.g., component_charlen = vim.fn.strchars(component) or
utf8.len(component) and compute byte offsets with vim.fn.byteidx(component,
keep_chars) or equivalent) so component_charlen, keep_chars and keep_bytes are
always valid on Neovim >=0.9 and <0.11.
---
Nitpick comments:
In `@tests/win_spec.lua`:
- Around line 417-561: Add a screen-level regression test that exercises
path_shorten end-to-end: create a picker window with path_shorten enabled, feed
a line requiring shortening, call PathShortener._compute_line then
win._apply_line (or trigger the codepath that does both), and assert the final
screen cell alignment/virtual-text width matches the expected rendered display
width (not just extmark specs) to catch alignment regressions; reference
PathShortener._compute_line and win._apply_line and verify using the same
parsing/assert helpers (e.g., parse_specs or screen comparisons) as other tests
so the test fails if ephemeral extmarks/inline virtual text produce incorrect
screen width.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0200691f-8369-41f3-9a19-b081d6b9900c
📒 Files selected for processing (2)
lua/fzf-lua/win.luatests/win_spec.lua
ibhagwan
force-pushed
the
fix/path_shorten_padding
branch
from
a49c583 to
91f48a6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lua/fzf-lua/win.lua`:
- Around line 164-178: The code calls vim.str_utfindex and vim.str_byteindex
with the newer (0.11+) signature causing runtime errors on Neovim 0.9–0.10;
guard these calls with the version flag utils.__HAS_NVIM_011 and branch: when
utils.__HAS_NVIM_011 use vim.str_utfindex(component, "utf-32") and
vim.str_byteindex(component, "utf-32", keep_chars, false) as currently written,
otherwise call the older APIs vim.str_utfindex(component) to get
component_charlen and vim.str_byteindex(component, keep_chars) (or equivalent
older signature) to compute keep_bytes, preserving the existing fallbacks
(component_charlen or `#component`, keep_bytes or keep_chars) and using the same
variables (component_charlen, keep_chars, keep_bytes, shorten_len).
Comment on lines
+164
to
+178
| -- Count UTF-8 characters | ||
| local _, component_charlen = vim.str_utfindex(component, "utf-32") | ||
| component_charlen = component_charlen or component_len -- fallback to byte length | ||
| component_charlen = component_charlen or #component | ||
|
|
||
| -- Only conceal if the component has more characters than shorten_len | ||
| if component_charlen > shorten_len then | ||
| -- Handle special case: component starts with '.' (hidden files/dirs) | ||
| local keep_chars = shorten_len | ||
| if string.byte(component, 1) == DOT_BYTE and component_charlen > shorten_len + 1 then | ||
| -- Keep the dot plus shorten_len characters | ||
| keep_chars = shorten_len + 1 | ||
| end | ||
|
|
||
| -- Bounds check to prevent errors | ||
| keep_chars = math.min(keep_chars, component_charlen) | ||
|
|
||
| -- Convert character count to byte offset using vim.str_byteindex | ||
| -- Convert character count to byte offset | ||
| local keep_bytes = vim.str_byteindex(component, "utf-32", keep_chars, false) | ||
| keep_bytes = keep_bytes or keep_chars -- fallback to character count (ASCII approximation) | ||
|
|
||
| -- Calculate 0-indexed byte positions in the full line for extmark | ||
| -- path_start is 1-indexed, component_start is 1-indexed within path_portion | ||
| local line_offset = path_start - 1 + component_start - 1 | ||
| local conceal_start = line_offset + keep_bytes | ||
| local conceal_end = line_offset + component_len -- end of component (before separator) | ||
|
|
||
| if conceal_end > conceal_start then | ||
| pcall(api.nvim_buf_set_extmark, buf, PathShortener._ns, row, conceal_start, { | ||
| end_col = conceal_end, | ||
| conceal = "", | ||
| ephemeral = true, | ||
| }) | ||
| end | ||
| keep_bytes = keep_bytes or keep_chars |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check minimum Neovim version requirement and any existing version guards for these APIs
echo "=== Checking minimum Neovim version requirement ==="
rg -n -C2 'minimum.*nvim|nvim.*minimum|required.*version|0\.10|0\.9' README.md doc/*.txt 2>/dev/null | head -30
echo -e "\n=== Checking for existing version guards on str_utfindex/str_byteindex ==="
rg -n 'str_utfindex|str_byteindex' --type lua
echo -e "\n=== Checking HAS_NVIM version flags usage ==="
rg -n '__HAS_NVIM' lua/fzf-lua/utils.lua | head -20Repository: ibhagwan/fzf-lua
Length of output: 3396
🏁 Script executed:
#!/bin/bash
# Check context around lines 164-178 in win.lua
echo "=== Full context of lines 150-185 in win.lua ==="
sed -n '150,185p' lua/fzf-lua/win.lua | cat -n
echo -e "\n=== Checking for version guards around line 164-178 ==="
sed -n '140,190p' lua/fzf-lua/win.lua | grep -n -E '__HAS_NVIM|if.*nvim|utils\.__'Repository: ibhagwan/fzf-lua
Length of output: 1782
🏁 Script executed:
#!/bin/bash
# Get broader context to find function definition and any wrapping
echo "=== Looking for function definition and broader context (lines 100-200) ==="
sed -n '100,200p' lua/fzf-lua/win.lua | cat -n | head -120
echo -e "\n=== Searching for pcall or try-catch patterns around this area ==="
sed -n '155,185p' lua/fzf-lua/win.luaRepository: ibhagwan/fzf-lua
Length of output: 6192
Add version guard for vim.str_utfindex and vim.str_byteindex API compatibility.
The code uses the Neovim 0.11+ API signatures for vim.str_utfindex(s, encoding, idx, strict) and vim.str_byteindex(s, encoding, idx, strict), but fzf-lua supports Neovim >= 0.9.0. On Neovim 0.9–0.10, these functions have different signatures (vim.str_utfindex(s, idx) where idx is a number), and passing "utf-32" as the second argument will cause a runtime error. The fallbacks (component_charlen or #component``, keep_bytes or keep_chars) only handle `nil` returns and won't catch API errors from mismatched signatures.
Wrap this section with a version check using utils.__HAS_NVIM_011 to use the correct API for each version.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lua/fzf-lua/win.lua` around lines 164 - 178, The code calls vim.str_utfindex
and vim.str_byteindex with the newer (0.11+) signature causing runtime errors on
Neovim 0.9–0.10; guard these calls with the version flag utils.__HAS_NVIM_011
and branch: when utils.__HAS_NVIM_011 use vim.str_utfindex(component, "utf-32")
and vim.str_byteindex(component, "utf-32", keep_chars, false) as currently
written, otherwise call the older APIs vim.str_utfindex(component) to get
component_charlen and vim.str_byteindex(component, keep_chars) (or equivalent
older signature) to compute keep_bytes, preserving the existing fallbacks
(component_charlen or `#component`, keep_bytes or keep_chars) and using the same
variables (component_charlen, keep_chars, keep_bytes, shorten_len).
91f48a6 to
96e18a6
Compare
Owner
Author
|
@phanen, tried all kinds of approaches:
Not sure if this has a solution, ideas welcome |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
lua/fzf-lua/win.lua (1)
182-193:⚠️ Potential issue | 🟠 MajorGuard UTF index API calls by Neovim version.
vim.str_utfindex/vim.str_byteindexare called with 0.11-style parameters without a version branch. This can break on older supported Neovim versions.#!/bin/bash set -euo pipefail echo "== Check documented Neovim support range ==" rg -n -C2 'Neovim|nvim|0\.9|0\.10|0\.11|minim' README.md doc/*.txt 2>/dev/null || true echo echo "== Locate UTF index API usage in win.lua ==" rg -n -C2 'str_utfindex\(|str_byteindex\(' lua/fzf-lua/win.lua echo echo "== Check for nearby version guards in the changed block ==" sed -n '170,240p' lua/fzf-lua/win.lua | rg -n '__HAS_NVIM_011|str_utfindex|str_byteindex'Guard pattern to apply
- local _, component_charlen = vim.str_utfindex(component, "utf-32") - component_charlen = component_charlen or `#component` + local component_charlen + if utils.__HAS_NVIM_011 then + component_charlen = vim.str_utfindex(component, "utf-32") + else + component_charlen = vim.str_utfindex(component) + end + component_charlen = component_charlen or `#component` ... - local keep_bytes = vim.str_byteindex(component, "utf-32", keep_chars, false) + local keep_bytes + if utils.__HAS_NVIM_011 then + keep_bytes = vim.str_byteindex(component, "utf-32", keep_chars, false) + else + keep_bytes = vim.str_byteindex(component, keep_chars) + end keep_bytes = keep_bytes or keep_charsAlso applies to: 219-228
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lua/fzf-lua/win.lua` around lines 182 - 193, The calls to vim.str_utfindex and vim.str_byteindex must be guarded by a Neovim-version check (use existing __HAS_NVIM_011 or vim.fn.has("nvim-0.11")) and call the appropriate API signature: when on >=0.11 call vim.str_utfindex(component, "utf-32") and vim.str_byteindex(component, "utf-32", keep_chars, false) as currently written, otherwise call the older signatures (without the extra args) and adapt to their return values (handle nils/defaults as you already do with component_charlen and keep_bytes). Update both occurrences (the block using component_charlen/keep_chars/keep_bytes and the similar block around 219-228) to branch on the version check and preserve DOT_BYTE, shorten_len and math.min logic unchanged.
🧹 Nitpick comments (1)
lua/fzf-lua/win.lua (1)
255-281: Remove unusedvirt_textconstruction block.The table built in Line 258-281 is never used; rendering uses
virt_text_str/virt_text_hlfrom Line 285 onward. This dead block adds overhead and confusion.Cleanup sketch
- local virt_text = {} - - -- Build virt_text with shortened path and explicit highlights - if has_fmt_hls then - ... - else - virt_text[`#virt_text` + 1] = { shortened_path } - end - -- Build virt_text with shortened path and explicit highlights -- Use a single virt_text entry to ensure proper overlay behavior local virt_text_str local virt_text_hlAlso applies to: 285-307
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lua/fzf-lua/win.lua` around lines 255 - 281, Remove the unused virt_text construction: the block that builds the local table virt_text (the if has_fmt_hls / filename_first_prefix / dirname_first branches that append entries using win_data.fmt_opts.hl_file, win_data.fmt_opts.hl_dir, shortened_path and shortened_last_sep_byte) is dead because rendering uses virt_text_str and virt_text_hl later; delete that entire block and any vestigial local virt_text declaration, leaving the code that computes virt_text_str/virt_text_hl untouched (ensure references to has_fmt_hls, filename_first_prefix, shortened_path, shortened_last_sep_byte, win_data.fmt_opts.hl_file and hl_dir are removed only if they are not used elsewhere in the function).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lua/fzf-lua/win.lua`:
- Around line 9-12: Add a missing type declaration for
fzf-lua.PathShortener.WinData so the PathShortener._wins field and its usages
type-check; specifically declare the WinData shape (the table fields referenced
later) near the top with the expected fields used by PathShortener (e.g., window
id, buffer id, and any cached short path or namespace fields) and attach it to
the PathShortener namespace (i.e., define fzf-lua.PathShortener.WinData before
PathShortener._wins is declared); update the declaration for PathShortener to
reference that newly defined WinData so the downstream references in methods
like those touching PathShortener._wins no longer produce lint/type warnings.
- Around line 251-253: original_width is currently computed as
fn.strdisplaywidth(utils.strip_ansi_coloring(original_path_portion)) +
filename_first_prefix_width which double-counts the filename prefix because
original_path_portion already contains that prefix; remove the extra addition so
original_width is just
fn.strdisplaywidth(utils.strip_ansi_coloring(original_path_portion)), keep
shortened_width calculation unchanged, and recompute padding = original_width -
shortened_width to fix the filename-first padding overcount (adjust any tests or
downstream uses of original_width if they assumed the previous inflated value).
---
Duplicate comments:
In `@lua/fzf-lua/win.lua`:
- Around line 182-193: The calls to vim.str_utfindex and vim.str_byteindex must
be guarded by a Neovim-version check (use existing __HAS_NVIM_011 or
vim.fn.has("nvim-0.11")) and call the appropriate API signature: when on >=0.11
call vim.str_utfindex(component, "utf-32") and vim.str_byteindex(component,
"utf-32", keep_chars, false) as currently written, otherwise call the older
signatures (without the extra args) and adapt to their return values (handle
nils/defaults as you already do with component_charlen and keep_bytes). Update
both occurrences (the block using component_charlen/keep_chars/keep_bytes and
the similar block around 219-228) to branch on the version check and preserve
DOT_BYTE, shorten_len and math.min logic unchanged.
---
Nitpick comments:
In `@lua/fzf-lua/win.lua`:
- Around line 255-281: Remove the unused virt_text construction: the block that
builds the local table virt_text (the if has_fmt_hls / filename_first_prefix /
dirname_first branches that append entries using win_data.fmt_opts.hl_file,
win_data.fmt_opts.hl_dir, shortened_path and shortened_last_sep_byte) is dead
because rendering uses virt_text_str and virt_text_hl later; delete that entire
block and any vestigial local virt_text declaration, leaving the code that
computes virt_text_str/virt_text_hl untouched (ensure references to has_fmt_hls,
filename_first_prefix, shortened_path, shortened_last_sep_byte,
win_data.fmt_opts.hl_file and hl_dir are removed only if they are not used
elsewhere in the function).
Comment on lines
9
to
12
| ---@class fzf-lua.PathShortener | ||
| ---@field _ns integer? | ||
| ---@field _wins table<integer, { bufnr: integer, shorten_len: integer }> | ||
| ---@field _wins table<integer, fzf-lua.PathShortener.WinData> | ||
| local PathShortener = {} |
There was a problem hiding this comment.
Define fzf-lua.PathShortener.WinData to fix current type/lint breakage.
Line 11 references fzf-lua.PathShortener.WinData, but that type is not declared; this causes the downstream field warnings at Line 28, Line 31, and Line 32.
Suggested type declaration
+---@class fzf-lua.PathShortener.WinData
+---@field bufnr integer
+---@field shorten_len integer
+---@field fmt_opts? { hl_dir?: string, hl_file?: string, formatter?: string, fmt_to?: fun(...): any, opts?: fzf-lua.config.Resolved|{} }
+---@field topline? integer
+---@field winid? integer
+
---@class fzf-lua.PathShortener
---@field _ns integer?
---@field _wins table<integer, fzf-lua.PathShortener.WinData>
local PathShortener = {}Also applies to: 25-33, 327-331
🧰 Tools
🪛 GitHub Actions: Lint
[warning] 11-11: Undefined type or alias fzf-lua.PathShortener.WinData.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lua/fzf-lua/win.lua` around lines 9 - 12, Add a missing type declaration for
fzf-lua.PathShortener.WinData so the PathShortener._wins field and its usages
type-check; specifically declare the WinData shape (the table fields referenced
later) near the top with the expected fields used by PathShortener (e.g., window
id, buffer id, and any cached short path or namespace fields) and attach it to
the PathShortener namespace (i.e., define fzf-lua.PathShortener.WinData before
PathShortener._wins is declared); update the declaration for PathShortener to
reference that newly defined WinData so the downstream references in methods
like those touching PathShortener._wins no longer produce lint/type warnings.
Comment on lines
+251
to
+253
| local original_width = fn.strdisplaywidth(utils.strip_ansi_coloring(original_path_portion)) + filename_first_prefix_width | ||
| local shortened_width = fn.strdisplaywidth(shortened_path) + filename_first_prefix_width | ||
| local padding = original_width - shortened_width |
There was a problem hiding this comment.
Fix filename-first padding overcount.
Line 251 double-counts the filename prefix width in original_width. original_path_portion already includes that prefix, so padding becomes too large in filename-first mode.
Proposed fix
- local original_width = fn.strdisplaywidth(utils.strip_ansi_coloring(original_path_portion)) + filename_first_prefix_width
+ local original_width = fn.strdisplaywidth(utils.strip_ansi_coloring(original_path_portion))
local shortened_width = fn.strdisplaywidth(shortened_path) + filename_first_prefix_width📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| local original_width = fn.strdisplaywidth(utils.strip_ansi_coloring(original_path_portion)) + filename_first_prefix_width | |
| local shortened_width = fn.strdisplaywidth(shortened_path) + filename_first_prefix_width | |
| local padding = original_width - shortened_width | |
| local original_width = fn.strdisplaywidth(utils.strip_ansi_coloring(original_path_portion)) | |
| local shortened_width = fn.strdisplaywidth(shortened_path) + filename_first_prefix_width | |
| local padding = original_width - shortened_width |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lua/fzf-lua/win.lua` around lines 251 - 253, original_width is currently
computed as fn.strdisplaywidth(utils.strip_ansi_coloring(original_path_portion))
+ filename_first_prefix_width which double-counts the filename prefix because
original_path_portion already contains that prefix; remove the extra addition so
original_width is just
fn.strdisplaywidth(utils.strip_ansi_coloring(original_path_portion)), keep
shortened_width calculation unchanged, and recompute padding = original_width -
shortened_width to fix the filename-first padding overcount (adjust any tests or
downstream uses of original_width if they assumed the previous inflated value).
When winopts.path_shorten uses conceal extmarks to visually shorten directory components, the reduced display width causes fzf's right-side UI elements (preview border, scrollbar) to shift left. This happens because terminal buffers calculate layout based on actual character cells, and concealing text makes the terminal see fewer characters. Replace the conceal-based approach with virt_text_pos="overlay" which renders shortened path text on top of the original, preserving the underlying line's full character cell count and keeping fzf's layout intact. Implementation details: - Build shortened path string (e.g. "l/f/cmd.lua" from "lua/fzf-lua/cmd.lua"), pad with trailing spaces to match original display width, apply single overlay extmark per line - Use vim.fn.strdisplaywidth for width calculation to correctly handle CJK wide characters, multi-byte UTF-8, and icons - Detect actual path boundaries by stopping at components containing whitespace, preventing fzf status text (e.g. "76/281") from being treated as path separators - Skip non-path lines (fzf prompt, action headers) via early validation: reject lines where a space appears before the first path separator - Handle directory paths with trailing "/" (cwd in fzf header) by checking for whitespace after the last separator - Remove conceallevel/concealcursor window option management since conceal is no longer used - Remove dead _calculate_concealed_width() function and abandoned on_lines callback infrastructure Refs #2607
ibhagwan
force-pushed
the
fix/path_shorten_padding
branch
from
96e18a6 to
2106e6f
Compare
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
1 participant
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.
…#2607)
When winopts.path_shorten uses ephemeral conceal extmarks to visually shorten directory components, the reduced display width causes fzf right- side UI elements (scrollbar, preview border) to shift left.
Add inline virtual text padding (spaces) at end-of-line to maintain the original visual line width. The padding width is computed using strdisplaywidth to correctly handle multibyte and CJK wide characters.
Refactor _apply_line into a pure _compute_line function (returns extmark specs) and a thin _apply_line wrapper (sets ephemeral extmarks), making the logic testable without ephemeral extmark limitations.
Also fix vim.str_utfindex call to use the 0.11+ single-return API and expose PathShortener via M._PathShortener for testing.
Fixes #2607
@phanen, created by Opus, needs more work
Summary by CodeRabbit
Refactor
New Features