Fix ArgumentException when editing a disabled Explorer action keyword#4339
Conversation
…lugin - Fix (false, false) case in EditActionKeyword to break gracefully instead of throwing ArgumentException. This handles the valid scenario where the user changes a disabled keyword's text but keeps it disabled. - Fix GetActiveActionKeywords to return an empty dictionary instead of null when actionKeywordStr is null/empty, preventing NullReferenceException. - Fix ActionKeyword setter to only auto-enable when the value actually changes, preventing unintended side-effects during construction. - Initialize ActionKeywordSetting backing fields directly in constructor to avoid the auto-enable triggering during initialization. - Change TextBox binding to UpdateSourceTrigger=PropertyChanged so the checkbox auto-enables as the user types (before they click the checkbox), fixing the checkbox toggle issue caused by LostFocus timing. Co-authored-by: Jack251970 <53996452+Jack251970@users.noreply.github.com>
|
🥷 Code experts: no user but you matched threshold 10 Jack251970 has most 👩💻 activity in the files. See details
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame: ✨ Comment |
|
Be a legend 🏆 by adding a before and after screenshot of the changes you made, especially if they are around UI/UX. |
📝 WalkthroughWalkthroughFixed a bug where editing a disabled action keyword throws an exception by replacing the error-throwing logic with a no-op path. Changes also include improved null-handling in keyword retrieval, updated binding behavior for real-time property updates, and refactored property initialization to avoid unintended side effects. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 4 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Plugins/Flow.Launcher.Plugin.Explorer/Views/ActionKeywordSetting.xaml">
<violation number="1" location="Plugins/Flow.Launcher.Plugin.Explorer/Views/ActionKeywordSetting.xaml:85">
P2: Updating the binding source on every keystroke can unintentionally enable a disabled keyword even when the final text is unchanged.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Pull request overview
This PR fixes an ArgumentException that occurred when editing a disabled Explorer action keyword (e.g., Folder Search). The root cause was that the ActionKeywordSetting dialog's ActionKeyword property setter unconditionally set KeywordEnabled = true whenever the property was written to — including during constructor initialization — and a (false, false) case in the switch statement was treated as unreachable when it was actually valid.
Changes:
- Fixed the
ActionKeywordSettingconstructor to initialize backing fields directly instead of going through property setters, avoiding the auto-enable side effect during initialization. - Made the
ActionKeywordproperty setter only auto-enable the keyword when the value actually changes (viaSetPropertyreturn value), and addedUpdateSourceTrigger=PropertyChangedto the XAML binding for immediate updates on keystroke. - Replaced the
throw ArgumentExceptionin the(false, false)case with abreak, since changing keyword text while keeping it disabled is a valid operation; also changedGetActiveActionKeywordsto return an empty dictionary instead ofnullfor defensive null-safety.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
Plugins/Flow.Launcher.Plugin.Explorer/Views/ActionKeywordSetting.xaml.cs |
Initialize backing fields directly in constructor; only auto-enable when keyword text actually changes |
Plugins/Flow.Launcher.Plugin.Explorer/Views/ActionKeywordSetting.xaml |
Add UpdateSourceTrigger=PropertyChanged to ActionKeyword textbox binding |
Plugins/Flow.Launcher.Plugin.Explorer/ViewModels/SettingsViewModel.cs |
Handle (false, false) case with a no-op break instead of throwing |
Plugins/Flow.Launcher.Plugin.Explorer/Settings.cs |
Return empty dictionary instead of null from GetActiveActionKeywords |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| { | ||
| var result = new Dictionary<ActionKeyword, string>(); | ||
| if (string.IsNullOrEmpty(actionKeywordStr)) return null; | ||
| if (string.IsNullOrEmpty(actionKeywordStr)) return result; |
There was a problem hiding this comment.
Any specific resaon for changing or it's about code style?
There was a problem hiding this comment.
Return a non-null value to enhance code robustness
…-action Fix ArgumentException when editing a disabled Explorer action keyword
Resolve #4289
Summary by cubic
Summary of changes
Fixes an ArgumentException when editing a disabled action keyword in
Flow.Launcher.Plugin.Explorerand prevents a potential NullReferenceException. The Action Keyword dialog now updates as you type and only auto-enables when the keyword actually changes.Changed logic/behavior:
New logic/behavior added:
Removed logic/behavior:
Memory usage impact:
Security risks:
Unit tests:
Written for commit 531b452. Summary will update on new commits.