Skip to content

Read send destinations only while the menu is open - #1142

Open
dotCooCoo wants to merge 2 commits into
storytold:mainfrom
dotCooCoo:fix/defer-send-menu-bus-list
Open

dotCooCoo wants to merge 2 commits into
storytold:mainfrom
dotCooCoo:fix/defer-send-menu-bus-list

Conversation

@dotCooCoo

@dotCooCoo dotCooCoo commented Oct 27, 2026 •

Copy link
Copy Markdown
Contributor

The mixer reads bus destinations when a send menu opens. Rendering a closed send menu no longer copies every bus and its name for each send slot.

The menu reads the bus list again when New Bus is selected, preserving the existing naming and routing behavior.

All seven Windows checks completed with 1,459 reported test passes, zero failures, and one ignored test. Validation included #1081 and its AudioSuite slider correction; this pull request contains only the send-menu change.

Summary by CodeRabbit

  • Chores
    • Bus destination selection and new-bus naming now avoid an unnecessary copy of the session’s bus list. Filtering, name-conflict checks, and send creation remain unchanged. This internal efficiency improvement does not change the available destinations or how users create sends.

@coderabbitai

coderabbitai Bot commented Oct 27, 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 8176230 and 80e384a.

Files selected for processing: 1
  • crates/ui-egui/src/mix_window/sends.rs

Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review.

Walkthrough

The send route menu no longer clones the session bus list. It borrows the list for destination filtering and new-bus name selection.

Changes: Send route menu

Layer / File(s) Summary
Borrow the session bus list
crates/ui-egui/src/mix_window/sends.rs
Destination filtering and new-bus name checks read the session bus list by reference. The menu no longer clones the list.

Estimated code review effort: 1 (Trivial) | ~5 minutes

Change: Refactor

Suggested reviewers: aiongg

Merge Risk: Minimal · up to 80e38

The focused borrowing change is mergeable with normal checks.

Architecture risk: Low · up to 81762

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; 1 changed file maps to changed impact.

Before / after behavior

  • observed — Modified behavior in crates/ui-egui/src/mix_window/sends.rs: Removed the owned clone of the bus list previously created before the menu header; the header now obtains the list by borrowing it from the session.
  • observed — Modified behavior in crates/ui-egui/src/mix_window/sends.rs: The destination search now reads the session’s bus list by reference instead of using the earlier cloned list.
  • observed — Modified behavior in crates/ui-egui/src/mix_window/sends.rs: The new-bus naming check now borrows the session’s bus list rather than cloning it.
Pre-merge checks: Passed 5
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 main change: send destinations are read only while the menu is open.
Docstring Coverage Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat…
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.
Full details: Docstring Coverage

Explanation

Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 1 functions across 1 files.

Finishing Touches
Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Warning

Some tools did not complete. Review the errors below.

Clippy (1.99.0)

Clippy execution failed

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

@FredApps FredApps left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ReviewerA — source-read COMMENT (not an approval)

I read the whole diff: the bus list is no longer cloned each frame; it's borrowed inside the open-menu closure and again in the new-bus branch. The change looks right and removes a per-frame allocation. The borrows end before any mutation of app in the visible code, but only a compile confirms it.

I ran nothing and have no exact-head CI result for this PR, so this is not an approval.

Status: waiting_author.

@dotCooCoo

Copy link
Copy Markdown
Contributor Author

Merged main (e5d3174) into the branch; no conflicts. Clippy, the soundcraft-ui-egui tests (421 passed) and cargo check --workspace --tests pass on the merged head 80e384a.

@dotCooCoo

Copy link
Copy Markdown
Contributor Author

@FredApps The borrows compile: CI's gate / gate (ubuntu-24.04) (cargo xtask ci) passed on the current head 80e384a, https://github.com/storytold/soundcraft/actions/runs/41541312661/job/125605881104. No code changed after your review besides the merge of main (e5d3174).

This branch has not been deployed

No deployments
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