Migrate core render util to use xterm.js as part of the rendering loop.#19044
Migrate core render util to use xterm.js as part of the rendering loop.#19044
Conversation
Summary of ChangesHello @jacob314, 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 undertakes a significant refactoring of the CLI's UI rendering infrastructure by replacing Highlights
Changelog
Activity
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.
Code Review
This pull request is a significant and well-executed migration of the core rendering utility for tests from ink-testing-library to a custom solution using xterm.js. This change provides a more accurate terminal emulation for testing purposes. The new render utility and its associated helper functions like waitUntilReady are well-designed and have been consistently applied across the entire test suite. The removal of setTimeout-based delays in tests is a major improvement for test stability and speed. The code quality is high, and the changes are thorough and consistent. I have no critical or high-severity issues to report. This is an excellent piece of work.
| stderr: stderr as unknown as NodeJS.WriteStream, | ||
| // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion | ||
| stdin: stdin as unknown as NodeJS.ReadStream, | ||
| debug: false, |
There was a problem hiding this comment.
previously we used debug: true which was needed for the hacks to get the last frame without going through a real ink pipeline.
418e58e to
ffb6f4c
Compare
|
Size Change: +58 B (0%) Total Size: 24.4 MB ℹ️ View Unchanged
|
Summary
Switch all tests to xterm headless for snapshot rendering.
render.ts is the main file requiring real review. All other changes are minor snapshot churn due to switching renderers (new renderer always adds trailing newlines).
Test changes should just be mechanical code adding async and waiting for the renderer to be ready.
Diff is way smaller than it looks
Fixes #19045
It is possible we could have gotten away with not making these async but they will need to be async anyway in a bit when we update the ink renderer so didn't attempt to remove that.