fix(core-tools): show ellipsis on multi-line edit snippets#28126
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves a display issue in the edit tool's confirmation UI where multi-line snippets were incorrectly presented without an ellipsis if the first line was short. The changes ensure that users are properly notified of hidden content in multi-line edits. Additionally, the PR improves test suite stability across different operating systems and removes legacy bot configuration files that are no longer in use. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
|
📊 PR Size: size/M
|
There was a problem hiding this comment.
Code Review
This pull request refactors the description snippet generation in EditTool to correctly append an ellipsis when content is hidden, adds comprehensive unit tests for these scenarios, and canonicalizes the temporary directory path in tests to resolve macOS path mismatches. It also removes several obsolete agent/skill definition files. The reviewer recommended handling CRLF line endings to prevent terminal rendering corruption and using Array.from() to avoid splitting multi-byte Unicode characters during truncation.
| const snippet = (s: string) => { | ||
| const firstLine = s.split('\n')[0]; | ||
| const hasHiddenContent = firstLine.length > 30 || s.includes('\n'); | ||
| return firstLine.substring(0, 30) + (hasHiddenContent ? '...' : ''); | ||
| }; |
There was a problem hiding this comment.
When splitting the string by \n on files with CRLF (\r\n) line endings, the resulting firstLine will retain the trailing carriage return (\r). Since this is a terminal-first CLI tool, printing a string containing a carriage return followed by an ellipsis (e.g., foo\r...) can cause the terminal cursor to reset to the beginning of the line, leading to rendering corruption where the ellipsis overwrites the start of the line.
Additionally, when truncating strings that may contain multi-byte Unicode characters (such as emojis), we should use methods that operate on grapheme clusters (like Array.from()) instead of UTF-16 code units (string.length, string.substring()) to prevent character splitting.
| const snippet = (s: string) => { | |
| const firstLine = s.split('\n')[0]; | |
| const hasHiddenContent = firstLine.length > 30 || s.includes('\n'); | |
| return firstLine.substring(0, 30) + (hasHiddenContent ? '...' : ''); | |
| }; | |
| const snippet = (s: string) => { | |
| const firstLine = s.split(/\r?\n/)[0]; | |
| const chars = Array.from(firstLine); | |
| const hasHiddenContent = chars.length > 30 || s.includes('\n'); | |
| return chars.slice(0, 30).join('') + (hasHiddenContent ? '...' : ''); | |
| }; |
References
- When truncating strings that may contain multi-byte Unicode characters (e.g., emojis), use methods that operate on grapheme clusters (like
Intl.SegmenterorArray.from()) instead of UTF-16 code units (string.length,string.slice()) to prevent character splitting.
🛑 Action Required: Evaluation ApprovalSteering changes have been detected in this PR. To prevent regressions, a maintainer must approve the evaluation run before this PR can be merged. Maintainers:
Once approved, the evaluation results will be posted here automatically. |
EditToolInvocation.getDescription() decided whether to append "..." to the displayed snippet based on the full string length. For multi-line edits whose first line was short, the snippet rendered without ellipsis and the confirmation UI looked like only a single short line changed. The ellipsis now reflects what is actually hidden from the snippet: either the first line was truncated past 30 chars, or there are additional lines below it. Fixes google-gemini#28110 Fixes google-gemini#28109
3542b59 to
5a19cf7
Compare
|
Force-pushed to remove an unintended 5-file deletion under |
Summary
EditToolInvocation.getDescription()decided whether to append...to thesnippet by checking the full string length. For multi-line edits whose first
line was short (≤ 30 chars), the ellipsis was skipped and the confirmation UI
looked like only a single short line changed.
New condition:
firstLine.length > 30 || s.includes('\n')— ellipsis appearswhenever the snippet actually hides content from the user. CRLF handling is
unchanged from the original implementation.
Related Issues
Fixes #28110
Fixes #28109
How to Validate
Drive-by: also canonicalizes the test's
tempDirviafs.realpathSyncsoedit.test.tsruns cleanly on macOS (previously ~30 unrelated cases in thesame file failed locally due to
/var→/private/var).Pre-Merge Checklist