Skip to content
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

fix(Blank Screen): Improve UTF-16 character handling in Typewriter class #5982

Merged
merged 3 commits into from
Oct 23, 2024

Conversation

PriNova
Copy link
Collaborator

@PriNova PriNova commented Oct 23, 2024

Problem

The Typewriter class, which provides the typing animation effect in chat responses, was not properly handling UTF-16 characters. This could lead to broken rendering of:

  • Emoji characters 😀
  • Characters outside the Basic Multilingual Plane
  • Combined Unicode characters
  • Other UTF-16 surrogate pairs

which resulted in a blank screen in the Cody chat.

Solution

Added proper UTF-16 character boundary handling by:

  • Implementing a new getNextValidSlicePoint method that respects code point boundaries
  • Using Array.from() to properly handle UTF-16 code points
  • Ensuring text slicing occurs at valid character boundaries

Demo

Before fix:

Before_BlankScreen.mp4
  • Shows broken emoji/special characters during typing animation

After fix:

After_BlankScreen.mp4
  • Shows proper rendering of all Unicode characters without a blank screen.

Test plan

All tests were performed with the 'Temperature' setting equal to 0 (zero) to obtain deterministic results.

  • Tested with various Unicode characters including:
    • Emoji sequences
    • Combined characters
    • Characters outside BMP
    • Multi-code point characters

Notes

  • No breaking changes to the Typewriter interface
  • Maintains existing typing animation timing and behavior
  • Only affects the character boundary handling

Changelog

…ation

- Add getNextValidSlicePoint method to safely slice text at valid UTF-16 boundaries
- Prevent breaking of special characters and emojis during typewriter animation
- Refactor text processing logic for better character handling
@PriNova PriNova requested a review from abeatrix October 23, 2024 21:06
@kalanchan kalanchan requested a review from a team October 23, 2024 21:30
jamesmcnamara added a commit that referenced this pull request Oct 23, 2024
Hoping to unblock #5983 and #5982.

In #4923 we added the google
auth action to the test-unit step but did not exclude the step if the PR
comes from a fork (as we do with the other auth steps).

@akalia25 Do you see any issue with this tact? Should the setup-gcloud
step get skipped too?

## Test plan
Runs in CI
Copy link
Contributor

@jamesmcnamara jamesmcnamara left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is nice! Thanks for contributing :)

I'd prefer if we kept the comments in place, they are helpful for new comers to the code.

If you pull the latest main, you should get this change which should fix the failing unit tests.

@PriNova
Copy link
Collaborator Author

PriNova commented Oct 23, 2024

This is nice! Thanks for contributing :)

I'd prefer if we kept the comments in place, they are helpful for new comers to the code.

If you pull the latest main, you should get this change which should fix the failing unit tests.

This was accidental because Cody deleted them, but I will add them back now.

@jamesmcnamara jamesmcnamara merged commit e4825cb into sourcegraph:main Oct 23, 2024
19 of 20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants