Catch exception when creating setting panel #3980
Conversation
|
Be a legend 🏆 by adding a before and after screenshot of the changes you made, especially if they are around UI/UX. |
Jack251970
added
enhancement
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdd guarded creation for plugin setting panels: introduce TryCreateSettingPanel to catch exceptions and return a fallback error UserControl; add two resource-backed margin fields and a logging/class-name constant; add a localization string for the error message. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as UI
participant PVM as PluginViewModel
participant Plug as Plugin (ISettingProvider)
UI->>PVM: Request SettingControl
alt plugin provides settings
PVM->>PVM: TryCreateSettingPanel(pair)
PVM->>Plug: CreateSettingPanel()
alt Create succeeds
Plug-->>PVM: Control
PVM-->>UI: Control
else Create throws
PVM->>PVM: App.API.LogException(...)
PVM->>PVM: Build translated error message
PVM->>PVM: Build fallback UserControl (Grid + TextBox), apply margins & Color04B
PVM-->>UI: Fallback UserControl
end
else plugin has no settings
PVM-->>UI: null / default
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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: 1
🧹 Nitpick comments (1)
Flow.Launcher/ViewModel/PluginViewModel.cs (1)
141-171: Polish the fallback UI: fix Foreground DP, use dynamic resources, and show full exception + scroll
- Use Control.ForegroundProperty on TextBox; TextBlock.ForegroundProperty won’t affect TextBox.
- Bind margins via SetResourceReference so theme changes propagate.
- Show e.ToString() for actionable stack traces.
- Add scrollbars for long exceptions.
Apply this diff:
private static Control TryCreateSettingPanel(PluginPair pair) { try { // We can safely cast here as we already check this in HasSettingControl return ((ISettingProvider)pair.Plugin).CreateSettingPanel(); } catch (System.Exception e) { - var errorMsg = $"Error creating setting panel for plugin {pair.Metadata}\n{e.Message}"; - var grid = new Grid() - { - Margin = SettingPanelMargin - }; + var errorMsg = $"Error creating setting panel for plugin {pair.Metadata}{System.Environment.NewLine}{e}"; + var grid = new Grid(); + grid.SetResourceReference(FrameworkElement.MarginProperty, "SettingPanelMargin"); var textBox = new TextBox { Text = errorMsg, IsReadOnly = true, HorizontalAlignment = HorizontalAlignment.Stretch, VerticalAlignment = VerticalAlignment.Top, TextWrapping = TextWrapping.Wrap, - Margin = SettingPanelItemTopBottomMargin + AcceptsReturn = true }; - textBox.SetResourceReference(TextBlock.ForegroundProperty, "Color04B"); + textBox.SetResourceReference(FrameworkElement.MarginProperty, "SettingPanelItemTopBottomMargin"); + textBox.SetResourceReference(Control.ForegroundProperty, "Color04B"); + textBox.VerticalScrollBarVisibility = ScrollBarVisibility.Auto; + textBox.HorizontalScrollBarVisibility = ScrollBarVisibility.Disabled; grid.Children.Add(textBox); return new UserControl { Content = grid }; } }Optionally, also log the exception to the app’s logger so it’s captured even if the panel isn’t open.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Flow.Launcher/ViewModel/PluginViewModel.cs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
Flow.Launcher/ViewModel/PluginViewModel.cs (2)
Flow.Launcher.Core/Plugin/JsonRPCPluginSettings.cs (1)
Control(155-496)Flow.Launcher.Core/Plugin/JsonRPCPluginBase.cs (1)
Control(144-147)
⏰ 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). (1)
- GitHub Check: build
🔇 Additional comments (1)
Flow.Launcher/ViewModel/PluginViewModel.cs (1)
134-136: Guarded panel creation: good changeUsing a guarded creator here avoids UI breakage when plugins misbehave. Caching via ??= preserves prior behavior.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Flow.Launcher/Languages/en.xaml (1)
222-222: Confirm placeholder order and propagate to other localesLooks good. Please verify the call site passes args in this order: pluginName → Environment.NewLine → exceptionMessage. Also add this key to other language resource files (or confirm fallback behavior).
If you want a blank line before details (matching patterns elsewhere), consider:
- <system:String x:Key="errorCreatingSettingPanel">Error creating setting panel for plugin {0}:{1}{2}</system:String> + <system:String x:Key="errorCreatingSettingPanel">Error creating setting panel for plugin {0}:{1}{1}{2}</system:String>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Flow.Launcher/Languages/en.xaml(1 hunks)Flow.Launcher/ViewModel/PluginViewModel.cs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Flow.Launcher/ViewModel/PluginViewModel.cs
⏰ 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). (4)
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: build
|
🥷 Code experts: onesounds 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: ✨ Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
Flow.Launcher/ViewModel/PluginViewModel.cs (1)
142-143: Avoid static resource lookups at type init; use dynamic resource bindingsApplication.Current.FindResource in static fields can throw before resources load (design-time/tests) and prevents live theme updates. Bind margins dynamically on the created elements instead.
- private static readonly Thickness SettingPanelMargin = (Thickness)Application.Current.FindResource("SettingPanelMargin"); - private static readonly Thickness SettingPanelItemTopBottomMargin = (Thickness)Application.Current.FindResource("SettingPanelItemTopBottomMargin"); @@ - var grid = new Grid() - { - Margin = SettingPanelMargin - }; + var grid = new Grid(); + grid.SetResourceReference(FrameworkElement.MarginProperty, "SettingPanelMargin"); @@ - var textBox = new TextBox - { - Text = errorMsg, - IsReadOnly = true, - HorizontalAlignment = HorizontalAlignment.Stretch, - VerticalAlignment = VerticalAlignment.Top, - TextWrapping = TextWrapping.Wrap, - Margin = SettingPanelItemTopBottomMargin - }; + var textBox = new TextBox + { + Text = errorMsg, + IsReadOnly = true, + HorizontalAlignment = HorizontalAlignment.Stretch, + VerticalAlignment = VerticalAlignment.Top, + TextWrapping = TextWrapping.Wrap + }; + textBox.SetResourceReference(FrameworkElement.MarginProperty, "SettingPanelItemTopBottomMargin");Also applies to: 160-171
🧹 Nitpick comments (1)
Flow.Launcher/ViewModel/PluginViewModel.cs (1)
137-139: Don’t cache the error UI; allow retry on next expandCurrent null-coalescing assignment (“??=”) caches the fallback error panel, preventing recovery if a transient failure occurs. Cache only when creation succeeds.
- public Control SettingControl - => IsExpanded - ? _settingControl - ??= HasSettingControl - ? TryCreateSettingPanel(PluginPair) - : null - : null; + public Control SettingControl + { + get + { + if (!IsExpanded) return null; + if (_settingControl != null) return _settingControl; + if (!HasSettingControl) return null; + + if (TryCreateSettingPanel(PluginPair, out var panel)) + { + _settingControl = panel; // cache success only + } + return panel; + } + } @@ - private static Control TryCreateSettingPanel(PluginPair pair) - { - try - { - // We can safely cast here as we already check this in HasSettingControl - return ((ISettingProvider)pair.Plugin).CreateSettingPanel(); - } - catch (Exception e) - { - // Log exception - App.API.LogException(ClassName, $"Failed to create setting panel for {pair.Metadata.Name}", e); - - // Show error message in UI - var errorMsg = string.Format(App.API.GetTranslation("errorCreatingSettingPanel"), - pair.Metadata.Name, Environment.NewLine, e.Message); - var grid = new Grid() - { - Margin = SettingPanelMargin - }; - var textBox = new TextBox - { - Text = errorMsg, - IsReadOnly = true, - HorizontalAlignment = HorizontalAlignment.Stretch, - VerticalAlignment = VerticalAlignment.Top, - TextWrapping = TextWrapping.Wrap, - Margin = SettingPanelItemTopBottomMargin - }; - textBox.SetResourceReference(TextBlock.ForegroundProperty, "Color04B"); - grid.Children.Add(textBox); - return new UserControl - { - Content = grid - }; - } - } + private static bool TryCreateSettingPanel(PluginPair pair, out Control panel) + { + try + { + // Safe cast: HasSettingControl already checked + panel = ((ISettingProvider)pair.Plugin).CreateSettingPanel(); + return true; + } + catch (Exception e) + { + App.API.LogException(ClassName, $"Failed to create setting panel for {pair.Metadata.Name}", e); + + var errorMsg = string.Format(App.API.GetTranslation("errorCreatingSettingPanel"), + pair.Metadata.Name, Environment.NewLine, e.Message); + + var grid = new Grid(); + grid.SetResourceReference(FrameworkElement.MarginProperty, "SettingPanelMargin"); + + var textBox = new TextBox + { + Text = errorMsg, + IsReadOnly = true, + HorizontalAlignment = HorizontalAlignment.Stretch, + VerticalAlignment = VerticalAlignment.Top, + TextWrapping = TextWrapping.Wrap + }; + textBox.SetResourceReference(FrameworkElement.MarginProperty, "SettingPanelItemTopBottomMargin"); + textBox.SetResourceReference(Control.ForegroundProperty, "Color04B"); + + grid.Children.Add(textBox); + panel = new UserControl { Content = grid }; + return false; + } + }Also applies to: 144-179
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Flow.Launcher/ViewModel/PluginViewModel.cs(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
Flow.Launcher/ViewModel/PluginViewModel.cs (2)
Flow.Launcher.Core/Plugin/JsonRPCPluginSettings.cs (1)
Control(155-496)Flow.Launcher.Core/Plugin/JsonRPCPluginBase.cs (1)
Control(144-147)
⏰ 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). (4)
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: build
🔇 Additional comments (2)
Flow.Launcher/ViewModel/PluginViewModel.cs (2)
18-19: LGTM: logging context constantNameof-based ClassName improves log context and is resilient to refactors.
157-159: Verify translation exists in all language packs and placeholders matchen.xaml contains the key at line 222: <system:String x:Key="errorCreatingSettingPanel">Error creating setting panel for plugin {0}:{1}{2}</system:String>. Confirm this key is present in every Flow.Launcher/Languages/*.xaml and that each translation uses three placeholders in the same order ({0} then {1} then {2}); add or fix translations where missing or mismatched.
There was a problem hiding this comment.
Pull Request Overview
Adds exception handling when creating plugin setting panels to prevent crashes and provides user-friendly error display.
- Wraps plugin setting panel creation in try-catch block
- Displays formatted error message in UI when panel creation fails
- Adds logging for troubleshooting plugin issues
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| Flow.Launcher/ViewModel/PluginViewModel.cs | Implements exception handling with fallback UI for failed setting panel creation |
| Flow.Launcher/Languages/en.xaml | Adds localized error message template for setting panel creation failures |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
Flow.Launcher/ViewModel/PluginViewModel.cs (1)
22-23: Avoid resource lookups at type init; defer with Lazy or resolve at use-siteApplication.Current.FindResource in static initializers can throw (design-time/tests) and blocks live theme updates if resources change. Defer lookup.
Apply this diff to make the lookups lazy and non-eager:
- private static readonly Thickness SettingPanelMargin = (Thickness)Application.Current.FindResource("SettingPanelMargin"); - private static readonly Thickness SettingPanelItemTopBottomMargin = (Thickness)Application.Current.FindResource("SettingPanelItemTopBottomMargin"); + private static readonly Lazy<Thickness> SettingPanelMargin = new(() => + (Thickness)Application.Current.FindResource("SettingPanelMargin")); + private static readonly Lazy<Thickness> SettingPanelItemTopBottomMargin = new(() => + (Thickness)Application.Current.FindResource("SettingPanelItemTopBottomMargin"));And update usages in CreateErrorSettingPanel (see comment below) to use .Value.
🧹 Nitpick comments (1)
Flow.Launcher/ViewModel/PluginViewModel.cs (1)
145-162: Exception path UX/logging nits + i18n key check
- Include plugin ID in the log for disambiguation (Flow.Launcher/ViewModel/PluginViewModel.cs — TryCreateSettingPanel):
- App.API.LogException(ClassName, $"Failed to create setting panel for {pair.Metadata.Name}", e); + App.API.LogException(ClassName, + $"Failed to create setting panel for {pair.Metadata.Name} ({pair.Metadata.ID})", e);
- i18n: Flow.Launcher/Languages/en.xaml contains
"Error creating setting panel for plugin {0}:{1}{2}" — placeholder order {0},{1},{2} matches the current Format call (name, Environment.NewLine, message). The key appears only in en.xaml; add translations to other locale files or confirm English-only.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Flow.Launcher/ViewModel/PluginViewModel.cs(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-05T11:56:27.267Z
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3593
File: Flow.Launcher/HotkeyControlDialog.xaml:6-6
Timestamp: 2025-09-05T11:56:27.267Z
Learning: In Flow.Launcher's migration to iNKORE.UI.WPF.Modern UI framework, dialog resource keys like PopuBGColor, PopupButtonAreaBGColor, PopupButtonAreaBorderColor, and PopupTextColor are provided by the iNKORE.UI.WPF.Modern framework itself, not defined locally in the codebase theme files.
Applied to files:
Flow.Launcher/ViewModel/PluginViewModel.cs
🧬 Code graph analysis (1)
Flow.Launcher/ViewModel/PluginViewModel.cs (2)
Flow.Launcher.Infrastructure/UserSettings/Settings.cs (1)
Settings(16-616)Flow.Launcher.Core/Plugin/JsonRPCPluginSettings.cs (1)
Control(155-496)
⏰ 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). (2)
- GitHub Check: gitStream.cm
- GitHub Check: build
🔇 Additional comments (4)
Flow.Launcher/ViewModel/PluginViewModel.cs (4)
1-2: LGTM: importsNeeded for Environment.NewLine and async members.
18-19: LGTM: explicit class name for loggingKeeping a constant is fine for consistency with other ViewModels.
140-142: Guarded creation in SettingControl is a good safety netThis prevents the UI from crashing on plugin panel failures and caches the result.
215-236: Use Control.ForegroundProperty; add scrollbars; use lazy Margin.ValueReplace SetResourceReference(TextBox.ForegroundProperty, "Color04B") with Control.ForegroundProperty; change Grid.Margin to SettingPanelMargin.Value and TextBox.Margin to SettingPanelItemTopBottomMargin.Value; add VerticalScrollBarVisibility = Auto and HorizontalScrollBarVisibility = Auto on the TextBox. rg shows Color04B is defined (Flow.Launcher/Resources/Light.xaml, Dark.xaml) and TextBox.ForegroundProperty is referenced at Flow.Launcher/ViewModel/PluginViewModel.cs:230 — if you keep TextBox.ForegroundProperty, confirm the project builds.
…etting_panel Catch exception when creating setting panel
Catch exception when creating setting panel
Catch exception when creating setting panel and display the exception for reporting.
Test