-
Notifications
You must be signed in to change notification settings - Fork 8.5k
fix: preserve tab characters when pasting text #10560
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Summary of ChangesHello @naaa760, 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 an issue where tab characters were inadvertently stripped during text input in the CLI, leading to a loss of formatting for structured content. By adjusting the character stripping logic within the 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 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 counter productive. 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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request correctly modifies stripUnsafeCharacters
to preserve tab characters in pasted text, which is a good improvement for handling structured content. My review identifies one high-severity maintainability issue: the removal of important explanatory comments that clarified the logic for handling control characters. I've provided a suggestion to restore these comments to ensure the code remains clear and easy to maintain.
|
||
// Preserve CR/LF for line handling | ||
if (code === 0x0a || code === 0x0d) return true; | ||
if (code === 0x0a || code === 0x0d || code === 0x09) return true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While this change is functionally correct, it removes helpful comments that explained the logic for handling C0 control characters. This reduces the code's clarity and maintainability, making it harder for future developers to understand why certain characters are preserved while others are stripped. Please consider restoring updated versions of these comments. For instance, a comment explaining the preservation of CR, LF, and TAB would be beneficial here. The comment explaining the removal of other C0 characters, which was previously before the next if
statement, was also valuable and should be restored.
if (code === 0x0a || code === 0x0d || code === 0x09) return true; | |
// Preserve CR, LF, and TAB for line breaks and structured text. | |
if (code === 0x0a || code === 0x0d || code === 0x09) return true; |
TLDR
Dive Deeper
The issue was in the
stripUnsafeCharacters
function which filters control characters during text input. It was removing all C0 control characters (0x00-0x1F) except CR/LF, but this also removed TAB characters (0x09) that are essential for structured text formatting.The fix adds TAB to the list of preserved characters alongside CR/LF, ensuring that when users paste HTML tables or other tab-separated content, the structure remains intact for the model to process correctly.
Testing Matrix
Linked issues / bugs
fixes #10546