Arbitrate concurrent macOS Now Playing sessions - #1
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe project replaces the Perl and Rust ChangesEmbedded macOS media session adapter
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CargoBuild
participant MediaRemoteSource
participant EmbeddedHelper
participant MediaRemote
participant TrackSnapshot
CargoBuild->>EmbeddedHelper: compile and ad-hoc sign
MediaRemoteSource->>EmbeddedHelper: launch and read JSON lines
EmbeddedHelper->>MediaRemote: discover players and request session data
MediaRemote-->>EmbeddedHelper: return session metadata and playback state
EmbeddedHelper-->>MediaRemoteSource: emit session payloads
MediaRemoteSource->>TrackSnapshot: select candidate and update playback state
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
|
Stop-switch follow-up pushed in Root cause: some MediaRemote sessions retain The adapter now marks player-scoped playback state as resolved explicitly, runs those requests on a dedicated serial queue, and excludes unresolved/error states instead of reviving the stale rate. Added a regression proving that an unresolved newer session falls back to another resolved playing session. Validation: Rust 1.88 locked tests, Clippy with warnings denied, release build, arm64 helper signature verification, and x86_64 helper build/signature/architecture verification all pass. |
|
@codex review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
src/media.rs (1)
2-48: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGate the shared items for non-macOS builds.
Clippy reports
POSITION_EDGE_TOLERANCE_SECONDS,PlaybackCandidate, andselect_playback_candidateas unused.mod platformis the only non-test consumer, and it is macOS-only, so a non-macOS check build warns on all three items. If CI runs Clippy with-D warningson any non-macOS target, the build fails.Add a
cfggate that keeps the items available for the macOS build and for tests.♻️ Proposed change
+#[cfg(any(target_os = "macos", test))] const POSITION_EDGE_TOLERANCE_SECONDS: f64 = 2.0; +#[cfg(any(target_os = "macos", test))] #[derive(Clone, Debug, Deserialize)] #[serde(rename_all = "camelCase")] struct PlaybackCandidate {+#[cfg(any(target_os = "macos", test))] fn select_playback_candidate(candidates: &[PlaybackCandidate]) -> Option<&PlaybackCandidate> {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/media.rs` around lines 2 - 48, Add a non-macOS exclusion gate to POSITION_EDGE_TOLERANCE_SECONDS, PlaybackCandidate, and select_playback_candidate so they compile only for macOS or tests. Ensure the existing macOS behavior and test availability remain unchanged.Source: Linters/SAST tools
README.md (1)
30-30: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRecord the unresolved-session rule.
The paragraph states that only sessions that macOS reports as playing are considered. The adapter applies a stricter rule.
select_playback_candidateinsrc/media.rsrequires bothplayingandplaying_resolved. The helper insrc/media_sessions.mleavesplayingResolved=NOwhen the per-player playback request does not answer within the refresh timeout, so such a session is excluded even when it is playing.Add one sentence that states that a session with an unresolved playback state is excluded.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` at line 30, Update the README paragraph describing eligible Now Playing sessions to add that sessions with an unresolved playback state are excluded, matching the `select_playback_candidate` requirement for both `playing` and `playing_resolved`.src/media_sessions.pl (1)
13-14: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the XSUB calling-convention assumption.
dl_install_xsubregisters the symbol as an XSUB. Perl calls it with the XS signaturevoid (*)(pTHX_ CV*), butchroma_media_sessions_streamis declared asvoid (void)insrc/media_sessions.m. The call works only because the extra arguments are ignored on arm64 and x86_64, and because the function never returns a value. Add a short comment that records this contract. A future change of the helper signature to return a value or to read arguments will break silently.📝 Proposed comment
+# The helper is installed as an XSUB. Perl invokes it with the XS signature +# void (*)(pTHX_ CV *), so chroma_media_sessions_stream must stay a +# no-argument, no-return-value function that blocks on its own run loop. DynaLoader::dl_install_xsub("main::chroma_media_sessions_stream", $symbol); chroma_media_sessions_stream();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/media_sessions.pl` around lines 13 - 14, Add a concise comment immediately before the dl_install_xsub registration or chroma_media_sessions_stream call documenting that Perl invokes the XSUB with pTHX and CV* arguments while the helper is void(void), and that correctness relies on ignoring those arguments and returning no value. Keep the existing registration and invocation unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/media_sessions.m`:
- Around line 32-40: Update integerProperty to invoke objc_msgSend with an int
return type, then explicitly widen that int to the function’s long result.
Preserve the existing selector-presence handling and ensure the
processIdentifier value used to build stableID remains consistent across
refreshes.
- Around line 125-128: Fix ARC ownership for both dynamic alloc/init calls in
the media-session path, including the objc_msgSend invocation using
initWithOrigin:client:player:. Expose these calls through suitable Objective-C
declarations or accurately annotated function types so ARC models the retained
initializer result and consumed self; do not use
CFBridgingRetain/CFBridgingRelease as a workaround.
In `@src/media.rs`:
- Around line 353-362: Update the snapshot handling in the surrounding run flow
to call ArtworkDelivery::should_deliver before cloning received.snapshot. When
delivery is allowed, clone the snapshot with its artwork; otherwise use
TrackSnapshot::clone_without_artwork so the decoded DynamicImage is not copied.
Preserve elapsed_time and track_key handling while avoiding artwork cloning in
the common rejection path.
---
Nitpick comments:
In `@README.md`:
- Line 30: Update the README paragraph describing eligible Now Playing sessions
to add that sessions with an unresolved playback state are excluded, matching
the `select_playback_candidate` requirement for both `playing` and
`playing_resolved`.
In `@src/media_sessions.pl`:
- Around line 13-14: Add a concise comment immediately before the
dl_install_xsub registration or chroma_media_sessions_stream call documenting
that Perl invokes the XSUB with pTHX and CV* arguments while the helper is
void(void), and that correctness relies on ignoring those arguments and
returning no value. Keep the existing registration and invocation unchanged.
In `@src/media.rs`:
- Around line 2-48: Add a non-macOS exclusion gate to
POSITION_EDGE_TOLERANCE_SECONDS, PlaybackCandidate, and
select_playback_candidate so they compile only for macOS or tests. Ensure the
existing macOS behavior and test availability remain unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5a114391-00e8-418e-ba52-1139ec659d19
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
Cargo.tomlREADME.mdbuild.rssrc/media.rssrc/media_sessions.msrc/media_sessions.pl
![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: f5388ee432
ℹ️ 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".
|
Review trace for https://github.com/hauntedfail/Codex-Micro-Chroma/pull/1#pullrequestreview-4906401280\n\nClassification: must_fix; fixed in 92134aa. I verified and addressed all three inline findings plus all three review-body nitpicks: non-macOS/test cfg gates, the unresolved-session/incomplete-refresh README contract, and the Perl XSUB calling-convention comment. Validation: cargo fmt --all --check; cargo clippy --all-targets --all-features -- -D warnings; cargo test --all-targets --all-features (51 passed); cargo build --all-targets --all-features; git diff --check. |
|
Review trace for https://github.com/hauntedfail/Codex-Micro-Chroma/pull/1#pullrequestreview-4906428549\n\nClassification: must_fix; fixed in 92134aa. I verified and addressed all five child findings: partial refresh publication, pause/resume identity, playback-query error handling, repeated artwork encoding, and helper EOF/read-error propagation. Validation: cargo fmt --all --check; cargo clippy --all-targets --all-features -- -D warnings; cargo test --all-targets --all-features (51 passed); cargo build --all-targets --all-features; git diff --check. |
|
@coderabbitai review Please re-review the current head 92134aa. All previously reported items were addressed, replied to inline, and resolved; the full local verification suite passes. |
|
|
|
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 92134aac84
ℹ️ 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".
|
Review trace for #1 (review) Classification: must_fix; fixed in 20619a5. Both child findings were addressed: last-playing-date request errors now invalidate incomplete refreshes, and unchanged selected artwork reuses exact-key decoded cache state without per-poll image cloning. Validation: cargo fmt --all --check; cargo clippy --all-targets --all-features -- -D warnings; cargo test --all-targets --all-features (55 passed); cargo build --all-targets --all-features; git diff --check. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 20619a5074
ℹ️ 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".
|
Resolved the aggregate Codex review at commit 2de5f89. All three inline findings were fixed, replied to individually, reacted to, and resolved: stopped-client date failures no longer freeze healthy playback, playing sessions require metadata resolution, and non-finite MediaRemote numbers are filtered before JSON serialization. The broader audit also removed the unsafe Perl XSUB bridge, hardened helper startup/environment/tool resolution, added pinned least-privilege macOS PR CI, and removed stale runtime documentation. Local validation passed on Rust 1.88: format, Clippy with warnings denied, 58 tests, doc tests, debug build, arm64/x86_64 release builds, universal binary signing/version smoke, helper ready/environment smoke, RustSec audit (1,211 advisories / 80 dependencies, zero vulnerabilities), and diff/secret checks. CI is now running on this head. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/media.rs (2)
522-542: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDeduplicate the three failure arms of the ready handshake.
Each arm repeats
child.kill(),child.wait(), andreader.join(), then callsbail!. Only the message differs. A future change to the shutdown sequence must be applied three times.♻️ Proposed refactor
- match ready_rx.recv_timeout(HELPER_READY_TIMEOUT) { - Ok(Ok(())) => {} - Ok(Err(error)) => { - let _ = child.kill(); - let _ = child.wait(); - let _ = reader.join(); - bail!("{error}"); - } - Err(mpsc::RecvTimeoutError::Timeout) => { - let _ = child.kill(); - let _ = child.wait(); - let _ = reader.join(); - bail!("MediaRemote session helper did not become ready within {HELPER_READY_TIMEOUT:?}"); - } - Err(mpsc::RecvTimeoutError::Disconnected) => { - let _ = child.kill(); - let _ = child.wait(); - let _ = reader.join(); - bail!("MediaRemote session helper reader stopped before ready"); - } - } + let ready_error = match ready_rx.recv_timeout(HELPER_READY_TIMEOUT) { + Ok(Ok(())) => None, + Ok(Err(error)) => Some(error), + Err(mpsc::RecvTimeoutError::Timeout) => Some(format!( + "MediaRemote session helper did not become ready within {HELPER_READY_TIMEOUT:?}" + )), + Err(mpsc::RecvTimeoutError::Disconnected) => Some( + "MediaRemote session helper reader stopped before ready".to_owned(), + ), + }; + if let Some(error) = ready_error { + let _ = child.kill(); + let _ = child.wait(); + let _ = reader.join(); + bail!("{error}"); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/media.rs` around lines 522 - 542, Deduplicate the failure handling in the ready handshake match around ready_rx.recv_timeout: have each failure case produce its distinct error message, then perform child.kill(), child.wait(), and reader.join() once in shared control flow before bailing. Preserve the existing messages and successful Ok(Ok(())) behavior.
496-499: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMalformed session lines are dropped without any signal.
If the helper payload schema drifts,
serde_json::from_str::<SessionPayload>fails and the loop silently skips the line. The Now Playing state then freezes at the last valid payload with no diagnostic. The stopped path at Lines 508-518 reports its reason on stderr; this path reports nothing.Log the first parse failure, or log at a throttled rate, so schema drift is visible.
♻️ Proposed change
Some(Ok(line)) => { - let Ok(payload) = serde_json::from_str::<SessionPayload>(&line) else { - continue; - }; + let payload = match serde_json::from_str::<SessionPayload>(&line) { + Ok(payload) => payload, + Err(error) => { + if !reported_parse_error { + reported_parse_error = true; + eprintln!( + "MediaRemote session helper sent an unparsable payload: {error}" + ); + } + continue; + } + };Declare
let mut reported_parse_error = false;before the loop.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/media.rs` around lines 496 - 499, Add a mutable parse-error reporting guard before the session-line loop, then update the `serde_json::from_str::<SessionPayload>` failure branch to log the first malformed payload error to stderr and mark it reported, while continuing to skip invalid lines without repeatedly logging every failure..github/workflows/ci.yml (1)
48-52: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider verifying the embedded helper architecture in CI.
build.rscompiles and ad-hoc signscodex_micro_chroma_media_sessionsper target architecture. The workflow builds both slices but does not assert that the embedded helper matches the requested target. A cross-arch mismatch would only appear at runtime on a user machine.Add a check that runs
lipo -infoorfileon the helper intarget/<triple>/release/build/*/out/codex_micro_chroma_media_sessions, andcodesign -dvto confirm the ad-hoc signature.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml around lines 48 - 52, Update the “Build arm64 release binary” and “Build x86_64 release binary” CI steps to inspect the generated codex_micro_chroma_media_sessions under each target’s release build output, using lipo -info or file to verify the expected architecture and codesign -dv to verify its ad-hoc signature. Ensure the checks locate the helper within the build/*/out directory and fail the workflow on mismatches.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/media_sessions.m`:
- Around line 99-121: Update the candidate-processing logic in the session
helper around the playing/metadataResolved checks so lastPlayingDateError no
longer sets valid to NO. When that flag is present, treat lastPlayingDate as
unavailable by removing or omitting its value from publicCandidate, allowing the
Rust selector to fall back to elected and stableId; retain the strict
invalidation for metadataResolved failures and existing publication behavior.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 48-52: Update the “Build arm64 release binary” and “Build x86_64
release binary” CI steps to inspect the generated
codex_micro_chroma_media_sessions under each target’s release build output,
using lipo -info or file to verify the expected architecture and codesign -dv to
verify its ad-hoc signature. Ensure the checks locate the helper within the
build/*/out directory and fail the workflow on mismatches.
In `@src/media.rs`:
- Around line 522-542: Deduplicate the failure handling in the ready handshake
match around ready_rx.recv_timeout: have each failure case produce its distinct
error message, then perform child.kill(), child.wait(), and reader.join() once
in shared control flow before bailing. Preserve the existing messages and
successful Ok(Ok(())) behavior.
- Around line 496-499: Add a mutable parse-error reporting guard before the
session-line loop, then update the `serde_json::from_str::<SessionPayload>`
failure branch to log the first malformed payload error to stderr and mark it
reported, while continuing to skip invalid lines without repeatedly logging
every failure.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a8673ef3-5e9f-44ec-bcb8-91ffb950ac7a
📒 Files selected for processing (6)
.github/workflows/ci.ymlREADME.mdbuild.rssrc/main.rssrc/media.rssrc/media_sessions.m
🚧 Files skipped from review as they are similar to previous changes (2)
- build.rs
- README.md
|
Addressed the complete CodeRabbit review at #1 (review) in signed commits e353e6d and 26647eb. The Major inline finding now degrades lastPlayingDate request failures to a missing date and falls back to macOS election/stable ID without weakening metadata or playback-state completeness. The three review-body quality items are also handled: the reader reports the first malformed post-ready payload, pre-ready termination uses one shared cleanup path, and CI asserts exactly one helper per target plus lipo architecture and codesign validity. Validation passed on Rust 1.88 with Clippy warnings denied, 60 tests, doc tests, debug and both release builds, strict Objective-C compile, and local arm64/x86_64 helper verification. The inline thread was replied to, reacted to, and resolved. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 26647ebb4f
ℹ️ 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 review Final re-review request for current head 5d45bd2. Since the previous request, scoped requestIsPlaying errors now degrade only that candidate to playingResolved=false instead of freezing the whole snapshot; global timeouts and successful scoped states remain unchanged. CI is green with 62 local tests and both helper architecture/signature checks. Please confirm no remaining correctness, security, performance, Objective-C/MediaRemote, or CI findings. |
|
@codex review\n\nFinal exact-head review requested for e55f249. The only findings on the previous exact head were that the Objective-C smoke harness was not executed in CI; this commit adds the strict compile/run gate. Please review the full PR for any remaining correctness, security, availability, lifecycle, arbitration, resource-bound, CI, or code-quality problem. |
|
Codex Review: Didn't find any major issues. Chef's kiss. 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". |
|
Review 4912979688 aggregate resolution trace All five findings were adjudicated as valid and fixed on the current head 0cc988a:
Each original comment now has an inline fix reply and +1 reaction, and each review thread is resolved. Current CI is green. |
|
Review 4913091571 aggregate resolution trace Both findings were adjudicated as valid and fixed on the current head 0cc988a:
Each original comment now has an inline fix reply and +1 reaction, and both review threads are resolved. Current CI is green. |
|
@codex review Please perform a fresh review of exact head 0cc988a. The seven delayed findings from reviews 4912979688 and 4913091571 have been fixed, replied to inline, reacted to, and resolved. Please re-check correctness, security/resource bounds, stop-to-fallback behavior, multi-session arbitration, and CI coverage. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0cc988a546
ℹ️ 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".
|
Review 4913234070 aggregate resolution trace The sole finding was adjudicated as valid and fixed on current head 7ed7336:
The original comment has an inline fix reply and +1 reaction, and its review thread is resolved. Regression coverage plus Rust 1.88 fmt, Clippy with warnings denied, all 79 tests, Objective-C smoke, and dual-architecture CI are being revalidated on this head. |
|
@codex review Please perform a fresh clean-slate review of exact head 7ed7336. The latest P2 from review 4913234070 is fixed with a focused regression, replied to inline, reacted to, and resolved. Please re-check the full PR, especially artwork recovery delivery state, stop/fallback behavior, multi-session arbitration, security/resource bounds, and CI coverage. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/media_sessions.m (1)
349-370: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
rankedPlayingCandidatesexists only for the smoke build and duplicates the production ranking path.Production selection uses
insertTopRecordwith streaming inserts. The smoke test exercises both, butrankedPlayingCandidatesis a second implementation of the same policy that no production code calls. If the ranking policy changes later, the two can diverge and the test will still pass.Consider expressing the batch-order tests at Lines 814-847 through
insertTopRecord, then removingrankedPlayingCandidates.Also applies to: 814-847
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/media_sessions.m` around lines 349 - 370, Replace the smoke-only batch ranking tests that use rankedPlayingCandidates with tests exercising the production insertTopRecord streaming path, preserving the existing ordering and limit assertions. Once those tests use insertTopRecord, remove the duplicate rankedPlayingCandidates helper and its conditional compilation block.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/media_sessions.m`:
- Around line 628-652: Update the scoped-playing callback in the request path
around isPlayingSelector so an error invokes getInfoForPlayer(playerPath, NO,
queue, ...) with resolvePlayingFromRate enabled, using the same metadata
completion and dispatch-group cleanup as the existing fallback. Preserve the
direct playing assignment for successful requests and ensure each branch leaves
batchGroup exactly once, allowing failed scoped requests to set playingResolved
through the playback-rate fallback.
---
Nitpick comments:
In `@src/media_sessions.m`:
- Around line 349-370: Replace the smoke-only batch ranking tests that use
rankedPlayingCandidates with tests exercising the production insertTopRecord
streaming path, preserving the existing ordering and limit assertions. Once
those tests use insertTopRecord, remove the duplicate rankedPlayingCandidates
helper and its conditional compilation block.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e496cad7-c800-4ebe-9dbb-4ee19d69d4e5
📒 Files selected for processing (4)
.github/workflows/ci.ymlREADME.mdsrc/media.rssrc/media_sessions.m
🚧 Files skipped from review as they are similar to previous changes (3)
- README.md
- src/media.rs
- .github/workflows/ci.yml
|
Review 4913261195 aggregate adjudication trace The sole comment 3763799236 was adjudicated as non-actionable intent mismatch on current head 7ed7336. Falling back to metadata playbackRate after an implemented scoped isPlaying API returns an error can revive stale playing state and regress immediate stop/fallback behavior. The intentional contract is: missing scoped selector uses metadata fallback; scoped error leaves only that client unresolved, Rust excludes it and selects another resolved active session or clears, then retries on the next refresh. README.md and three focused unresolved-scoped-state regressions cover this behavior. The original comment has an inline rationale, -1 reaction, and resolved thread. No code change was made for this invalid suggestion. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7ed73366a5
ℹ️ 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".
|
Review 4913288880 aggregate resolution trace The sole finding 3763820505 was adjudicated as valid and fixed on current head 5129acd. Artwork is now selected winner-first from exact pre-allocation base64/cache costs, encoded only when both bounded budgets admit it, and retained in the cache without stale-entry or duplicate-ID oversubscription. The original comment has an inline fix reply and +1 reaction, and its thread is resolved. Production/smoke Objective-C -Werror builds, budget/cache regressions, Rust 1.88 fmt/Clippy/all 79 tests, and cargo-audit pass. |
|
@codex review Please perform another fresh clean-slate review of exact head 5129acd. The latest P2 from review 4913288880 is fixed with pre-allocation winner-first artwork budgeting and cache regressions, replied to inline, reacted to, and resolved. Please re-check the full PR, especially phase-2 async lifetime, resource/accounting invariants, stop/fallback and arbitration, security, and CI. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5129acd99a
ℹ️ 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".
|
Review 4913392542 aggregate resolution trace Both findings were valid and are fixed on current head d4e69c2:
Both comments have inline fix replies and +1 reactions, and both threads are resolved. Rust 1.88 fmt/Clippy/all 79 tests, Objective-C arm64/x86_64 smoke -Werror, cargo-audit, remote SHA, and GitHub signature verification pass. |
|
@codex review Please perform a final fresh clean-slate review of exact head d4e69c2. All prior valid findings have been fixed, replied to inline, reacted to, and resolved. Please re-check the complete PR for correctness, security/resource bounds, duplicate/session arbitration, stop-to-fallback behavior, metadata/artwork recovery, and CI coverage. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d4e69c2759
ℹ️ 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".
|
Review 4913539143 aggregate adjudication trace Current head: 7784252.
Rust 1.88 fmt/Clippy/all 79 tests, Objective-C arm64/x86_64 smoke, remote SHA, and GitHub signature verification pass. |
|
@codex review Please perform a final fresh clean-slate review of exact head 7784252. The valid missing-playbackRate fallback issue from review 4913539143 is fixed with focused smoke coverage; the date-error comment was adjudicated against the documented availability contract. All threads are resolved. Please re-check the complete PR for any remaining actionable correctness, security/resource, arbitration, stop/fallback, metadata/artwork, or CI issues. |
|
Codex Review: Didn't find any major issues. Nice work! 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". |
|
Codex Review: Didn't find any major issues. Hooray! 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
Summary
/usr/bin/perlboundarylastPlayingDate, then the OS-elected player path, then a stable identifierWhy
The previous adapter exposed only macOS's single elected Now Playing item. That made concurrent Spotify, Music, browser, or other playback sessions invisible to the application, so it could not deterministically move to another active source after the selected source stopped.
This change derives priority from each refresh of macOS-owned session state and does not persist a separate application playback-order history.
Validation
cargo fmt --all -- --checkcargo clippy --all-targets --all-features -- -D warningscargo test --all-targets --all-featurescargo build --release/usr/bin/perlhelper startup against the current macOS MediaRemote service (no active playback sessions at probe time)Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores