fix: gray out commands of disabled plugins in behavior manager - #9664
Open
chufeng wants to merge 2 commits into
Open
fix: gray out commands of disabled plugins in behavior manager#9664chufeng wants to merge 2 commits into
chufeng wants to merge 2 commits into
Conversation
Show inactive-plugin commands as 未启用, dim the whole row with theme on-surface colors, and disable row actions. Fixes AstrBotDevs#9562
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
_apply_plugin_activation_to_descriptors, you’re mutatingdesc.enabledbased on plugin activation; consider keeping the originalenabledflag untouched and exposing aneffective_enabled/computed flag instead to avoid confusing other consumers that rely on the stored command toggle state. - The CSS rule
.plugin-inactive-row .v-btn-group .v-btn { pointer-events: none !important; }combined with:disabledon the buttons will prevent the newpluginInactivetooltips from ever appearing; you may want to rely on:disabledand cursor styling alone so hover tooltips still work. - Disabling the "view details" action for commands of inactive plugins may make it harder to inspect or debug their configuration; consider leaving that action enabled while still visually indicating that the plugin is inactive.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `_apply_plugin_activation_to_descriptors`, you’re mutating `desc.enabled` based on plugin activation; consider keeping the original `enabled` flag untouched and exposing an `effective_enabled`/computed flag instead to avoid confusing other consumers that rely on the stored command toggle state.
- The CSS rule `.plugin-inactive-row .v-btn-group .v-btn { pointer-events: none !important; }` combined with `:disabled` on the buttons will prevent the new `pluginInactive` tooltips from ever appearing; you may want to rely on `:disabled` and cursor styling alone so hover tooltips still work.
- Disabling the "view details" action for commands of inactive plugins may make it harder to inspect or debug their configuration; consider leaving that action enabled while still visually indicating that the plugin is inactive.
## Individual Comments
### Comment 1
<location path="dashboard/src/components/extension/componentPanel/types.ts" line_range="22" />
<code_context>
aliases: string[];
permission: PermissionType;
enabled: boolean;
+ plugin_activated?: boolean;
is_group: boolean;
has_conflict: boolean;
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Align the `CommandItem` type with the backend by making `plugin_activated` non-optional.
Since `_descriptor_to_dict` always sends `plugin_activated`, the TS type should require it. Keeping it optional hides backend omissions and encourages unnecessary defensive checks in the UI. Making it required will better match the API contract and simplify consumers such as `isPluginInactive`.
Suggested implementation:
```typescript
aliases: string[];
permission: PermissionType;
enabled: boolean;
plugin_activated: boolean;
is_group: boolean;
has_conflict: boolean;
reserved: boolean;
```
Search for all usages of `plugin_activated` in the codebase:
1. Remove any optional chaining or defensive checks that assume `plugin_activated` can be `undefined` (e.g. `command.plugin_activated ?? false`, `command.plugin_activated === false || command.plugin_activated === undefined`), and simplify them to work with a required boolean (e.g. `!command.plugin_activated`).
2. If there are constructors, mappers, or test fixtures that build `CommandItem` objects, ensure they all explicitly set `plugin_activated` to a boolean value to satisfy the now-required field.
</issue_to_address>
### Comment 2
<location path="tests/unit/test_command_plugin_activation.py" line_range="19" />
<code_context>
+ star_map["data.plugins.foo.main"] = SimpleNamespace(activated=True)
+ star_map["data.plugins.bar.main"] = SimpleNamespace(activated=False)
+
+ active = SimpleNamespace(module_path="data.plugins.foo.main", enabled=True)
+ inactive = SimpleNamespace(module_path="data.plugins.bar.main", enabled=True)
+ unknown = SimpleNamespace(module_path="data.plugins.missing.main", enabled=True)
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test that verifies the new `plugin_activated` field in the command JSON/dict, not just the in-memory `enabled` flag.
The production change also added `plugin_activated` to `_descriptor_to_dict`, which is what the dashboard consumes. This test only verifies that `enabled` is flipped for inactive plugins and never asserts the serialized `plugin_activated` value. Please add a unit test that builds descriptors for active and inactive plugins, runs `_descriptor_to_dict`, and checks both `enabled` and `plugin_activated` so the UI’s distinction between “plugin off” and per-command toggles is covered.
Suggested implementation:
```python
_is_plugin_activated,
star_map,
)
original = dict(star_map)
try:
star_map.clear()
star_map["data.plugins.foo.main"] = SimpleNamespace(activated=True)
star_map["data.plugins.bar.main"] = SimpleNamespace(activated=False)
active = SimpleNamespace(module_path="data.plugins.foo.main", enabled=True)
inactive = SimpleNamespace(module_path="data.plugins.bar.main", enabled=True)
unknown = SimpleNamespace(module_path="data.plugins.missing.main", enabled=True)
_apply_plugin_activation_to_descriptors([active, inactive, unknown])
# In-memory activation/enabled flags
assert _is_plugin_activated(active) is True
assert _is_plugin_activated(inactive) is False
assert _is_plugin_activated(unknown) is True
assert active.enabled is True
assert inactive.enabled is False
assert unknown.enabled is True
# Serialized descriptor fields consumed by the dashboard
active_dict = _descriptor_to_dict(active)
inactive_dict = _descriptor_to_dict(inactive)
unknown_dict = _descriptor_to_dict(unknown)
# enabled should reflect the per-command toggle after plugin activation
assert active_dict["enabled"] is True
assert inactive_dict["enabled"] is False
assert unknown_dict["enabled"] is True
# plugin_activated should reflect whether the plugin itself is on/off
assert active_dict["plugin_activated"] is True
assert inactive_dict["plugin_activated"] is False
assert unknown_dict["plugin_activated"] is True
finally:
```
To make this compile and run:
1. Ensure `_descriptor_to_dict` is available in this test scope. If it is not already imported or provided as a fixture, add an import at the top of `tests/unit/test_command_plugin_activation.py`, e.g.:
`from <your_command_module> import _descriptor_to_dict`
2. If you are using pytest fixtures for `_descriptor_to_dict` (similar to `_is_plugin_activated` and `star_map`), add `_descriptor_to_dict` to the test function parameters instead of importing it, and remove the import suggestion.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Keep the stored command enabled flag unchanged and expose plugin activation separately. Allow viewing details, keep hover tooltips, and cover serialized plugin_activated in unit tests.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #9562
After a plugin is disabled, its commands still show as Enabled on Plugins → Behavior Manager, but they no longer work at runtime.
Modifications / 改动点
The command list only checked each command's own
enabledflag and ignored the pluginactivatedstate.Backend
command_management.py: addplugin_activated. If the plugin is off, treat its commands as inactive without changing stored command toggles.Frontend
CommandTable.vue: keep those commands visible, mark them as 未启用 / Plugin off, dim the whole row, and disable row actions.Use theme
on-surfacecolors so light and dark mode both stay readable.Add zh-CN / en-US / ru-RU copy and a small backend unit test.
This is NOT a breaking change. / 这不是一个破坏性变更。
Screenshots or Test Results / 运行截图或测试结果
Verification:
pytest tests/unit/test_command_plugin_activation.py -q
passed
(Paste a screenshot of the dimmed row here.)
Checklist / 检查清单
😊 If there are new features added in the PR, I have discussed it with the authors through issues/emails, etc.
/ 如果 PR 中有新加入的功能,已经通过 Issue / 邮件等方式和作者讨论过。
👀 My changes have been well-tested, and "Verification Steps" and "Screenshots" have been provided above.
/ 我的更改经过了良好的测试,并已在上方提供了“验证步骤”和“运行截图”。
🤓 I have ensured that no new dependencies are introduced, OR if new dependencies are introduced, they have been added to the appropriate locations in
requirements.txtandpyproject.toml./ 我确保没有引入新依赖库,或者引入了新依赖库的同时将其添加到
requirements.txt和pyproject.toml文件相应位置。😮 My changes do not introduce malicious code.
/ 我的更改没有引入恶意代码。
Summary by Sourcery
Reflect plugin activation status in command listings so commands from disabled plugins no longer appear enabled in the behavior manager.
Bug Fixes:
Enhancements:
Tests: