fix(core): do not set tasks which cannot be interactive to interactive#31240
Merged
FrozenPandaz merged 1 commit intomasterfrom May 15, 2025
Merged
fix(core): do not set tasks which cannot be interactive to interactive#31240FrozenPandaz merged 1 commit intomasterfrom
FrozenPandaz merged 1 commit intomasterfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
Contributor
|
View your CI Pipeline Execution ↗ for commit d240aa8.
☁️ Nx Cloud last updated this comment at |
Contributor
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<()> {
| 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.
Suggested change
| pub interactive: bool, | |
| interactive: bool, |
Comment on lines
176
to
178
| 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.
Suggested change
| 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) |
Comment on lines
+269
to
+283
| 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.
389a4ef to
a03ef0d
Compare
Contributor
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. |
ed6e309 to
b00bc9e
Compare
b00bc9e to
d240aa8
Compare
Cammisuli
approved these changes
May 15, 2025
Contributor
|
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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 #