Skip to content

Strip Silence: closing the window while Mix shows stops the rectangle scan - #1095

Merged
echelon merged 2 commits into
storytold:mainfrom
dotCooCoo:fix/strip-silence-scan-stops
Oct 30, 2026
Merged

echelon merged 2 commits into
storytold:mainfrom
dotCooCoo:fix/strip-silence-scan-stops

Conversation

@dotCooCoo

@dotCooCoo dotCooCoo commented Oct 25, 2026 •

Copy link
Copy Markdown
Contributor

Follow-up to #1055, for two review comments left open when it merged.

On main, only the Edit window drops the Strip Silence rectangles and stops their scan when Strip Silence closes (strip_silence_rectangles, called from edit_window::track_row). If the Mix window is showing when the window closes (after window.mix), a scan for old values runs to the end and strip_scan_running() stays true.

  • dialogs::show, which runs every frame whichever window shows, now drops the rectangles and stops the scan whenever neither Strip Silence nor its Rename Selected Clips dialog is open. strip_silence_rectangles (in dialogs/strip_silence.rs) returns no rectangles in that case and no longer resets the state itself.
  • edit.strip_silence_preview, strip_silence_scan and strip_settings move from cmd/edit.rs to a new cmd/strip_silence.rs with its own specs(), registered in cmd.rs, as AGENTS.md asks for new commands. edit.strip_silence reads its settings through the same strip_settings. The command's id, label and params do not change.

New test strip_silence_rectangles::closing_the_window_while_the_mix_window_shows_stops_the_scan (ui-egui): it starts a scan, shows the Mix window, closes Strip Silence and checks that no scan is running. It fails with the dialogs::show change removed.

On Windows 11 (x86-64) with rustc 1.99.0, with SOUNDCRAFT_NO_SYSTEM_PLUGINS=1 set:

  • On ee55acb (from main at a44d1b8), cargo xtask ci passed its 7 steps: 1,437 tests passed, 0 failed, 1 ignored.
  • On c14dc61 (merged with main at e82c168): cargo fmt --all -- --check; cargo clippy -p soundcraft-engine -p soundcraft-ui-egui --all-targets -- -D warnings; cargo test --no-fail-fast -p soundcraft-engine -p soundcraft-ui-egui -p soundcraft-automation -p soundcraft -p soundcraft-cli -p soundcraft-web: 928 passed, 3 failed, 0 ignored, in 24 binaries (the 3 are soundcraft-cli app_ tests that fail the same way on main at e82c168 on this machine); cargo xtask layers and assets pass.

Summary by CodeRabbit

  • Bug Fixes
    • Improved Strip Silence preview handling when switching between dialogs. Preview scan activity now stops when the Strip Silence dialog closes, and stale preview rectangles are cleared when another dialog is open.
    • Updated how Strip Silence preview scans are accessed, with no change to the available preview settings or detected-range results.

… scan

Only the Edit window dropped the Strip Silence rectangles and their scan
when Strip Silence closed. With the Mix window showing, a scan for old
values ran to the end and strip_scan_running() stayed true.
dialogs::show, which runs every frame, now drops them whenever neither
Strip Silence nor its Rename Selected Clips dialog is open.

edit.strip_silence_preview, strip_silence_scan and strip_settings move
from cmd/edit.rs to a new cmd/strip_silence.rs with its own specs(), as
AGENTS.md asks for new commands. edit.strip_silence reads its settings
through the same strip_settings.
@coderabbitai

coderabbitai Bot commented Oct 25, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review.

Recent review info
Run configuration
Commits

Reviewing files that changed from the base of the PR and between ee55acb and c14dc61.

Files selected for processing: 4
  • crates/engine/src/cmd.rs
  • crates/engine/src/cmd/edit.rs
  • crates/ui-egui/src/dialogs.rs
  • crates/ui-egui/src/dialogs/strip_silence.rs

Limit details: You’ve used all 10 included reviews currently available.

Walkthrough

The Strip Silence preview command and scan function moved to a dedicated command module. The edit command now uses its settings parser. Dialog display resets rectangle state outside Strip Silence dialogs, and an integration test checks scan status after the dialog closes.

Changes: Strip Silence command and dialog lifecycle

Layer / File(s) Summary
Shared settings and preview command
crates/engine/src/cmd/strip_silence.rs, crates/engine/src/cmd/edit.rs, crates/engine/src/cmd.rs
The new module parses settings, constructs scans, and provides the preview command. The edit command uses the shared parser, and the command registry registers and re-exports the module’s functions.
Dialog rectangle cleanup
crates/ui-egui/src/dialogs.rs, crates/ui-egui/src/dialogs/strip_silence.rs, crates/ui-egui/tests/it/strip_silence_rectangles.rs
Dialog display resets rectangle state outside Strip Silence dialogs. The rectangle helper no longer resets cached state or an active scan when neither dialog is open. The integration test checks scan status after the dialog closes.

Priority: Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Suggested reviewers: echelon

Merge Risk: Minimal · up to c14dc

Closing Strip Silence stops its scan. No merge-blocking risk is identified.

Architecture risk: Low · up to c14dc

The change affects 1 system.

Changed systems: crates

Architecture concerns
No architecture-level concerns identified.

Review details

Systems and components

  • observed — crates (service) was modified; 6 changed files map to changed impact.

Before / after behavior

  • observed — Modified behavior in crates/engine/src/cmd/strip_silence.rs: Adds the module documentation and registers the edit.strip_silence_preview query command, which accepts optional threshold, minimum-length, padding, track, and range parameters and delegates to strip_silence_preview.
  • observed — Modified behavior in crates/engine/src/cmd/strip_silence.rs: Adds StripSettings and strip_settings, which require tracks and a range, default the threshold to -48 dB and clamp it to [-96, 0], and convert minimum length and before/after padding from milliseconds to samples after clamping each value to [0, 10,000] ms.
  • observed — Modified behavior in crates/engine/src/cmd/strip_silence.rs: Adds the public scan function, which builds a StripScan from the current session, parsed settings, and a default stop value.
  • observed — Modified behavior in crates/engine/src/cmd/strip_silence.rs: Adds the preview handler, which runs the scan and returns each track ID with its detected ranges represented as start/end pairs.
Pre-merge checks: Passed 4
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly and concisely describes the primary change: closing the Strip Silence window while Mix is open stops the rectangle scan.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

…can-stops

# Conflicts:
#	crates/ui-egui/src/dialogs.rs
@dotCooCoo

Copy link
Copy Markdown
Contributor Author

Merged main at e82c168 (c14dc61). main had moved strip_silence_rectangles into dialogs/strip_silence.rs, so the conflict in dialogs.rs was resolved by taking main's version and making this change there; cmd/mod.rs became cmd.rs on main and merged without a conflict. The branch now merges cleanly with main at e82c168.

Checks on Windows 11 (x86-64), rustc 1.99.0, on c14dc61: cargo fmt --all -- --check; cargo clippy -p soundcraft-engine -p soundcraft-ui-egui --all-targets -- -D warnings; cargo test --no-fail-fast for soundcraft-engine, soundcraft-ui-egui and their dependents (soundcraft-automation, soundcraft, soundcraft-cli, soundcraft-web): 928 passed, 3 failed, 0 ignored, in 24 binaries; cargo xtask layers and assets pass. The 3 failures are soundcraft-cli's app_port_comes_from_the_flag_then_the_environment, app_takes_an_empty_port_variable_as_unset and app_reports_a_refused_connection_and_checks_arguments_first, which fail the same way on main at e82c168 on this machine (connection timed out instead of refused). The full cargo xtask ci has not run on the merged head yet. Integration pull request #1107 picks up the new head at its next refresh.

@echelon
echelon merged commit 60d18f2 into storytold:main Oct 30, 2026
6 checks passed
@echelon

echelon commented Oct 30, 2026

Copy link
Copy Markdown
Member

Merged. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants