fix(preview): expand tabs based on current line position#1371
Conversation
Tab characters are now expanded to reach the next tab stop based on the current display position in the line, rather than always expanding to exactly 4 spaces. This matches standard terminal behavior where tabs align to fixed column positions. For example, with TabWidth=4: - Position 1: tab expands to 3 spaces (reaches column 4) - Position 3: tab expands to 1 space (reaches column 4) - Position 4: tab expands to 4 spaces (reaches column 8)
📝 WalkthroughWalkthroughThe change adds display-width tracking ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/internal/common/string_function.go (1)
254-278:⚠️ Potential issue | 🟠 MajorDon’t let ANSI escape payloads advance
curWidth.With
allowEsc=true, Line 256 preserves\x1b, but Lines 271-278 still count the rest of the escape sequence ([1m,[38;...m, etc.) as visible width. That makes Line 249 expand a later tab from the wrong column. For example,MakePrintableWithEscCheck("\x1b[1mA\tB", true)will treat the tab as if it starts at column 4 instead of column 1, so the preview still misaligns for colored content. Please keep ANSI sequences in the output without incrementingcurWidth, and add a regression case for a tab after an ANSI sequence.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/internal/common/string_function.go` around lines 254 - 278, The code currently writes ANSI escape sequences when allowEsc is true but still increments curWidth for the bytes that follow the EscapeChar; update MakePrintableWithEscCheck (and the loop using allowEsc/curWidth/EscapeChar) to, upon seeing an EscapeChar and allowEsc==true, write the ESC and then consume/write the rest of the CSI/escape sequence without modifying curWidth — i.e., after writing ESC inspect subsequent runes and keep writing them until you reach the ANSI sequence terminator (ASCII range '@'..'~', e.g. 'm' for SGR), skipping any curWidth increments for those runes; also add a regression test asserting MakePrintableWithEscCheck("\x1b[1mA\tB", true) treats the tab as starting at column 1 (so the preview aligns correctly).
🤖 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 `@src/internal/common/string_function.go`:
- Around line 254-278: The code currently writes ANSI escape sequences when
allowEsc is true but still increments curWidth for the bytes that follow the
EscapeChar; update MakePrintableWithEscCheck (and the loop using
allowEsc/curWidth/EscapeChar) to, upon seeing an EscapeChar and allowEsc==true,
write the ESC and then consume/write the rest of the CSI/escape sequence without
modifying curWidth — i.e., after writing ESC inspect subsequent runes and keep
writing them until you reach the ANSI sequence terminator (ASCII range '@'..'~',
e.g. 'm' for SGR), skipping any curWidth increments for those runes; also add a
regression test asserting MakePrintableWithEscCheck("\x1b[1mA\tB", true) treats
the tab as starting at column 1 (so the preview aligns correctly).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 87e325cf-1355-4a9e-b8c4-12b71da3a5f2
📒 Files selected for processing (2)
src/internal/common/string_function.gosrc/internal/common/string_function_test.go
Br1an67
force-pushed
the
fix/issue-1290-tab-expansion
branch
from
915caec to
83684b0
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 `@src/internal/common/string_function_test.go`:
- Around line 180-185: The inline comment for the test case {"a\tb\tc", "a b
c"} is incorrect about the second tab's start position; update the comment next
to that test entry (the line with the literal "a\tb\tc") to reflect that after
expanding the first tab and the 'b' the cursor is at position 5, so the second
tab needs 3 spaces to reach position 8 (e.g. change the comment to something
like "// Position 1: 3 spaces, then position 5: 3 spaces to reach 8").
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 45218cbe-a644-453e-80f2-bd821d0ba853
📒 Files selected for processing (2)
src/internal/common/string_function.gosrc/internal/common/string_function_test.go
| // Tab expansion tests - tabs should expand to reach next tab stop (TabWidth=4) | ||
| {"a\tb", "a b"}, // Position 1: need 3 spaces to reach position 4 | ||
| {"ab\tc", "ab c"}, // Position 2: need 2 spaces to reach position 4 | ||
| {"abc\td", "abc d"}, // Position 3: need 1 space to reach position 4 | ||
| {"abcd\te", "abcd e"}, // Position 4: need 4 spaces to reach position 8 | ||
| {"a\tb\tc", "a b c"}, // Position 1: 3 spaces, then position 4: 4 spaces to reach 8 |
There was a problem hiding this comment.
Test cases are correct, but comment on line 185 is inaccurate.
The test logic and expected outputs are correct, but the comment on line 185 is misleading:
// Position 1: 3 spaces, then position 4: 4 spaces to reach 8
After the first tab expansion and 'b', the position is 5 (not 4). At position 5, 4 - (5 % 4) = 3 spaces are needed to reach position 8, not 4 spaces.
📝 Suggested comment fix
- {"a\tb\tc", "a b c"}, // Position 1: 3 spaces, then position 4: 4 spaces to reach 8
+ {"a\tb\tc", "a b c"}, // Position 1: 3 spaces to reach 4, then position 5: 3 spaces to reach 8🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/internal/common/string_function_test.go` around lines 180 - 185, The
inline comment for the test case {"a\tb\tc", "a b c"} is incorrect about the
second tab's start position; update the comment next to that test entry (the
line with the literal "a\tb\tc") to reflect that after expanding the first tab
and the 'b' the cursor is at position 5, so the second tab needs 3 spaces to
reach position 8 (e.g. change the comment to something like "// Position 1: 3
spaces, then position 5: 3 spaces to reach 8").
yorukot
left a comment
yorukot
left a comment
There was a problem hiding this comment.
@Br1an67 Thanks. I think this should be handled in ReadFileContent(...), not in MakePrintableWithEscCheck(...).
MakePrintableWithEscCheck(...) is a shared sanitization helper, but this problem is in the preview path. Preview content gets read and truncated before rendering, so expanding tabs there feels more correct and avoids changing global behavior.
yorukot
added
pr needs work
2 participants
Fixes #1290
Description
This PR fixes the tab expansion logic in the file preview functionality. Previously, tab characters were always replaced with exactly 4 spaces, regardless of the current position in the line. This caused misaligned columns when viewing files containing tabs.
The fix tracks the current display width while processing each line and expands tabs to reach the next tab stop position. This matches standard terminal behavior where tabs align to fixed column boundaries.
For example, with
TabWidth=4:Related Issues
Fixes #1290
Screenshots (Optional)
Before: Tabs always expanded to 4 spaces, causing column misalignment
After: Tabs expand to reach the next tab stop, properly aligning columns
Pre-Submission Checklist
go fmt ./...to format the codegolangci-lint runand fixed any reported issuesDiff stats:
Summary by CodeRabbit