added command diagnostic#33854
Conversation
📝 WalkthroughWalkthroughThe pull request refactors the playback command infrastructure by splitting the monolithic 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsLinked repositories: Your configuration references 1 linked repositories, but your current plan allows 0. Analyzed ``, skipped 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 |
![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
🤖 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 @.gitmodules:
- Line 3: The muse_framework submodule URL in the .gitmodules file has been
changed from the official musescore repository to a personal fork, which
violates the CI contract enforced by check_submodules.yml. Revert the url
configuration for the muse_framework submodule from
git@github.com:igorkorsukov/muse_framework.git back to the official repository
URL https://github.com/musescore/muse_framework.git to restore CI compliance.
In `@muse`:
- Line 1: The submodule URL in `.gitmodules` pointing to a personal fork
conflicts with the hardcoded EXPECTED_URL in the
`.github/workflows/check_submodules.yml` workflow file which is set to the
official repository URL. To resolve this CI failure, either revert the submodule
URL in `.gitmodules` back to the official repository
(https://github.com/musescore/muse_framework.git), or alternatively update the
EXPECTED_URL variable in the workflow file to match the personal fork URL and
provide explicit justification for this supply-chain decision in the PR
description.
In `@src/playback/internal/playbackcommandsstate.cpp`:
- Around line 40-50: The state invalidation is incomplete because
updateCommandStates() is not triggered when playback status changes or when the
interactive/project closes. Add two additional signal connections after the
existing ones: connect to playbackController()->isPlayingChanged() to catch
playback state transitions, and connect to interactive()->closed() to catch page
close events. Both connections should follow the same pattern as the existing
signal handlers and invoke updateCommandStates() to ensure command states remain
synchronized with the actual playback and project state.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7651528c-8cf1-4f28-abd5-60dcf207f5dc
📒 Files selected for processing (11)
.gitmodulesmusesrc/appshell/qml/MuseScore/AppShell/appmenumodel.cppsrc/playback/CMakeLists.txtsrc/playback/internal/playbackcommandsregister.cppsrc/playback/internal/playbackcommandsregister.hsrc/playback/internal/playbackcommandsstate.cppsrc/playback/internal/playbackcommandsstate.hsrc/playback/internal/playbackcontroller.cppsrc/playback/playbackcommands.hsrc/playback/playbackmodule.cpp
| [submodule "muse_framework"] | ||
| path = muse | ||
| url = https://github.com/musescore/muse_framework.git | ||
| url = git@github.com:igorkorsukov/muse_framework.git |
There was a problem hiding this comment.
⛔ Critical: Submodule URL points to personal fork and breaks CI contract.
Line 3 changes the muse_framework submodule URL from the official https://github.com/musescore/muse_framework.git to a personal fork git@github.com:igorkorsukov/muse_framework.git. This violates the CI contract enforced by .github/workflows/check_submodules.yml, which explicitly validates that the URL must match the musescore official repository. The check_muse_framework CI checks are already failing with the error: "muse_framework submodule URL must be https://github.com/musescore/muse_framework.git, found git@github.com:igorkorsukov/muse_framework.git. Did you accidentally leave it pointing at a fork?"
This change will block the PR from merging. Given that the PR's primary objective is refactoring playback commands (unrelated to submodule configuration), this appears to be an accidental development artifact.
🔧 Proposed fix: Revert to official repository URL
[submodule "muse_framework"]
path = muse
- url = git@github.com:igorkorsukov/muse_framework.git
+ url = https://github.com/musescore/muse_framework.git📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| url = git@github.com:igorkorsukov/muse_framework.git | |
| [submodule "muse_framework"] | |
| path = muse | |
| url = https://github.com/musescore/muse_framework.git |
🤖 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 @.gitmodules at line 3, The muse_framework submodule URL in the .gitmodules
file has been changed from the official musescore repository to a personal fork,
which violates the CI contract enforced by check_submodules.yml. Revert the url
configuration for the muse_framework submodule from
git@github.com:igorkorsukov/muse_framework.git back to the official repository
URL https://github.com/musescore/muse_framework.git to restore CI compliance.
| @@ -1 +1 @@ | |||
| Subproject commit e573f8bc57ab1373d0a80b013e3d3fed932fb7f4 | |||
| Subproject commit e04ff5e0c16e475009cf0ec6daa182d201bcf513 | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify that the pinned muse commit exists in the personal fork.
# Test: Check if the commit is reachable from the personal fork's default branch.
# Expect: The commit should be listed; if not found, the clone will fail.
COMMIT="e04ff5e0c16e475009cf0ec6daa182d201bcf513"
FORK_URL="git@github.com:igorkorsukov/muse_framework.git"
echo "Checking if commit ${COMMIT} exists in personal fork..."
git ls-remote "${FORK_URL}" | grep -q "^${COMMIT}" && echo "✓ Commit found in personal fork" || echo "✗ Commit NOT found in personal fork"
# Also check if it's reachable from main/master
for branch in main master develop; do
echo "Checking ${branch} branch..."
git ls-remote "${FORK_URL}" "refs/heads/${branch}" | awk '{print $1}' | xargs -I {} bash -c "git merge-base --is-ancestor ${COMMIT} {} 2>/dev/null && echo ' ✓ Commit is an ancestor of ${branch}' || echo ' ✗ Commit is NOT an ancestor of ${branch}'"
doneRepository: musescore/MuseScore
Length of output: 909
🏁 Script executed:
# First, find and examine the .gitmodules file to confirm the submodule configuration
find . -name ".gitmodules" -type f -exec cat {} \;Repository: musescore/MuseScore
Length of output: 159
🏁 Script executed:
# Check the CI workflow file mentioned in the review
cat .github/workflows/check_submodules.yml 2>/dev/null || echo "File not found"Repository: musescore/MuseScore
Length of output: 2495
🏁 Script executed:
# Try to verify the commit using HTTPS instead of SSH, and check the official upstream
COMMIT="e04ff5e0c16e475009cf0ec6daa182d201bcf513"
echo "Attempting to verify commit in official upstream via HTTPS..."
git ls-remote "https://github.com/musescore/muse_framework.git" | grep "^${COMMIT}" && echo "✓ Commit found in official upstream" || echo "✗ Commit NOT found in official upstream"
echo -e "\nTrying personal fork via HTTPS..."
git ls-remote "https://github.com/igorkorsukov/muse_framework.git" | head -20Repository: musescore/MuseScore
Length of output: 1641
The CI workflow will fail: .github/workflows/check_submodules.yml explicitly rejects fork URLs.
The .gitmodules file has been changed to point to the personal fork (git@github.com:igorkorsukov/muse_framework.git), but the workflow step "Verify muse_framework submodule URL" in .github/workflows/check_submodules.yml is hardcoded with EXPECTED_URL: https://github.com/musescore/muse_framework.git (the official repository). The workflow explicitly rejects mismatches with the error:
muse_framework submodule URL must be ${EXPECTED_URL}, found ${ACTUAL_URL}. Did you accidentally leave it pointing at a fork?
The commit e04ff5e0c16e475009cf0ec6daa182d201bcf513 does exist in both the official upstream (refs/pull/89/head) and the personal fork (refs/heads/w/rcmd/rcmd_step4), so commit availability is not the issue.
To merge this PR, either:
- Revert the
.gitmodulesURL to the official repository, or - Update the
EXPECTED_URLin the workflow and explicitly justify this supply-chain decision in the PR description
🤖 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 `@muse` at line 1, The submodule URL in `.gitmodules` pointing to a personal
fork conflicts with the hardcoded EXPECTED_URL in the
`.github/workflows/check_submodules.yml` workflow file which is set to the
official repository URL. To resolve this CI failure, either revert the submodule
URL in `.gitmodules` back to the official repository
(https://github.com/musescore/muse_framework.git), or alternatively update the
EXPECTED_URL variable in the workflow file to match the personal fork URL and
provide explicit justification for this supply-chain decision in the PR
description.
| globalContext()->currentProjectChanged().onNotify(this, [this]() { | ||
| updateCommandStates(); | ||
| }); | ||
|
|
||
| interactive()->opened().onReceive(this, [this](const muse::Uri&) { | ||
| updateCommandStates(); | ||
| }); | ||
|
|
||
| playbackController()->isPlayAllowedChanged().onNotify(this, [this]() { | ||
| updateCommandStates(); | ||
| }); |
There was a problem hiding this comment.
State invalidation misses playback and page-close transitions.
commandState() derives values from isPlaying() and isProjectOpened(), but updateCommandStates() is never triggered on playback status changes or page close events. That leaves PLAY/PAUSE/STOP state notifications stale until another unrelated event fires.
Proposed fix
void PlaybackCommandsState::init()
{
globalContext()->currentProjectChanged().onNotify(this, [this]() {
updateCommandStates();
});
interactive()->opened().onReceive(this, [this](const muse::Uri&) {
updateCommandStates();
});
+ interactive()->closed().onReceive(this, [this](const muse::Uri&) {
+ updateCommandStates();
+ });
playbackController()->isPlayAllowedChanged().onNotify(this, [this]() {
updateCommandStates();
});
- // playbackController()->isPlayingChanged().onNotify(this, [this]() {
- // updateCommandStates();
- // });
+ playbackController()->isPlayingChanged().onNotify(this, [this]() {
+ updateCommandStates();
+ });
m_moduleRegister = commandsRegister()->moduleRegister(moduleName());
IF_ASSERT_FAILED(m_moduleRegister) {
return;
}
@@
void PlaybackCommandsState::deinit()
{
globalContext()->currentProjectChanged().disconnect(this);
interactive()->opened().disconnect(this);
+ interactive()->closed().disconnect(this);
playbackController()->isPlayAllowedChanged().disconnect(this);
- //playbackController()->isPlayingChanged().disconnect(this);
+ playbackController()->isPlayingChanged().disconnect(this);
}Also applies to: 52-54, 66-69
🤖 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/playback/internal/playbackcommandsstate.cpp` around lines 40 - 50, The
state invalidation is incomplete because updateCommandStates() is not triggered
when playback status changes or when the interactive/project closes. Add two
additional signal connections after the existing ones: connect to
playbackController()->isPlayingChanged() to catch playback state transitions,
and connect to interactive()->closed() to catch page close events. Both
connections should follow the same pattern as the existing signal handlers and
invoke updateCommandStates() to ensure command states remain synchronized with
the actual playback and project state.
2 participants
No description provided.