Fix ArgumentOutOfRangeException in WebSearch Plugin #4041
Conversation
Add bounds checking and fallback logic to handle cases where the search source reference is not found in the collection. This prevents the crash when editing a search source that may have been modified or removed from the collection. Co-authored-by: Jack251970 <[email protected]>
…ettings Add defensive programming checks to CustomHotkeyEdit and CustomShortcutEdit methods to prevent potential IndexOutOfRangeException in rare edge cases where collection might be modified concurrently. Co-authored-by: Jack251970 <[email protected]>
Added a new localized string `flowlauncher_plugin_websearch_edit_failed` in `en.xaml` for handling errors when updating a search source. Updated `SearchSourceSetting.xaml.cs` to use this localized string and removed fallback logic for default error messages.
Removed fallback mechanism for locating a search source by `ActionKeyword` when not found by reference. The update now proceeds only if the search source is found by reference, streamlining the logic and reducing complexity.
There was a problem hiding this comment.
Pull Request Overview
This PR fixes an ArgumentOutOfRangeException in the WebSearch plugin and similar issues in settings by adding bounds checking before accessing collections using IndexOf results. The fix prevents crashes when trying to update items that may have been removed from collections between retrieval and modification.
- Added bounds checking (index >= 0 && index < collection.Count) before accessing collections
- Added proper error handling with localized warning messages for failed operations
- Applied the same defensive pattern across multiple similar operations
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| SearchSourceSetting.xaml.cs | Added bounds checking and error handling for search source editing |
| en.xaml | Added localized error message for failed search source updates |
| SettingsPaneHotkeyViewModel.cs | Added bounds checking for hotkey and shortcut editing operations |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
🥷 Code experts: onesounds, Yusyuriv onesounds, Jack251970 have 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: ✨ Comment |
|
Be a legend 🏆 by adding a before and after screenshot of the changes you made, especially if they are around UI/UX. |
📝 WalkthroughWalkthroughAdds bounds checks before updating collections in hotkey and web search edit flows. If an index is invalid, skip the update; for web search, show a new localized warning. Localization adds a new English string for edit failure. No public APIs changed. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UI as SearchSourceSettingWindow
participant List as _searchSources
User->>UI: Confirm edit
UI->>UI: Find index of selected source
alt Index within bounds
UI->>List: Update item at index
UI-->>User: Close dialog
else Index invalid
UI-->>User: Show "edit failed" warning
UI-->>User: Close dialog
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ 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: 0
🧹 Nitpick comments (2)
Flow.Launcher/SettingPages/ViewModels/SettingsPaneHotkeyViewModel.cs (2)
92-97: Approve bounds check; consider adding user feedback.The bounds check correctly prevents ArgumentOutOfRangeException when the item is removed from the collection between the
FirstOrDefaultcheck and theIndexOfcall. This scenario can occur if the user opens the edit dialog, then switches to another window to delete the item, and switches back to confirm the edit.However, the silent failure may confuse users—their edit won't be saved, and they won't know why. Consider showing a message similar to the WebSearch plugin's approach (lines 118-122 in SearchSourceSetting.xaml.cs).
If you'd like to add user feedback, apply this diff:
var index = Settings.CustomPluginHotkeys.IndexOf(settingItem); if (index >= 0 && index < Settings.CustomPluginHotkeys.Count) { Settings.CustomPluginHotkeys[index] = new CustomPluginHotkey(window.Hotkey, window.ActionKeyword); HotKeyMapper.RemoveHotkey(settingItem.Hotkey); // remove origin hotkey HotKeyMapper.SetCustomQueryHotkey(Settings.CustomPluginHotkeys[index]); // set new hotkey } +else +{ + App.API.ShowMsgBox(Localize.invalidPluginHotkey()); +}
156-159: Approve bounds check; consider adding user feedback.The bounds check correctly prevents ArgumentOutOfRangeException. However, like
CustomHotkeyEdit, the silent failure may confuse users when their edit isn't saved.If you'd like to add user feedback for consistency, apply this diff:
var index = Settings.CustomShortcuts.IndexOf(settingItem); if (index >= 0 && index < Settings.CustomShortcuts.Count) { Settings.CustomShortcuts[index] = new CustomShortcutModel(window.Key, window.Value); } +else +{ + App.API.ShowMsgBox(Localize.invalidShortcut()); +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Flow.Launcher/SettingPages/ViewModels/SettingsPaneHotkeyViewModel.cs(2 hunks)Plugins/Flow.Launcher.Plugin.WebSearch/Languages/en.xaml(1 hunks)Plugins/Flow.Launcher.Plugin.WebSearch/SearchSourceSetting.xaml.cs(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3593
File: Plugins/Flow.Launcher.Plugin.WebSearch/SettingsControl.xaml.cs:143-165
Timestamp: 2025-09-13T06:04:26.977Z
Learning: In Plugins/Flow.Launcher.Plugin.WebSearch/SettingsControl.xaml, the ListView_SizeChanged handler in the C# file assumes 5 GridViewColumns but the XAML may have more columns. The user Jack251970 indicated this mismatch doesn't need to be fixed when pointed out during PR #3593 review.
🧬 Code graph analysis (1)
Flow.Launcher/SettingPages/ViewModels/SettingsPaneHotkeyViewModel.cs (3)
Flow.Launcher.Infrastructure/UserSettings/Settings.cs (1)
Settings(15-615)Flow.Launcher/Helper/HotKeyMapper.cs (3)
HotKeyMapper(13-168)RemoveHotkey(96-119)SetCustomQueryHotkey(138-148)Flow.Launcher.Infrastructure/UserSettings/CustomShortcutModel.cs (1)
CustomShortcutModel(69-79)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: build
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
🔇 Additional comments (2)
Plugins/Flow.Launcher.Plugin.WebSearch/SearchSourceSetting.xaml.cs (1)
111-123: LGTM! Excellent user feedback on edge case.The bounds check correctly prevents ArgumentOutOfRangeException, and the user-facing warning message provides clear feedback when the update fails. This implementation handles the TOCTOU scenario where the item could be removed from the collection while the edit dialog is open (e.g., user switches to another window to delete the item, then returns to confirm the edit).
Plugins/Flow.Launcher.Plugin.WebSearch/Languages/en.xaml (1)
50-50: LGTM! Clear and helpful error message.The localization string provides clear feedback to users when an edit operation fails due to an invalid index. The message appropriately explains the likely cause.
3 participants
Check that index >= 0 && index < _searchSources.Count before accessing the collection and show message box if necessary
Fix #4036