toolchain #129

Merged

navicore merged 1 commit from toolchain into main

2026-04-11 20:29:55 +00:00

navicore commented

2026-04-11 20:19:47 +00:00

(Migrated from github.com)

No description provided.

claude[bot] commented

2026-04-11 20:27:36 +00:00

(Migrated from github.com)

PR Review: Toolchain Modernization

Good overall direction — consolidating build logic into justfile so CI and local dev use the same recipes is a solid pattern. The Rust idiom cleanup is also welcome. A few things worth addressing:

Bug: Redundant assignments in `test_buffer_size_limit` (src/player/telemetry.rs)

The refactoring was only half-completed. The struct initializer now includes buffer_size and capture_interval_ms, but the old free-standing assignments were left in place:

let mut config = TelemetryConfig {
    enabled: true,
    buffer_size: 2,          // set here
    capture_interval_ms: 0,  // set here
    ..Default::default()
};
config.buffer_size = 2;          // redundant
config.capture_interval_ms = 0;  // redundant

The redundant lines below the struct literal should be removed to match the pattern used consistently in the other two refactored tests.

CI: macOS only runs on `push` to `main`, not on PRs

ci-linux.yml triggers on pull_request (pre-merge), but ci-macos.yml triggers on push to main (post-merge). This means macOS-only failures won't be caught until after the code lands on main.

# ci-linux.yml
on:
  pull_request:
    branches: [main]   ✓ catches failures before merge

# ci-macos.yml
on:
  push:
    branches: [main]   ✗ only runs after merge

If the intent is to save macOS runner minutes on PRs, that's a reasonable trade-off — but it's worth making explicit in a comment. If it was unintentional, adding pull_request to the macOS trigger is the fix.

Performance: `target/` not cached, but CI runs a full release build

The cargo cache covers ~/.cargo/registry and ~/.cargo/git (the source registry), but not the target/ directory. Since just ci includes cargo build --locked --release --all-features, every CI run will recompile the entire project from scratch. For a project with audio/UI dependencies this will be slow.

Consider adding target-dir caching, or splitting the ci recipe so that the release build step (which isn't strictly needed for correctness checks) is optional / separated from the PR gate.

Minor: `rust-analyzer` in `rust-toolchain.toml` pulls into every toolchain install

components = ["rustfmt", "clippy", "rust-analyzer"]

rust-analyzer is an IDE tool and isn't needed for CI or CLI builds. While dtolnay/rust-toolchain uses its own components: input (not rust-toolchain.toml) so CI isn't directly affected, local rustup installs triggered by rust-toolchain.toml will pull the component unnecessarily on machines that don't use it. Consider removing it, or documenting that it's there for IDE users.

Positives worth keeping

Inline format args ({error_msg} instead of "{}", error_msg) — correct Rust 2021 idiom, enforced by the clippy::uninlined-format-args lint already in the project.
assert!(x) / assert!(!x) over assert_eq!(x, true/false) — proper idiomatic usage.
..Default::default() struct initialization pattern is cleaner.
Pinning the toolchain in rust-toolchain.toml and mirroring the same version in CI workflows eliminates "works on my rustc" drift.
The justfile comment block clearly explains the source-of-truth intent — helpful for new contributors.

## PR Review: Toolchain Modernization Good overall direction — consolidating build logic into `justfile` so CI and local dev use the same recipes is a solid pattern. The Rust idiom cleanup is also welcome. A few things worth addressing: --- ### Bug: Redundant assignments in `test_buffer_size_limit` (src/player/telemetry.rs) The refactoring was only half-completed. The struct initializer now includes `buffer_size` and `capture_interval_ms`, but the old free-standing assignments were left in place: ```rust let mut config = TelemetryConfig { enabled: true, buffer_size: 2, // set here capture_interval_ms: 0, // set here ..Default::default() }; config.buffer_size = 2; // redundant config.capture_interval_ms = 0; // redundant ``` The redundant lines below the struct literal should be removed to match the pattern used consistently in the other two refactored tests. --- ### CI: macOS only runs on `push` to `main`, not on PRs `ci-linux.yml` triggers on `pull_request` (pre-merge), but `ci-macos.yml` triggers on `push` to `main` (post-merge). This means macOS-only failures won't be caught until after the code lands on `main`. ```yaml # ci-linux.yml on: pull_request: branches: [main] ✓ catches failures before merge # ci-macos.yml on: push: branches: [main] ✗ only runs after merge ``` If the intent is to save macOS runner minutes on PRs, that's a reasonable trade-off — but it's worth making explicit in a comment. If it was unintentional, adding `pull_request` to the macOS trigger is the fix. --- ### Performance: `target/` not cached, but CI runs a full release build The cargo cache covers `~/.cargo/registry` and `~/.cargo/git` (the source registry), but not the `target/` directory. Since `just ci` includes `cargo build --locked --release --all-features`, every CI run will recompile the entire project from scratch. For a project with audio/UI dependencies this will be slow. Consider adding target-dir caching, or splitting the `ci` recipe so that the release build step (which isn't strictly needed for correctness checks) is optional / separated from the PR gate. --- ### Minor: `rust-analyzer` in `rust-toolchain.toml` pulls into every toolchain install ```toml components = ["rustfmt", "clippy", "rust-analyzer"] ``` `rust-analyzer` is an IDE tool and isn't needed for CI or CLI builds. While `dtolnay/rust-toolchain` uses its own `components:` input (not `rust-toolchain.toml`) so CI isn't directly affected, local `rustup` installs triggered by `rust-toolchain.toml` will pull the component unnecessarily on machines that don't use it. Consider removing it, or documenting that it's there for IDE users. --- ### Positives worth keeping - Inline format args (`{error_msg}` instead of `"{}", error_msg`) — correct Rust 2021 idiom, enforced by the `clippy::uninlined-format-args` lint already in the project. - `assert!(x)` / `assert!(!x)` over `assert_eq!(x, true/false)` — proper idiomatic usage. - `..Default::default()` struct initialization pattern is cleaner. - Pinning the toolchain in `rust-toolchain.toml` and mirroring the same version in CI workflows eliminates "works on my rustc" drift. - The `justfile` comment block clearly explains the source-of-truth intent — helpful for new contributors.