Prefer https over http setting for url #4037
Conversation
|
Be a legend 🏆 by adding a before and after screenshot of the changes you made, especially if they are around UI/UX. |
|
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. 📝 WalkthroughWalkthroughAdds a user-configurable preference to use https when inferring URL schemes. Introduces a new settings property, updates URL-building logic to respect it, and exposes a checkbox in the settings UI with a new localized string. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Plugin as Url Plugin (Main)
participant Settings
participant Shell as OS/Shell
User->>Plugin: Enter URL without scheme (e.g., "example.com")
Plugin->>Settings: Read AlwaysOpenWithHttps
Settings-->>Plugin: true/false
Plugin->>Plugin: scheme = (AlwaysOpenWithHttps ? "https" : "http")
Plugin->>Shell: Open scheme + "://" + input
Shell-->>User: Browser opens constructed URL
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🪛 GitHub Check: Check SpellingPlugins/Flow.Launcher.Plugin.Url/SettingsControl.xaml[warning] 123-123: ⏰ 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). (3)
🔇 Additional comments (2)
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 (2)
Plugins/Flow.Launcher.Plugin.Url/SettingsViewModel.cs (1)
1-2: Remove unused imports.Neither
System.Collections.GenericnorSystem.Linqare used in this file.Apply this diff:
-using System.Collections.Generic; -using System.Linq; - namespace Flow.Launcher.Plugin.Url;Plugins/Flow.Launcher.Plugin.Url/URLSettings.xaml.cs (1)
7-7: Consider removing the unused field.The
_viewModelfield is assigned but never read. SinceDataContextis set directly, the field is not needed.Apply this diff if you want to simplify:
public partial class URLSettings : UserControl { - private readonly SettingsViewModel _viewModel; - public URLSettings(Settings settings) { - _viewModel = new SettingsViewModel(settings); - DataContext = _viewModel; + DataContext = new SettingsViewModel(settings); InitializeComponent(); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
Plugins/Flow.Launcher.Plugin.Url/Languages/de.xaml(1 hunks)Plugins/Flow.Launcher.Plugin.Url/Languages/en.xaml(1 hunks)Plugins/Flow.Launcher.Plugin.Url/Main.cs(4 hunks)Plugins/Flow.Launcher.Plugin.Url/Settings.cs(1 hunks)Plugins/Flow.Launcher.Plugin.Url/SettingsViewModel.cs(1 hunks)Plugins/Flow.Launcher.Plugin.Url/URLSettings.xaml(1 hunks)Plugins/Flow.Launcher.Plugin.Url/URLSettings.xaml.cs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
Plugins/Flow.Launcher.Plugin.Url/SettingsViewModel.cs (2)
Plugins/Flow.Launcher.Plugin.Url/Settings.cs (1)
Settings(3-9)Plugins/Flow.Launcher.Plugin.ProcessKiller/ViewModels/SettingsViewModel.cs (1)
SettingsViewModel(3-6)
Plugins/Flow.Launcher.Plugin.Url/URLSettings.xaml.cs (2)
Plugins/Flow.Launcher.Plugin.Url/SettingsViewModel.cs (1)
SettingsViewModel(6-9)Plugins/Flow.Launcher.Plugin.Url/Settings.cs (1)
Settings(3-9)
Plugins/Flow.Launcher.Plugin.Url/Main.cs (1)
Plugins/Flow.Launcher.Plugin.Url/URLSettings.xaml.cs (2)
URLSettings(5-15)URLSettings(9-14)
🪛 GitHub Check: Check Spelling
Plugins/Flow.Launcher.Plugin.Url/URLSettings.xaml
[warning] 28-28:
usehttps is not a recognized word. (unrecognized-spelling)
🔇 Additional comments (8)
Plugins/Flow.Launcher.Plugin.Url/Languages/de.xaml (1)
17-19: LGTM!The German localization for the HTTPS preference setting is properly added and the translation is accurate.
Plugins/Flow.Launcher.Plugin.Url/Settings.cs (1)
8-8: LGTM!The new
AlwaysOpenWithHttpsproperty is well-named with a sensible default that maintains backward compatibility.Plugins/Flow.Launcher.Plugin.Url/SettingsViewModel.cs (1)
6-9: LGTM!The ViewModel structure is clean and follows the pattern used in other plugins (e.g., ProcessKiller plugin). The primary constructor syntax is modern and concise.
Plugins/Flow.Launcher.Plugin.Url/Languages/en.xaml (1)
18-19: LGTM!The English localization for the HTTPS preference setting is properly added and clearly worded.
Plugins/Flow.Launcher.Plugin.Url/URLSettings.xaml (1)
1-31: LGTM!The settings UI is properly structured with correct two-way binding for the checkbox and dynamic resource lookup for localization. The static analysis warning about "usehttps" is a false positive—it's part of a valid resource key name.
Plugins/Flow.Launcher.Plugin.Url/Main.cs (3)
66-76: LGTM!The try-catch block properly handles URL opening failures and displays an error message to the user, improving the robustness of the plugin.
84-87: LGTM!The
GetHttpPreference()helper method is clear and concise, encapsulating the preference logic.
123-126: LGTM!The
CreateSettingPanel()implementation correctly returns a settings UI bound to the plugin's settings instance.
There was a problem hiding this comment.
Pull Request Overview
This PR implements a user setting to prefer HTTPS over HTTP when opening URLs that don't explicitly specify a protocol. The change allows users to configure the plugin to default to HTTPS instead of HTTP for better security practices.
Key changes:
- Added a new setting
AlwaysOpenWithHttpsto control protocol preference - Created a settings UI panel with a checkbox to toggle HTTPS preference
- Updated URL handling logic to use the preferred protocol based on user setting
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| Settings.cs | Added new boolean property for HTTPS preference setting |
| Main.cs | Updated URL protocol logic and added settings panel support |
| URLSettings.xaml | Created settings UI with HTTPS preference checkbox |
| URLSettings.xaml.cs | Code-behind for the settings panel |
| SettingsViewModel.cs | View model to bind settings to the UI |
| Languages/en.xaml | Added English translation for HTTPS preference label |
| Languages/de.xaml | Added German translation for HTTPS preference label |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
Plugins/Flow.Launcher.Plugin.Url/Main.cs (1)
84-87: Fix formatting and verify null safety.Two concerns:
- Missing space after the colon in the ternary operator (for consistent formatting).
- Verify that
_settingscannot be null when this method is called. While the plugin framework typically ensuresInit()runs first, confirm thatLoadSettingJsonStorage<Settings>()at line 110 never returns null, or add a null-check/default fallback.Apply this diff to fix the formatting:
- return _settings.AlwaysOpenWithHttps ? "https" : "http"; + return _settings.AlwaysOpenWithHttps ? "https" : "http";To verify null safety, run the following script:
#!/bin/bash # Description: Check if LoadSettingJsonStorage can return null and verify plugin initialization order # Search for LoadSettingJsonStorage implementation to check null handling rg -n -A 10 "LoadSettingJsonStorage" --type=cs # Search for ISettingProvider.CreateSettingPanel calls to verify if Init is guaranteed first rg -n -B 3 -A 3 "CreateSettingPanel" --type=cs
🧹 Nitpick comments (1)
Plugins/Flow.Launcher.Plugin.Url/Main.cs (1)
66-76: Good error handling, consider more specific exceptions.The try/catch block prevents crashes and provides user feedback, which is appropriate for a plugin. However, catching all
Exceptiontypes is very broad.Consider catching more specific exceptions if you want to provide different error messages:
try { Context.API.OpenUrl(raw); return true; } -catch(Exception) +catch (UriFormatException ex) { - Context.API.ShowMsgError(Localize.flowlauncher_plugin_url_cannot_open_url(raw)); + Context.API.ShowMsgError($"Invalid URL format: {raw}"); + return false; +} +catch (Exception) +{ + Context.API.ShowMsgError(Localize.flowlauncher_plugin_url_cannot_open_url(raw)); return false; }Note: If the current broad catch is intentional to handle all edge cases gracefully, you can disregard this suggestion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Plugins/Flow.Launcher.Plugin.Url/Main.cs(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
Plugins/Flow.Launcher.Plugin.Url/Main.cs (1)
Plugins/Flow.Launcher.Plugin.Url/URLSettings.xaml.cs (2)
URLSettings(5-15)URLSettings(9-14)
⏰ 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: gitStream.cm
🔇 Additional comments (4)
Plugins/Flow.Launcher.Plugin.Url/Main.cs (4)
4-4: LGTM!The import is necessary for the
Controlreturn type inCreateSettingPanel().
8-8: LGTM!The
ISettingProviderimplementation is correctly added to enable settings UI integration.
62-65: LGTM! Previous critical issue resolved.The scheme detection logic now correctly checks for both
http://andhttps://explicitly, resolving the previous bug where "https" would match StartsWith("http"). URLs without an explicit scheme now get the preferred scheme prefix, while existing schemes are preserved.
123-126: Guard against_settingsbeing null in CreateSettingPanel
Add a null check to_settings(e.g., throw or fallback) to prevent a NullReferenceException ifInit()hasn’t run, or confirm with the host framework thatInit()is guaranteed to execute beforeCreateSettingPanel().
|
Merged 3944. |
4 participants
Closes #3773
This is still missing some translations though and it also adds a settings panel to the url plugin.
There are some other unused properties in this plugin, not sure if these can be deleted?
Also default behaviour is http but I think https should be the state of the art at this point