feat: add incremental segment cleanup controls#19
Conversation
|
@codex review |
|
@codex review |
ElbertePlinio
force-pushed
the
feat/segment-cleanup
branch
from
13bae1a to
dffb23b
Compare
|
@codex review |
![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: 13bae1ab03
ℹ️ 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".
| if args.local_only && !is_local_endpoint(&endpoint) { | ||
| return Err(anyhow!( | ||
| "local-only mode blocks remote endpoint {endpoint}; use Ollama or disable cleanup" | ||
| )); | ||
| } |
There was a problem hiding this comment.
When incremental segment cleanup invokes this helper with --local-only and the cleanup provider is left at the default auto, resolve_llm_config has already preferred DeepSeek/OpenAI above whenever an API key is present. This new check then rejects that remote endpoint instead of resolving auto to Ollama like the app config path does, so users with local-only enabled and a remote API key get raw/no segment cleanup even when a local Ollama endpoint is available. Resolve auto to the local provider before applying the endpoint block.
Useful? React with 👍 / 👎.
| let status = wait_for_cancellable_child(&mut child, process_group, is_cancelled) | ||
| .context("failed to wait for cleanup command")?; | ||
| let mut stdout = String::new(); | ||
| if let Some(mut child_stdout) = child.stdout.take() { | ||
| child_stdout |
There was a problem hiding this comment.
When incremental segment cleanup uses a custom or verbose cleanup helper that writes more than the OS pipe buffer to stdout, the child can block on its write because this path waits for process exit before reading from the piped stdout. In that case the segment cleanup worker stops producing cleaned live segments until the recording is stopped or cancelled; read stdout concurrently with the cancellable wait or otherwise avoid waiting on a full pipe.
Useful? React with 👍 / 👎.
| break; | ||
| } | ||
|
|
||
| let outcome = cleanup::clean(&cfg, &raw.raw_text); |
There was a problem hiding this comment.
When a recording finishes normally while a raw segment is queued, dropping SegmentCleanupWorker only drops the channels; it never cancels or joins this spawned thread. If the session is still current while the final cleanup/paste pipeline is running, the thread can still execute this extra cleanup::clean even though the result receiver is gone, causing a redundant LLM request for a partial segment after stop. Cancel the worker token or otherwise shut the thread down before returning.
Useful? React with 👍 / 👎.
ElbertePlinio
force-pushed
the
feat/segment-cleanup
branch
from
dffb23b to
35b1b6d
Compare
|
Addressed the Codex findings from the previous head:
Also added regressions for local-only auto and large cleanup stdout, and reran the Rust coverage gate locally. |
|
@codex review |
ElbertePlinio
force-pushed
the
feat/segment-cleanup
branch
from
35b1b6d to
f503513
Compare
|
@codex review |
|
Removed the signal-heavy unit test that was killing |
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
1 participant
Changed
--incremental-cleanup/PICKSCRIBE_INCREMENTAL_CLEANUP=1, gated to output-only or no-paste worker starts.Refs #14
Tested
cargo test --workspace --locked --all-targetsbun run checkbun run testbun run test:coveragegit diff --checkrustfmt --edition 2024 src/bin/pickscribe.rs src/bin/pickscribe-cleanup.rs src-tauri/src/engine.rsNot tested
cargo fmt --all --checkis blocked here because this toolchain is missingcargo-fmt.Risks