added commands diagnostic#90
Conversation
📝 WalkthroughWalkthroughThe PR extends the 🚥 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)
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: 4
🤖 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 `@framework/rcommand/CMakeLists.txt`:
- Around line 39-41: The MUSE_MODULE_RCOMMAND_QML build flag currently only
controls the QML subdirectory compilation, but the diagnostics action handler
and URI registration that depend on the QML resources are unconditional. This
creates a dead action path when the flag is OFF since the
diagnostic-show-rcommands action tries to open a QML URI that was never
compiled. Add preprocessor guards using `#if` MUSE_MODULE_RCOMMAND_QML and `#endif`
around the URI registration in rcommandmodule.cpp, the action definition in
diagnosticsactions.cpp, and the action handler in
diagnosticsactionscontroller.cpp to ensure these diagnostic components are only
built when the QML module is enabled.
In `@framework/rcommand/internal/commandsregister.cpp`:
- Around line 45-47: The command registration loop iterates through
module->commandInfoList() and unconditionally overwrites m_commandModuleNames
entries, while the corresponding unregistration code unconditionally erases
them. This causes issues when multiple modules register the same command
ID—unregistering one module incorrectly removes the mapping for the other. Fix
this by adding a conditional check in the registration loop (lines 45-47) to
only insert the command-to-module mapping if the command ID does not already
exist in m_commandModuleNames, and similarly in the unregistration section
(lines 63-65) only erase the mapping if it actually belongs to the module being
unregistered.
In `@framework/rcommand/internal/commandsstate.cpp`:
- Around line 62-66: The unreg() method does not validate that moduleName exists
in m_modules before attempting to disconnect and deinit, and it fails to
invalidate the cache entries for that module. Before calling disconnect() and
deinit() on the module, first verify that the module exists in m_modules.
Additionally, clear any cached command states for that module from m_cache to
prevent stale state from being returned by commandState(). Only proceed with the
disconnect(), deinit(), and erase() operations if the module was actually
registered.
In `@framework/rcommand/qml/Muse/RCommand/commandlistmodel.cpp`:
- Around line 95-103: The data() method validates that the QModelIndex itself is
valid with the isValid() check, but does not verify that index.row() is actually
within the bounds of the m_items QList before calling at(). Add an additional
bounds check after the isValid() validation to ensure that index.row() is
greater than or equal to 0 and less than m_items.size(), returning an empty
QVariant if the row index is out of bounds. Apply the same bounds validation fix
to the dispatch() method mentioned at lines 118-121 to prevent crashes or
assertions when accessing m_items with invalid indices.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 4367c5c2-8592-4007-995c-5d1db171854f
📒 Files selected for processing (20)
framework/diagnostics/internal/diagnosticsactions.cppframework/diagnostics/internal/diagnosticsactionscontroller.cppframework/rcommand/CMakeLists.txtframework/rcommand/commandtypes.hframework/rcommand/icommanddispatcher.hframework/rcommand/icommandsregister.hframework/rcommand/icommandsstate.hframework/rcommand/imodulecommandsregister.hframework/rcommand/imodulecommandsstate.hframework/rcommand/internal/commandsregister.cppframework/rcommand/internal/commandsregister.hframework/rcommand/internal/commandsstate.cppframework/rcommand/internal/commandsstate.hframework/rcommand/qml/Muse/RCommand/CMakeLists.txtframework/rcommand/qml/Muse/RCommand/CommandListDialog.qmlframework/rcommand/qml/Muse/RCommand/CommandListPanel.qmlframework/rcommand/qml/Muse/RCommand/commandlistmodel.cppframework/rcommand/qml/Muse/RCommand/commandlistmodel.hframework/rcommand/rcommandmodule.cppframework/rcommand/rcommandmodule.h
| if (MUSE_MODULE_RCOMMAND_QML) | ||
| add_subdirectory(qml/Muse/RCommand) | ||
| endif() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "1) Locate definitions/defaults/usages of MUSE_MODULE_RCOMMAND_QML"
rg -n "MUSE_MODULE_RCOMMAND_QML" -g 'CMakeLists.txt' -g '*.cmake' -g 'CMakePresets.json' -g '*.json'
echo
echo "2) Locate unconditional rcommands diagnostics wiring"
rg -n -C2 "diagnostic-show-rcommands|RCOMMAND_LIST_URI|muse://diagnostics/rcommand/list"
echo
echo "3) Check whether diagnostics code is conditionally compiled with the same flag"
rg -n "target_compile_definitions\\(|add_definitions\\(|MUSE_MODULE_RCOMMAND_QML" -g 'CMakeLists.txt' -g '*.cmake'Repository: musescore/muse_framework
Length of output: 15643
🏁 Script executed:
# Check rcommandmodule.cpp for conditional compilation around the URI registration
cat -n framework/rcommand/rcommandmodule.cpp | head -60Repository: musescore/muse_framework
Length of output: 2292
🏁 Script executed:
# Check diagnosticsactionscontroller.cpp for conditional compilation around RCOMMAND_LIST_URI
cat -n framework/diagnostics/internal/diagnosticsactionscontroller.cpp | head -70Repository: musescore/muse_framework
Length of output: 4685
🏁 Script executed:
# Check diagnosticsactions.cpp for conditional compilation around the UiAction
cat -n framework/diagnostics/internal/diagnosticsactions.cpp | head -110Repository: musescore/muse_framework
Length of output: 5421
Gate the rcommands diagnostic flow behind the same QML build flag.
The MUSE_MODULE_RCOMMAND_QML flag gates only the QML subdirectory build (framework/rcommand/qml/), but the diagnostics action handler and URI registration in framework/rcommand/rcommandmodule.cpp, framework/diagnostics/internal/diagnosticsactionscontroller.cpp, and framework/diagnostics/internal/diagnosticsactions.cpp are unconditional. When MUSE_MODULE_RCOMMAND_QML is OFF, the "diagnostic-show-rcommands" action will attempt to open a URI pointing to a QML dialog that was never compiled, creating a dead action path.
Wrap the URI registration in rcommandmodule.cpp and the action definition/handler in diagnosticsactionscontroller.cpp and diagnosticsactions.cpp with #if MUSE_MODULE_RCOMMAND_QML guards, or ensure the flag is always ON in build configurations.
🤖 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 `@framework/rcommand/CMakeLists.txt` around lines 39 - 41, The
MUSE_MODULE_RCOMMAND_QML build flag currently only controls the QML subdirectory
compilation, but the diagnostics action handler and URI registration that depend
on the QML resources are unconditional. This creates a dead action path when the
flag is OFF since the diagnostic-show-rcommands action tries to open a QML URI
that was never compiled. Add preprocessor guards using `#if`
MUSE_MODULE_RCOMMAND_QML and `#endif` around the URI registration in
rcommandmodule.cpp, the action definition in diagnosticsactions.cpp, and the
action handler in diagnosticsactionscontroller.cpp to ensure these diagnostic
components are only built when the QML module is enabled.
| for (const auto& info : module->commandInfoList()) { | ||
| m_commandModuleNames[info.command] = moduleName; | ||
| } |
There was a problem hiding this comment.
Preserve command ownership when duplicate command IDs are present.
On Line 45–Line 47, ownership is overwritten unconditionally; on Line 63–Line 65, ownership is erased unconditionally. If two modules expose the same Command, unregistering one can remove the other module’s mapping and break commandModuleName() resolution.
Suggested fix
void CommandsRegister::reg(const IModuleCommandsRegisterPtr& module)
{
@@
- for (const auto& info : module->commandInfoList()) {
- m_commandModuleNames[info.command] = moduleName;
- }
+ for (const auto& info : module->commandInfoList()) {
+ auto [ownerIt, inserted] = m_commandModuleNames.emplace(info.command, moduleName);
+ if (!inserted && ownerIt->second != moduleName) {
+ LOGW() << "command already registered by another module: " << info.command.toString()
+ << ", current owner: " << ownerIt->second
+ << ", attempted owner: " << moduleName;
+ }
+ }
}
@@
void CommandsRegister::unreg(const IModuleCommandsRegisterPtr& module)
{
@@
- for (const auto& info : module->commandInfoList()) {
- m_commandModuleNames.erase(info.command);
- }
+ for (const auto& info : module->commandInfoList()) {
+ auto ownerIt = m_commandModuleNames.find(info.command);
+ if (ownerIt != m_commandModuleNames.end() && ownerIt->second == moduleName) {
+ m_commandModuleNames.erase(ownerIt);
+ }
+ }
}Also applies to: 63-65
🤖 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 `@framework/rcommand/internal/commandsregister.cpp` around lines 45 - 47, The
command registration loop iterates through module->commandInfoList() and
unconditionally overwrites m_commandModuleNames entries, while the corresponding
unregistration code unconditionally erases them. This causes issues when
multiple modules register the same command ID—unregistering one module
incorrectly removes the mapping for the other. Fix this by adding a conditional
check in the registration loop (lines 45-47) to only insert the
command-to-module mapping if the command ID does not already exist in
m_commandModuleNames, and similarly in the unregistration section (lines 63-65)
only erase the mapping if it actually belongs to the module being unregistered.
| module->commandStateChanged().disconnect(this); | ||
| module->deinit(); | ||
|
|
||
| m_modules.erase(moduleName); | ||
| } |
There was a problem hiding this comment.
Unregister path leaves stale cache and can double-deinit modules.
unreg() does not verify that moduleName is currently registered before disconnect()/deinit(), and it never invalidates cached states for that module. Because commandState() checks m_cache first, removed-module commands can keep returning stale state indefinitely.
Suggested fix
void CommandsState::unreg(const IModuleCommandsStatePtr& module)
{
@@
const std::string& moduleName = module->moduleName();
IF_ASSERT_FAILED(!moduleName.empty()) {
return;
}
+
+ auto mit = m_modules.find(moduleName);
+ IF_ASSERT_FAILED(mit != m_modules.end()) {
+ LOGW() << "module not registered: " << moduleName;
+ return;
+ }
module->commandStateChanged().disconnect(this);
module->deinit();
+ if (auto reg = commandsRegister()->moduleRegister(moduleName)) {
+ for (const CommandInfo& info : reg->commandInfoList()) {
+ m_cache.erase(info.command);
+ }
+ }
+
m_modules.erase(moduleName);
}Also applies to: 73-80
🤖 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 `@framework/rcommand/internal/commandsstate.cpp` around lines 62 - 66, The
unreg() method does not validate that moduleName exists in m_modules before
attempting to disconnect and deinit, and it fails to invalidate the cache
entries for that module. Before calling disconnect() and deinit() on the module,
first verify that the module exists in m_modules. Additionally, clear any cached
command states for that module from m_cache to prevent stale state from being
returned by commandState(). Only proceed with the disconnect(), deinit(), and
erase() operations if the module was actually registered.
| QVariant CommandListModel::data(const QModelIndex& index, int role) const | ||
| { | ||
| if (!index.isValid()) { | ||
| return QVariant(); | ||
| } | ||
|
|
||
| switch (role) { | ||
| case rItemData: return QString::number(index.row()) + ". " + m_items.at(index.row()).formatted; | ||
| } |
There was a problem hiding this comment.
Guard model index bounds before QList::at() access.
Both data() and dispatch() trust indices without range validation. Invalid or stale indices can crash/assert at runtime.
Suggested fix
QVariant CommandListModel::data(const QModelIndex& index, int role) const
{
- if (!index.isValid()) {
+ if (!index.isValid() || index.row() < 0 || index.row() >= m_items.size()) {
return QVariant();
}
@@
void CommandListModel::dispatch(int index)
{
+ if (index < 0 || index >= m_items.size()) {
+ LOGW() << "dispatch: invalid index " << index;
+ return;
+ }
const Item& item = m_items.at(index);
commandDispatcher()->dispatch(item.info.command);
LOGD() << "dispatch: " << item.info.command.toString();
}Also applies to: 118-121
🧰 Tools
🪛 Clang (14.0.6)
[warning] 95-95: use a trailing return type for this function
(modernize-use-trailing-return-type)
[warning] 95-95: 2 adjacent parameters of 'data' of similar type are easily swapped by mistake
(bugprone-easily-swappable-parameters)
[note] 95-95: the first parameter in the range is 'index'
(clang)
[note] 95-95: the last parameter in the range is 'role'
(clang)
[note] 95-95: 'const int &' and 'int' parameters accept and bind the same kind of values
(clang)
🪛 Cppcheck (2.21.0)
[style] 95-95: The function 'async_disconnect' is never used.
(unusedFunction)
🤖 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 `@framework/rcommand/qml/Muse/RCommand/commandlistmodel.cpp` around lines 95 -
103, The data() method validates that the QModelIndex itself is valid with the
isValid() check, but does not verify that index.row() is actually within the
bounds of the m_items QList before calling at(). Add an additional bounds check
after the isValid() validation to ensure that index.row() is greater than or
equal to 0 and less than m_items.size(), returning an empty QVariant if the row
index is out of bounds. Apply the same bounds validation fix to the dispatch()
method mentioned at lines 118-121 to prevent crashes or assertions when
accessing m_items with invalid indices.
| anchors.leftMargin: 16 | ||
| verticalAlignment: Text.AlignVCenter | ||
| horizontalAlignment: Text.AlignLeft | ||
| font.family: "Consolas" |
2 participants
No description provided.