fix(core): do not set tasks which cannot be interactive to interactive #31240
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
|
View your CI Pipeline Execution ↗ for commit d240aa8.
☁️ Nx Cloud last updated this comment at |
There was a problem hiding this comment.
Pull Request Overview
This PR prevents tasks that aren’t capable of interactive I/O from being marked interactive by:
- Splitting
PtyInstanceinto interactive and non-interactive constructors and tracking aninteractiveflag - Updating lifecycle and app logic to register and render tasks based on their interactivity
- Swapping to
parking_lot::Mutexfor writer locking and adjusting write paths accordingly
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/nx/src/native/tui/pty.rs | Introduces interactive flag, non_interactive() constructor, and optional writer |
| packages/nx/src/native/tui/lifecycle.rs | Renames register_running_task to register_running_interactive_task |
| packages/nx/src/native/tui/components/terminal_pane.rs | Adds debug logging for ASCII control sequences |
| packages/nx/src/native/tui/app.rs | Branches on PtyInstance.interactive for status updates and registration |
| packages/nx/src/native/pseudo_terminal/pseudo_terminal.rs | Swaps in parking_lot::Mutex and removes unused imports |
| packages/nx/src/native/pseudo_terminal/* | Adjusts writer locking in Windows/Unix command handlers |
Comments suppressed due to low confidence (2)
packages/nx/src/native/tui/lifecycle.rs:259
- [nitpick] The new method name
register_running_interactive_taskand the existingregister_running_taskare similar and may cause confusion. Consider renaming one or consolidating behavior for clarity.
.register_running_interactive_task(task_id, parser_and_writer)
packages/nx/src/native/pseudo_terminal/process_killer/unix.rs:17
- [nitpick] The parameter
signalis shadowed by the local variable of the same name. Consider renaming the argument (e.g.,signal_str) to avoid confusion.
pub fn kill(&self, signal: Option<&str>) -> anyhow::Result<()> {
packages/nx/src/native/tui/pty.rs
Outdated
| pub task_id: String, | ||
| pub parser: Arc<RwLock<Parser>>, | ||
| pub writer: Arc<Mutex<Box<dyn Write + Send>>>, | ||
| pub interactive: bool, |
There was a problem hiding this comment.
[nitpick] The interactive flag is public while the other fields are private, which can lead to inconsistent encapsulation. Consider making it private with a getter or enforcing access through methods.
| pub interactive: bool, | |
| interactive: bool, |
Copilot uses AI. Check for mistakes.
packages/nx/src/native/tui/app.rs
Outdated
| if status == TaskStatus::InProgress | ||
| && self.tasks.len() == 1 | ||
| && self | ||
| .pty_instances | ||
| .get(&task_id) | ||
| .is_some_and(|pty| pty.interactive) | ||
| { | ||
| self.terminal_pane_data[0].set_interactive(true); | ||
| } |
There was a problem hiding this comment.
[nitpick] This conditional spans multiple lines and checks task count and interactive flag. Extracting it into a well-named helper would improve readability.
| if status == TaskStatus::InProgress | |
| && self.tasks.len() == 1 | |
| && self | |
| .pty_instances | |
| .get(&task_id) | |
| .is_some_and(|pty| pty.interactive) | |
| { | |
| self.terminal_pane_data[0].set_interactive(true); | |
| } | |
| if self.should_set_terminal_interactive(&task_id, status) { | |
| self.terminal_pane_data[0].set_interactive(true); | |
| } | |
| } | |
| fn should_set_terminal_interactive(&self, task_id: &String, status: TaskStatus) -> bool { | |
| status == TaskStatus::InProgress | |
| && self.tasks.len() == 1 | |
| && self | |
| .pty_instances | |
| .get(task_id) | |
| .is_some_and(|pty| pty.interactive) |
Copilot uses AI. Check for mistakes.
packages/nx/src/native/tui/app.rs
Outdated
| let pty = PtyInstance::new(parser_and_writer.0.clone(), parser_and_writer.1.clone()); | ||
| self.register_running_task(task_id, pty) | ||
| } | ||
|
|
||
| pub fn register_running_task_with_empty_parser(&mut self, task_id: String) { | ||
| let (_, parser_and_writer) = Self::create_empty_parser_and_noop_writer(); | ||
| self.create_and_register_pty_instance(&task_id, parser_and_writer); | ||
| debug!("Registering task with an empty parser and writer: {task_id}"); | ||
| let pty = Self::create_empty_parser(); | ||
| self.register_pty_instance(&task_id, pty); | ||
| self.update_task_status(task_id.clone(), TaskStatus::InProgress); | ||
| // Ensure the pty instances get resized appropriately | ||
| let _ = self.debounce_pty_resize(); | ||
| } | ||
|
|
||
| fn register_running_task(&mut self, task_id: String, pty: PtyInstance) { | ||
| debug!("Registering task: {task_id}"); |
There was a problem hiding this comment.
[nitpick] This private helper overlaps with register_running_interactive_task. Consider merging or renaming to reduce duplication and improve discoverability.
Copilot uses AI. Check for mistakes.
FrozenPandaz
force-pushed
the
fix-non-interactive
branch
from
389a4ef to
a03ef0d
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR fixes the interactive mode for tasks by ensuring that tasks not capable of interactivity are not mistakenly set as interactive.
- Introduces separate constructors (interactive and non_interactive) in the PtyInstance struct.
- Renames and restructures task registration methods to clearly differentiate between interactive and non-interactive tasks.
- Replaces some standard Mutex imports with parking_lot's Mutex, aligning synchronization across modules.
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/nx/src/native/tui/pty.rs | Updates PtyInstance constructors and writer handling for interactivity. |
| packages/nx/src/native/tui/lifecycle.rs | Renames task registration methods to distinguish interactive tasks. |
| packages/nx/src/native/tui/app.rs | Refactors task registration, output handling, and interactivity condition logic. |
| Various pseudo_terminal files | Replaces std::sync::Mutex with parking_lot::Mutex for performance consistency. |
FrozenPandaz
force-pushed
the
fix-non-interactive
branch
3 times, most recently
from
ed6e309 to
b00bc9e
Compare
FrozenPandaz
force-pushed
the
fix-non-interactive
branch
from
b00bc9e to
d240aa8
Compare
|
This pull request has already been merged/closed. If you experience issues related to these changes, please open a new issue referencing this pull request. |
FrozenPandaz
added
scope: docs
3 participants
Current Behavior
Some tasks cannot be interactive because of the way they are implemented. At the moment, those tasks can be set to interactive mode.
Expected Behavior
Tasks which cannot be interactive are not set as interactive
Related Issue(s)
Fixes #