Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e123ccd0aa
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
codex-rs/tui/src/clipboard_text.rs
Outdated
| }); | ||
|
|
||
| match stdin_result.and_then(|()| { | ||
| child.wait_with_output().map_err(|e| { |
There was a problem hiding this comment.
In the Ok(child) arm, child comes from child.as_mut(), so its type is &mut std::process::Child. wait_with_output consumes self, which cannot be called on a mutable reference. This causes a Linux compile error (E0507: cannot move out of *child) in the new WSL fallback path.
Useful? React with 👍 / 👎.
won-openai
force-pushed
the
slash-copy-osc52-wsl-support
branch
from
296a62c to
7f13c2b
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7f13c2b57f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
codex-rs/tui/src/clipboard_text.rs
Outdated
| child | ||
| .stdin | ||
| .take() | ||
| .ok_or_else(|| "clipboard unavailable: failed to open powershell.exe stdin".to_string())? | ||
| .write_all(text.as_bytes()) | ||
| .map_err(|e| format!("clipboard unavailable: failed to write to powershell.exe: {e}"))?; |
There was a problem hiding this comment.
copy_via_wsl_clipboard spawns powershell.exe before writing to its stdin, but failures from stdin.take()/write_all return early. On those paths the Child is dropped without any wait* call, and dropped children are not reaped automatically, so repeated failures can accumulate zombies in WSL/Linux sessions.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is likely worth investigating more.
joshka-oai
left a comment
joshka-oai
left a comment
There was a problem hiding this comment.
General approval. I'd suggest throwing codex at the nits / refactoring ideas I put here.
I'd also want to make sure the powershell issue is covered.
codex-rs/tui/src/clipboard_text.rs
Outdated
| child | ||
| .stdin | ||
| .take() | ||
| .ok_or_else(|| "clipboard unavailable: failed to open powershell.exe stdin".to_string())? | ||
| .write_all(text.as_bytes()) | ||
| .map_err(|e| format!("clipboard unavailable: failed to write to powershell.exe: {e}"))?; |
There was a problem hiding this comment.
This is likely worth investigating more.
2 participants
This PR is a followup to the /copy feature to support WSL and SSH!