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 focuses on simplifying and refining the internal architecture of the library. It primarily involves adjusting the visibility of several structs, enums, and methods to Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request refactors the attr, color, framebuffer, render, term, and window modules, primarily by reducing the public API surface and making many functions and structs pub(crate) or private. Specifically, to_ansi methods were removed from Attr and Color, Cell fields and its constructor were made private, and Framebuffer's internal buffer was changed from Vec<Cell> to Box<[Cell]> for potential performance benefits. The csi! macro was removed, and Terminal::exec was updated to use write_all on a locked stdout for efficiency. In RenderThread, the continue on TryLockError::WouldBlock was replaced with thread::yield_now(). However, critical issues were introduced in WindowSizeListener where the thread is detached and the stop signal sender is immediately dropped, making the thread unmanageable and its stop method a no-op, leading to resource leaks. Additionally, in Terminal::enable_raw_mode, the ECHONL flag is no longer explicitly removed, which, while potentially benign due to ICANON being disabled, could be explicitly removed for best practice and consistent terminal state.
| let (_stop_tx, stop_rx): (Sender<()>, Receiver<()>) = mpsc::channel(); | ||
|
|
||
| let handle = thread::spawn(move || { | ||
| let _handle = thread::spawn(move || { |
There was a problem hiding this comment.
The JoinHandle returned by thread::spawn is assigned to _handle, which immediately drops it, detaching the thread. This prevents proper management of the thread's lifecycle, making it impossible to join or explicitly stop it. This reinforces the critical issue of an unmanaged thread.
| let _handle = thread::spawn(move || { | |
| let handle = thread::spawn(move || { |
| let (size_tx, size_rx): (SyncSender<(usize, usize)>, Receiver<(usize, usize)>) = | ||
| mpsc::sync_channel(1); | ||
| let (stop_tx, stop_rx): (Sender<()>, Receiver<()>) = mpsc::channel(); | ||
| let (_stop_tx, stop_rx): (Sender<()>, Receiver<()>) = mpsc::channel(); |
There was a problem hiding this comment.
The stop_tx sender for the stop signal is assigned to _stop_tx, which immediately drops it. This means there is no way to send a stop signal to the WindowSizeListener thread, making it impossible to gracefully terminate the thread. This is a critical issue for thread management.
| let (_stop_tx, stop_rx): (Sender<()>, Receiver<()>) = mpsc::channel(); | |
| let (stop_tx, stop_rx): (Sender<()>, Receiver<()>) = mpsc::channel(); |
| stop_signal: Some(stop_tx), | ||
| size_rx, | ||
| } | ||
| Self { size_rx } |
There was a problem hiding this comment.
The WindowSizeListener constructor no longer stores the handle and stop_signal fields, which were removed from the struct definition. This confirms the detached nature of the thread and the inability to stop it, as noted in previous comments.
Self {
handle: Some(handle),
stop_signal: Some(stop_tx),
size_rx,
}| fn stop(&mut self) -> Result<(), Box<dyn std::error::Error>> { | ||
| Ok(()) |
There was a problem hiding this comment.
The stop method for WindowSizeListener has been made a no-op. Combined with the detached thread and dropped stop_signal sender, this means there is no mechanism to stop the window size listener thread, leading to a resource leak and unmanaged background process.
fn stop(&mut self) -> Result<(), Box<dyn std::error::Error>> {
if let Some(tx) = self.stop_signal.take() {
tx.send(())?; // Send stop signal
}
if let Some(handle) = self.handle.take() {
handle
.join()
.map_err(|_| "Failed to join window size listener thread")?; // Wait for the thread to finish
}
Ok(())
}| raw.local_flags | ||
| .remove(LocalFlags::ICANON | LocalFlags::ECHO | LocalFlags::ISIG | LocalFlags::IEXTEN); |
There was a problem hiding this comment.
The ECHONL flag is no longer explicitly removed from raw.local_flags. While ICANON is disabled, which usually makes ECHONL irrelevant, it's generally best practice in raw mode to explicitly disable all echoing to prevent any unexpected behavior or ensure consistent terminal state. Consider explicitly removing ECHONL if it's not intended to be active.
| raw.local_flags | |
| .remove(LocalFlags::ICANON | LocalFlags::ECHO | LocalFlags::ISIG | LocalFlags::IEXTEN); | |
| raw.local_flags | |
| .remove(LocalFlags::ICANON | LocalFlags::ECHONL | LocalFlags::ECHO | LocalFlags::ISIG | LocalFlags::IEXTEN); |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the library to improve performance and clarify its public API by making several modules and functions internal to the crate. Specifically, attr::to_ansi and color::to_ansi methods were removed in favor of write_ansi to avoid string allocations, and the csi! macro was removed. The framebuffer::Cell fields and constructor, framebuffer::refresh, render::RenderThread and its associated methods, and the term::Terminal struct and its methods were all changed from public to pub(crate) or private. The Framebuffer's internal buffer was optimized from Vec<Cell> to Box<[Cell]>. The RenderThread's TryLockError handling was improved by using thread::yield_now() to prevent busy-waiting, and term::Terminal::exec was optimized to use direct byte writes to stdout. Additionally, term::Terminal::enable_raw_mode now correctly handles multi-byte characters by inserting InputFlags::IUTF8, and term::Terminal::set_nonblocking was clarified to apply to STDIN_FILENO. However, the WindowSizeListener implementation introduced a thread leak and failed to restore the terminal state because the stop signal sender and thread handle were not properly retained, leading to the background thread running indefinitely and the terminal remaining in raw mode after application exit.
| let (_stop_tx, stop_rx): (Sender<()>, Receiver<()>) = mpsc::channel(); | ||
|
|
||
| let handle = thread::spawn(move || { | ||
| let _handle = thread::spawn(move || { |
There was a problem hiding this comment.
The WindowSizeListener implementation introduced in this PR contains a thread leak and fails to restore the terminal state.
- Thread Leak: The stop signal sender
_stop_txis created and immediately dropped at the end of thenewfunction. Inside the thread's loop,stop_rx.try_recv().is_ok()will always be false because the sender is disconnected, and the code does not check for theDisconnectedstate. The thread only terminates if a window resize occurs and it fails to send a message to a disconnectedsize_rx. If no resize occurs, the thread runs indefinitely. - Terminal State Persistence: Because the leaked thread owns the
Terminalinstance, itsDropimplementation (which restores terminal settings) is never called. This leaves the user's terminal in 'raw' mode even after the application exits.
To fix this, you should retain the stop signal sender and the thread handle in the WindowSizeListener struct and properly join the thread during drop.
fn new(terminal: Terminal, rate: Duration) -> Self {
#[allow(clippy::type_complexity)]
let (size_tx, size_rx): (SyncSender<(usize, usize)>, Receiver<(usize, usize)>) = mpsc::sync_channel(1);
let (stop_tx, stop_rx): (Sender<()>, Receiver<()>) = mpsc::channel();
let _handle = thread::spawn(move || {
let mut prev_window_size: Option<(usize, usize)> = None;
loop {
match stop_rx.try_recv() {
Ok(_) | Err(mpsc::TryRecvError::Disconnected) => break,
Err(mpsc::TryRecvError::Empty) => {}
}
if let Ok((width, height)) = terminal.get_size() {
if prev_window_size != Some((width, height)) {
if size_tx.try_send((width, height)).is_err() {
break;
}
prev_window_size = Some((width, height));
}
}
thread::sleep(rate);
}
});
Self { size_rx } // Note: This struct needs to be updated to hold stop_tx and _handle to fully resolve the leak.| /// Stop the window size listener | ||
| /// | ||
| /// Returns `Ok(())` if the listener was stopped successfully, or an error if it failed. | ||
| pub fn stop(&mut self) -> Result<(), Box<dyn std::error::Error>> { | ||
| if let Some(tx) = self.stop_signal.take() { | ||
| tx.send(())?; // Send stop signal | ||
| } | ||
| if let Some(handle) = self.handle.take() { | ||
| handle | ||
| .join() | ||
| .map_err(|_| "Failed to join window size listener thread")?; // Wait for the thread to finish | ||
| } | ||
| fn stop(&mut self) -> Result<(), Box<dyn std::error::Error>> { | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
The stop method is now a no-op and is only called from the Drop implementation. Since the background thread is correctly terminated by the dropping of size_rx when WindowSizeListener is dropped, this explicit stop method is no longer necessary and can be removed for simplification. The Drop implementation on lines 234-238 can also be removed.
1 participant
No description provided.