fix: use display-width-aware text utils for CJK character rendering#39
Merged
fix: use display-width-aware text utils for CJK character rendering#39
Conversation
Full-width characters (CJK, emoji) occupy 2 terminal columns but were counted as 1 by .chars().count(), causing display corruption in the history browser and help browser. Add unicode-width crate and a shared pager/text_utils module that performs all width calculations in display columns. Remove duplicated truncate_str/is_truncated/scroll_display_str from both help.rs and history_browser.rs in favor of the shared utilities. Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Fix skip_columns overshoot: return actual columns skipped so callers can compensate when a wide char straddles the column boundary. Update scroll_display to pad with spaces when overshoot occurs, maintaining exact output width. - Add doc note to pad_to_width clarifying intentional no-ellipsis truncation behavior. - Add emoji tests (display_width, truncate_to_width) to document expected behavior and catch regressions on unicode-width upgrades. - Remove stray blank line in help.rs. Co-Authored-By: Claude Opus 4.5 <[email protected]>
Pin exact expected values in tests instead of permissive range/property checks, so they can actually detect regressions on unicode-width upgrades. Co-Authored-By: Claude Opus 4.5 <[email protected]>
Co-Authored-By: Claude Opus 4.5 <[email protected]>
There was a problem hiding this comment.
Pull request overview
Adds display-width-aware text handling for the pager UI so CJK/full-width characters render and truncate correctly in terminal columns.
Changes:
- Introduces
text_utilsmodule implementing display-width-aware width, truncation, scrolling, and padding utilities (with tests). - Updates help/history pager rendering to use the new utilities instead of character-count-based logic.
- Adds
unicode-widthas a workspace dependency and wires it intoarf-console.
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/arf-console/src/pager/text_utils.rs | New display-width-aware text utilities + unit tests. |
| crates/arf-console/src/pager/mod.rs | Exposes the new text_utils module to sibling pager modules. |
| crates/arf-console/src/pager/history_browser.rs | Switches truncation/padding/scrolling to display-width-aware utilities. |
| crates/arf-console/src/pager/help.rs | Switches truncation/padding/scrolling to display-width-aware utilities. |
| crates/arf-console/Cargo.toml | Adds unicode-width dependency via workspace. |
| Cargo.toml | Pins workspace unicode-width version. |
| Cargo.lock | Records the new dependency in the lockfile. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- truncate_to_width: return empty string when max_width==0 instead of returning "…" which violates the width contract - scroll_display: add early guard for max_width==0 to prevent underflow panic on the bare `max_width - 1` subtraction - scroll_display: pad the eff==0 (start) branch output when a wide char straddles the boundary, matching the behavior of middle and end branches - Add tests for all three edge cases Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Assert exact string value for scroll_start_cjk_boundary_padding no-padding sub-case instead of width-only check - Add non-zero scroll_pos case to scroll_zero_width test to confirm the early guard catches all paths Co-Authored-By: Claude Opus 4.5 <[email protected]>
…th==0 The previous commit changed truncate_to_width to return "" for max_width==0 but missed updating the corresponding test in help.rs. Co-Authored-By: Claude Opus 4.5 <[email protected]>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 7 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- scroll_display: add early guard for max_width==1 (always return "…") - scroll_display: clamp overshoot to inner_cols in middle branch so output never exceeds max_width (e.g. max_width==2 with CJK text) - help.rs: build footer as plain text before padding, apply dark_grey after pad_to_width to avoid ANSI escape sequences being counted as visible columns Co-Authored-By: Claude Opus 4.5 <[email protected]>
Assert exact output strings (" …", "……", "… ") in addition to
display_width checks, consistent with previous review feedback.
Co-Authored-By: Claude Opus 4.5 <[email protected]>
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.