fix: disable command menu doesnt work #852

lollipopkit · 2025-08-13T14:56:46Z

Fixes #841

Summary by Sourcery

Refactor command type handling to use a unified ShellCmdType interface with system metadata, update the disabled commands UI to display icons and correctly toggle items by displayName, and adjust script builders to filter commands by displayName, thereby fixing the issue where disabling commands did not work.

Bug Fixes:

Fix disabled command menu not correctly toggling command types

Enhancements:

Introduce CmdTypeSys enum to associate command types with systems and icons
Rename CommandType to ShellCmdType and add displayName and sysType properties
Update disabled commands dialog to use ShellCmdType items with icons and displayName
Modify script builders to filter Windows and Unix commands by displayName instead of name

Fixes #841

sourcery-ai · 2025-08-13T14:56:52Z

Reviewer's Guide

This patch refactors the command-type abstraction by introducing a system-specific enum, renaming the base interface to ShellCmdType (with icon and displayName support), updates all existing command enums accordingly, adjusts the server edit UI to use the new types and displayNames, and fixes script builders to filter disabled commands by displayName.

Sequence diagram for filtering disabled command types in script builders

sequenceDiagram
    participant ScriptBuilder
    participant DisabledCmdTypes
    participant CmdTypeEnum
    ScriptBuilder->>DisabledCmdTypes: Get disabled command types (by displayName)
    ScriptBuilder->>CmdTypeEnum: Filter command types not in disabledCmdTypes
    CmdTypeEnum-->>ScriptBuilder: Return enabled command types
    ScriptBuilder->>ScriptBuilder: Build command string from enabled types

File-Level Changes

Change	Details	Files
Refactor command-type abstractions with system enum and unified interface	Add CmdTypeSys enum with system sign and icon mapping Rename CommandType to ShellCmdType and add sysType getter Introduce ShellCmdTypeX extension for displayName Update CommandTypeX extension to target ShellCmdType	`lib/data/model/app/scripts/cmd_types.dart`
Migrate existing enums to implement ShellCmdType	Change StatusCmdType, BSDStatusCmdType, WindowsStatusCmdType to implement ShellCmdType Add sysType override in each command-type enum	`lib/data/model/app/scripts/cmd_types.dart`
Update server edit page to use ShellCmdType and displayName	Switch allCmdTypes set from String to ShellCmdType and remove name-based handling Modify _showCmdTypesDialog signature and return type to work with enums Replace CheckboxListTile with ListTile+Checkbox showing enum icon and name Adapt onChanged/onTap to add/remove displayName strings and notify the ValueNotifier Filter initial _disabledCmdTypes by valid displayNames	`lib/view/page/server/edit.dart`
Adjust script builders to filter by displayName	Use e.displayName instead of e.name when excluding disabled commands in Windows builder Use e.displayName instead of e.name when excluding disabled commands in Unix builder	`lib/data/model/app/scripts/script_builders.dart`

Assessment against linked issues

Issue	Objective	Addressed	Explanation
#841	Fix the bug where selecting an option in the disable command menu does nothing.	✅
#841	Fix the bug where the disable command menu gets stuck and requires restarting the app to close.	✅

Tips and commands

Interacting with Sourcery

Trigger a new review: Comment @sourcery-ai review on the pull request.
Continue discussions: Reply directly to Sourcery's review comments.
Generate a GitHub issue from a review comment: Ask Sourcery to create an
issue from a review comment by replying to it. You can also reply to a
review comment with @sourcery-ai issue to create an issue from it.
Generate a pull request title: Write @sourcery-ai anywhere in the pull
request title to generate a title at any time. You can also comment
@sourcery-ai title on the pull request to (re-)generate the title at any time.
Generate a pull request summary: Write @sourcery-ai summary anywhere in
the pull request body to generate a PR summary at any time exactly where you
want it. You can also comment @sourcery-ai summary on the pull request to
(re-)generate the summary at any time.
Generate reviewer's guide: Comment @sourcery-ai guide on the pull
request to (re-)generate the reviewer's guide at any time.
Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
pull request to resolve all Sourcery comments. Useful if you've already
addressed all the comments and don't want to see them anymore.
Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
request to dismiss all existing Sourcery reviews. Especially useful if you
want to start fresh with a new review - don't forget to comment
@sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

Enable or disable review features such as the Sourcery-generated pull request
summary, the reviewer's guide, and others.
Change the review language.
Add, remove or edit custom review instructions.
Adjust other review settings.

Getting Help

Contact our support team for questions or feedback.
Visit our documentation for detailed guides and information.
Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

claude · 2025-08-13T14:57:02Z

Claude finished @lollipopkit's task —— View job

Code Review for PR #852: fix: disable command menu doesn't work

I've completed my review of the changes that fix issue #841. Here's my comprehensive feedback:

Todo List:

Read repository guidelines (CLAUDE.md)
Analyze changed files for code quality and potential issues
Check for security concerns
Evaluate performance considerations
Review test coverage implications
Provide comprehensive feedback

✅ Overall Assessment: Good Fix

This PR successfully resolves the disabled command menu functionality by implementing a proper type system and fixing the UI interaction logic. The changes are well-architected and improve code maintainability.

Key Changes Analysis

1. Enhanced Type System (`cmd_types.dart`)

Strengths:

✅ Excellent refactoring: Renamed CommandType → ShellCmdType for better clarity
✅ Proper abstraction: Added CmdTypeSys enum with system icons for better UI representation
✅ Clean extension: ShellCmdTypeX.displayName provides consistent "System.name" format
✅ Type safety: All command enums now implement the same interface consistently

Minor suggestions:

Documentation is comprehensive and well-written
Consider adding unit tests for the new displayName extension

2. Script Builder Updates (`script_builders.dart`)

Fix Analysis:

✅ Correct fix: Changed from e.name to e.displayName in filtering logic (lines 109, 199, 202)
✅ Consistency: Both Windows and Unix builders now use the same display name format
✅ Backward compatibility: Maintains existing functionality while fixing the filtering bug

3. UI Improvements (`edit.dart`)

Major Improvements:

✅ Better UX: Dialog now shows system icons and proper command type grouping
✅ Type safety: Uses ShellCmdType objects instead of string manipulation
✅ Simplified logic: Removed unnecessary return value from dialog
✅ Data integrity: Added validation to remove invalid disabled commands during initialization (lines 780-786)

UI Enhancement:

The new ListTile with system icons is much more user-friendly than the previous CheckboxListTile
Proper tap handling on both tile and checkbox

Security Assessment 🔒

✅ No security concerns identified
✅ Input validation: Proper validation of disabled command types
✅ No sensitive data exposure: Changes are purely UI/logic related

Performance Considerations ⚡

✅ Minimal impact: Changes are mostly structural with no performance implications
✅ Efficient filtering: Uses proper Set operations for disabled command filtering
✅ UI optimization: Dialog uses ListView.builder for efficient rendering

Potential Issues & Suggestions

Minor Issues:

Consistency in edit.dart:606:
```
allCmdTypes.remove(StatusCmdType.time);
```
Consider adding a comment explaining why time command specifically is excluded from the disable list.
Type Safety Enhancement:
The validation logic in _initWithSpi (lines 780-786) could be moved to a utility method to avoid code duplication if used elsewhere.

Test Coverage Implications 🧪

Recommendation: Add unit tests for:
- ShellCmdTypeX.displayName extension
- Disabled command filtering logic in script builders
- Dialog state management in UI

Code Quality Score: A-

Strengths:

Proper type system design
Clean separation of concerns
Good documentation
Consistent naming conventions
Backward compatibility maintained

Areas for improvement:

Add unit tests for new functionality
Consider extracting validation logic to utilities

Conclusion: This is a solid fix that not only resolves the immediate issue but also improves the overall architecture. The type system improvements will make future maintenance easier and the UI enhancements provide better user experience.

sourcery-ai

Hey @lollipopkit - I've reviewed your changes and they look great!

Prompt for AI Agents

Please address the comments from this code review:
## Individual Comments

### Comment 1
<location> `lib/data/model/app/scripts/script_builders.dart:199` </location>
<code_context>
   /// Get Unix status command with OS detection
   String _getUnixStatusCommand({required List<String> disabledCmdTypes}) {
     // Generate command lists with command-specific separators, filtering disabled commands
-    final filteredLinuxCmdTypes = StatusCmdType.values.where((e) => !disabledCmdTypes.contains(e.name));
+    final filteredLinuxCmdTypes = StatusCmdType.values.where((e) => !disabledCmdTypes.contains(e.displayName));
     final linuxCommands = filteredLinuxCmdTypes.map((e) => '${e.divider}${e.cmd}').join('').trimRight();

</code_context>

<issue_to_address>
Linux command filtering now depends on displayName.

Ensure all code and data sources use displayName for disabledCmdTypes to prevent commands from being incorrectly enabled.
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

lib/data/model/app/scripts/script_builders.dart

claude · 2025-08-13T15:29:27Z

Claude finished @lollipopkit's task —— View job

Code Review for PR #852: fix: disable command menu doesn't work

Todo List:

Read repository guidelines (CLAUDE.md)
Analyze changed files for code quality and potential issues
Check for security concerns
Evaluate performance considerations
Review test coverage implications
Provide comprehensive feedback

✅ Overall Assessment: Good Fix

This PR successfully resolves the disabled command menu functionality by implementing a proper type system and fixing the UI interaction logic. The changes are well-architected and improve code maintainability.

Key Changes Analysis

1. Enhanced Type System (`cmd_types.dart`)

Strengths:

✅ Excellent refactoring: Renamed CommandType → ShellCmdType for better clarity
✅ Proper abstraction: Added CmdTypeSys enum with system icons for better UI representation
✅ Clean extension: ShellCmdTypeX.displayName provides consistent "System.name" format
✅ Type safety: All command enums now implement the same interface consistently

Minor suggestions:

Documentation is comprehensive and well-written
Consider adding unit tests for the new displayName extension

2. Script Builder Updates (`script_builders.dart`)

Fix Analysis:

✅ Correct fix: Changed from e.name to e.displayName in filtering logic (lines 109, 199, 202)
✅ Consistency: Both Windows and Unix builders now use the same display name format
✅ Backward compatibility: Maintains existing functionality while fixing the filtering bug

3. UI Improvements (`edit.dart`)

Major Improvements:

✅ Better UX: Dialog now shows system icons and proper command type grouping
✅ Type safety: Uses ShellCmdType objects instead of string manipulation
✅ Simplified logic: Removed unnecessary return value from dialog
✅ Data integrity: Added validation to remove invalid disabled commands during initialization (lines 780-786)

UI Enhancement:

The new ListTile with system icons is much more user-friendly than the previous CheckboxListTile
Proper tap handling on both tile and checkbox

Security Assessment 🔒

✅ No security concerns identified
✅ Input validation: Proper validation of disabled command types
✅ No sensitive data exposure: Changes are purely UI/logic related

Performance Considerations ⚡

✅ Minimal impact: Changes are mostly structural with no performance implications
✅ Efficient filtering: Uses proper Set operations for disabled command filtering
✅ UI optimization: Dialog uses ListView.builder for efficient rendering

Potential Issues & Suggestions

Minor Issues:

Consistency in edit.dart:606:
```
allCmdTypes.remove(StatusCmdType.time);
```
Consider adding a comment explaining why time command specifically is excluded from the disable list.
Type Safety Enhancement:
The validation logic in _initWithSpi (lines 780-786) could be moved to a utility method to avoid code duplication if used elsewhere.

Test Coverage Implications 🧪

Recommendation: Add unit tests for:
- ShellCmdTypeX.displayName extension
- Disabled command filtering logic in script builders
- Dialog state management in UI

Code Quality Score: A-

Strengths:

Proper type system design
Clean separation of concerns
Good documentation
Consistent naming conventions
Backward compatibility maintained

Areas for improvement:

Add unit tests for new functionality
Consider extracting validation logic to utilities

Conclusion: This is a solid fix that not only resolves the immediate issue but also improves the overall architecture. The type system improvements will make future maintenance easier and the UI enhancements provide better user experience.

fix: disable command menu doesnt work

52ea790

Fixes #841

sourcery-ai bot approved these changes Aug 13, 2025

View reviewed changes

lib/data/model/app/scripts/script_builders.dart Show resolved Hide resolved

opt. & tests

5f57da5

lollipopkit merged commit 68734a9 into main Aug 13, 2025
2 checks passed

lollipopkit deleted the lollipopkit/issue841 branch August 13, 2025 15:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

fix: disable command menu doesnt work #852

fix: disable command menu doesnt work #852

Uh oh!

lollipopkit commented Aug 13, 2025 •

edited by sourcery-ai bot

Loading

Uh oh!

sourcery-ai bot commented Aug 13, 2025 •

edited

Loading

Interacting with Sourcery

Customizing Your Experience

Getting Help

Uh oh!

claude bot commented Aug 13, 2025 •

edited

Loading

Uh oh!

sourcery-ai bot left a comment

Uh oh!

Uh oh!

claude bot commented Aug 13, 2025 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

fix: disable command menu doesnt work #852

fix: disable command menu doesnt work #852

Uh oh!

Conversation

lollipopkit commented Aug 13, 2025 • edited by sourcery-ai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by Sourcery

Uh oh!

sourcery-ai bot commented Aug 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviewer's Guide

Sequence diagram for filtering disabled command types in script builders

File-Level Changes

Assessment against linked issues

Interacting with Sourcery

Customizing Your Experience

Getting Help

Uh oh!

claude bot commented Aug 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review for PR #852: fix: disable command menu doesn't work

Todo List:

✅ Overall Assessment: Good Fix

Key Changes Analysis

1. Enhanced Type System (cmd_types.dart)

2. Script Builder Updates (script_builders.dart)

3. UI Improvements (edit.dart)

Security Assessment 🔒

Performance Considerations ⚡

Potential Issues & Suggestions

Minor Issues:

Test Coverage Implications 🧪

Code Quality Score: A-

Uh oh!

sourcery-ai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

claude bot commented Aug 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review for PR #852: fix: disable command menu doesn't work

Todo List:

✅ Overall Assessment: Good Fix

Key Changes Analysis

1. Enhanced Type System (cmd_types.dart)

2. Script Builder Updates (script_builders.dart)

3. UI Improvements (edit.dart)

Security Assessment 🔒

Performance Considerations ⚡

Potential Issues & Suggestions

Minor Issues:

Test Coverage Implications 🧪

Code Quality Score: A-

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

lollipopkit commented Aug 13, 2025 •

edited by sourcery-ai bot

Loading

sourcery-ai bot commented Aug 13, 2025 •

edited

Loading

claude bot commented Aug 13, 2025 •

edited

Loading

1. Enhanced Type System (`cmd_types.dart`)

2. Script Builder Updates (`script_builders.dart`)

3. UI Improvements (`edit.dart`)

claude bot commented Aug 13, 2025 •

edited

Loading

1. Enhanced Type System (`cmd_types.dart`)

2. Script Builder Updates (`script_builders.dart`)

3. UI Improvements (`edit.dart`)