Update UrlPattern in URL plugin to include private addresses and update test cases #4185
Conversation
|
🥷 Code experts: Jack251970 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 |
|
Be a legend 🏆 by adding a before and after screenshot of the changes you made, especially if they are around UI/UX. |
There was a problem hiding this comment.
Pull request overview
This PR updates the URL plugin to support private IP addresses (like 192.168.x.x, 10.x.x.x, 172.16-31.x.x) and adds comprehensive IPv6 support. The regex pattern has been rewritten to remove the exclusion of private networks and add explicit support for IPv6 addresses with and without brackets.
Key Changes
- Removed private IP address exclusions from the URL regex pattern
- Added IPv6 address support (both bracketed and non-bracketed formats)
- Simplified the
IsURLmethod by removing redundant localhost checks (now handled by regex) - Refactored
Querymethod to invert the conditional logic for better readability
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| Plugins/Flow.Launcher.Plugin.Url/Main.cs | Updated URL regex to include private addresses and IPv6, simplified IsURL method, refactored Query method structure, added UrlSchemes array for scheme detection |
| Flow.Launcher.Test/Plugins/UrlPluginTest.cs | Added comprehensive test cases for private IPs, IPv6 addresses, localhost variations, case insensitivity, and negative test cases |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "(?:(?:[0-9a-fA-F]{1,4}:){7}[0-9a-fA-F]{1,4}|" + // standard IPv6 | ||
| "(?:[0-9a-fA-F]{1,4}:){1,7}:|" + // IPv6 with trailing :: | ||
| "(?:[0-9a-fA-F]{1,4}:){1,6}:[0-9a-fA-F]{1,4}|" + // IPv6 compressed | ||
| "::(?:[0-9a-fA-F]{1,4}:){0,6}[0-9a-fA-F]{1,4}|" + // IPv6 with leading :: | ||
| "::1)(?!:[0-9])" + // IPv6 loopback (not followed by port) |
There was a problem hiding this comment.
The IPv6 regex patterns are incomplete and won't match many valid compressed IPv6 addresses. For example, fe80:1:2::3:4:5:6 (where :: appears in the middle with multiple segments on both sides) won't match any of these patterns. Line 30's pattern (?:[0-9a-fA-F]{1,4}:){1,6}:[0-9a-fA-F]{1,4} expects the final segment to have no colons, but in addresses like fe80:1:2::3:4, there are multiple segments after the ::. A comprehensive IPv6 regex would need additional patterns to cover all valid positions and combinations of the :: compression.
There was a problem hiding this comment.
Added a test case to valid.
| ")" + | ||
| // port number | ||
| "(?::\\d{2,5})?" + | ||
| "(?::\\d{1,5})?" + |
There was a problem hiding this comment.
The port number pattern allows single-digit ports (1-9) with the pattern \d{1,5}, but also allows invalid port numbers up to 99999. Valid TCP/UDP port numbers range from 1 to 65535. The pattern should validate that the port number is within this valid range, or at minimum should be \d{2,5} to match the original pattern (though that still allows invalid ports like 99999). Consider using a more restrictive pattern like (?:6553[0-5]|655[0-2]\d|65[0-4]\d{2}|6[0-4]\d{3}|[1-5]\d{4}|[1-9]\d{0,3}) to properly validate port numbers.
| "(?::\\d{1,5})?" + | |
| "(?::(?:6553[0-5]|655[0-2]\\d|65[0-4]\\d{2}|6[0-4]\\d{3}|[1-5]\\d{4}|[1-9]\\d{0,3}))?" + |
There was a problem hiding this comment.
It's right but not necessary.
|
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. 📝 WalkthroughWalkthroughRefactors URL plugin tests to parameterized cases and injects runtime settings; replaces permissive URL handling with normalized, regex-plus-Uri.TryCreate validation, scheme normalization (http/https/ftp), IPv4/IPv6/localhost support, and adjusted Query result construction. Changes
Sequence Diagram(s)(Skipped — conditions for a diagram are not met.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 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: 2
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Flow.Launcher.Test/Plugins/UrlPluginTest.csPlugins/Flow.Launcher.Plugin.Url/Main.cs
🧰 Additional context used
🧠 Learnings (7)
📚 Learning: 2025-09-04T11:52:29.096Z
Learnt from: jjw24
Repo: Flow-Launcher/Flow.Launcher PR: 3932
File: Flow.Launcher.Core/ExternalPlugins/PluginsManifest.cs:48-55
Timestamp: 2025-09-04T11:52:29.096Z
Learning: In Flow Launcher's PluginsManifest.cs, when dealing with version parsing for the MinimumAppVersion feature, maintainer jjw24 prefers to keep the solution simple rather than implementing comprehensive helper methods for SemVer parsing normalization.
Applied to files:
Plugins/Flow.Launcher.Plugin.Url/Main.cs
📚 Learning: 2025-10-16T10:48:30.573Z
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3854
File: Flow.Launcher/App.xaml.cs:252-275
Timestamp: 2025-10-16T10:48:30.573Z
Learning: In Flow Launcher's App.xaml.cs, the plugin initialization block (lines 252-275) that includes PreStartPluginExecutablePathUpdate, PluginManager.LoadPlugins, PluginManager.InitializePluginsAsync, _mainVM.QueryResults(), and API.SaveAppAllSettings() does not require additional Task.Run wrappers or Dispatcher.InvokeAsync calls according to maintainer Jack251970, as the threading model is already safe as designed.
Applied to files:
Plugins/Flow.Launcher.Plugin.Url/Main.cs
📚 Learning: 2025-07-21T09:19:49.684Z
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3854
File: Flow.Launcher/App.xaml.cs:246-262
Timestamp: 2025-07-21T09:19:49.684Z
Learning: In Flow Launcher's App.xaml.cs, the asynchronous plugin initialization task (containing AbstractPluginEnvironment.PreStartPluginExecutablePathUpdate, PluginManager.LoadPlugins, PluginManager.InitializePluginsAsync, and AutoPluginUpdates) does not require additional try-catch error handling according to maintainer Jack251970, as these operations are designed to handle exceptions internally.
Applied to files:
Plugins/Flow.Launcher.Plugin.Url/Main.cs
📚 Learning: 2025-07-21T09:19:19.012Z
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3854
File: Flow.Launcher.Core/Plugin/PluginManager.cs:280-292
Timestamp: 2025-07-21T09:19:19.012Z
Learning: In Flow Launcher's PluginManager.cs, the post-initialization operations (RegisterResultsUpdatedEvent, UpdatePluginMetadataTranslation, RegisterPluginActionKeywords, DialogJump.InitializeDialogJumpPlugin, and AddPluginToLists) are designed to be exception-safe and do not require additional try-catch error handling according to the maintainer Jack251970.
Applied to files:
Plugins/Flow.Launcher.Plugin.Url/Main.cs
📚 Learning: 2024-11-03T07:40:11.014Z
Learnt from: Yusyuriv
Repo: Flow-Launcher/Flow.Launcher PR: 3057
File: Flow.Launcher.Core/Plugin/JsonRPCPluginSettings.cs:0-0
Timestamp: 2024-11-03T07:40:11.014Z
Learning: In Flow Launcher, when using Windows Forms dialogs (e.g., in `JsonRPCPluginSettings.cs`), path validation is enabled by default in `OpenFileDialog` and `FolderBrowserDialog`, preventing users from selecting invalid paths, but it's possible to opt out of this validation on individual dialogs.
Applied to files:
Plugins/Flow.Launcher.Plugin.Url/Main.cs
📚 Learning: 2025-09-05T11:56:27.267Z
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 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:
Plugins/Flow.Launcher.Plugin.Url/Main.cs
📚 Learning: 2025-07-06T12:21:37.947Z
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3572
File: Flow.Launcher/App.xaml.cs:214-216
Timestamp: 2025-07-06T12:21:37.947Z
Learning: In Flow Launcher, the UpdatePluginManifestAsync method in PluginsManifest.cs already has comprehensive internal try-catch handling that logs exceptions and returns false on failure rather than throwing, making external try-catch wrappers unnecessary.
Applied to files:
Plugins/Flow.Launcher.Plugin.Url/Main.cs
🧬 Code graph analysis (2)
Flow.Launcher.Test/Plugins/UrlPluginTest.cs (1)
Plugins/Flow.Launcher.Plugin.Url/Main.cs (1)
IsURL(116-121)
Plugins/Flow.Launcher.Plugin.Url/Main.cs (1)
Plugins/Flow.Launcher.Plugin.Url/Settings.cs (1)
Settings(3-52)
🪛 GitHub Check: Check Spelling
Flow.Launcher.Test/Plugins/UrlPluginTest.cs
[warning] 19-19:
google is not a recognized word. (unrecognized-spelling)
[warning] 18-18:
google is not a recognized word. (unrecognized-spelling)
[warning] 19-19:
google is not a recognized word. (unrecognized-spelling)
⏰ 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: CodeQL analysis (csharp)
- GitHub Check: Agent
- GitHub Check: gitStream.cm
- GitHub Check: build
🔇 Additional comments (6)
Flow.Launcher.Test/Plugins/UrlPluginTest.cs (2)
17-39: Good test coverage for new URL patterns.The new test cases comprehensively cover the expanded URL detection requirements: FTP scheme, localhost variants, IPv4 with ports, and IPv6 addresses (both bracketed and non-bracketed forms). These align well with the regex changes in
Main.cs.
46-57: Good negative test coverage for edge cases.The negative tests ensure proper rejection of invalid inputs including scheme-only URLs, invalid IPv4 octets, and bare words without TLDs. Note: The static analysis warnings about "google" spelling are false positives for test URL data and can be safely ignored.
Plugins/Flow.Launcher.Plugin.Url/Main.cs (4)
19-32: IPv6 patterns are well-structured.The dual approach with bracketed and non-bracketed IPv6 patterns is appropriate. The negative lookahead
(?!:[0-9])correctly prevents non-bracketed IPv6 addresses from being confused with port specifications.
34-35: IPv4 pattern correctly excludes 0.0.0.0 while allowing all valid private addresses.The first octet pattern
[1-9]\\d|[1-9]ensures addresses starting with 0 are rejected, which aligns with the PR objective to support private/internal network addresses while excluding invalid edge cases.
58-109: Query flow improvements look good.The early return pattern for non-URLs is clean, and the scheme detection using
UrlSchemes.Any()withStringComparison.OrdinalIgnoreCasecorrectly handles case-insensitive scheme matching. The default scheme addition based on user preference (AlwaysOpenWithHttpssetting fromSettings.cs) is properly integrated.
48-48: Port pattern change allows single-digit ports.The change from
{2,5}to{1,5}correctly allows single-digit ports. Note that this doesn't validate the port range (1-65535), but invalid ports like:99999are uncommon in practice and would fail when attempting to open anyway.
|
Could you please resolve the reviews from AI? And I am not sure. Need jjw24's review |
|
@VictoriousRaptor I've opened a new pull request, #4187, to work on those changes. Once the pull request is ready, I'll request review from you. |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Flow.Launcher.Test/Plugins/UrlPluginTest.cs
🧰 Additional context used
🧬 Code graph analysis (1)
Flow.Launcher.Test/Plugins/UrlPluginTest.cs (1)
Plugins/Flow.Launcher.Plugin.Url/Main.cs (1)
IsURL(116-123)
🪛 GitHub Check: Check Spelling
Flow.Launcher.Test/Plugins/UrlPluginTest.cs
[warning] 19-19:
google is not a recognized word. (unrecognized-spelling)
[warning] 18-18:
google is not a recognized word. (unrecognized-spelling)
[warning] 19-19:
google is not a recognized word. (unrecognized-spelling)
⏰ 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 (6)
Flow.Launcher.Test/Plugins/UrlPluginTest.cs (6)
17-17: LGTM!Adding FTP scheme support is appropriate and aligns with comprehensive URL handling.
24-25: LGTM!Supporting
localhostandlocalhost:8080without explicit schemes is valuable for development workflows.
28-30: LGTM!IPv4 with ports and private address support (e.g.,
192.168.1.1,192.168.1.1:3000) directly addresses the PR objective to support private IP addresses.
32-37: LGTM!IPv6 URLs with bracket notation are correctly formatted for URL usage and comprehensive (with/without ports, with/without schemes).
50-53: LGTM!The negative test cases comprehensively cover invalid URL patterns:
- Strings with spaces
- Incomplete schemes
- Invalid IPv4 octets (
256.1.1.1)- Incomplete domain names
Also applies to: 55-58
41-44: Case-sensitivity fix confirmed in code — uppercase URLs will be handled correctly.The uppercase URL tests at lines 41-44 will pass because the
IsURLmethod (lines 116-123 in Main.cs) converts input to lowercase on line 118 before performing the regex match on line 120. Inputs like"HTTP://EXAMPLE.COM"are normalized to"http://example.com"and matched against the regex pattern, which includesRegexOptions.IgnoreCaseas an additional safeguard.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ClassicAssert.IsFalse(plugin.IsURL("example")); // No TLD | ||
| ClassicAssert.IsFalse(plugin.IsURL(".com")); | ||
| ClassicAssert.IsFalse(plugin.IsURL("http://.com")); | ||
| } |
There was a problem hiding this comment.
Missing test coverage for invalid port numbers. The regex pattern allows port numbers up to 99999 (5 digits), but valid TCP/UDP ports range from 0 to 65535. Consider adding test cases like "example.com:99999" or "192.168.1.1:70000" to verify they are correctly rejected or accepted based on the intended behavior.
| ClassicAssert.IsFalse(plugin.IsURL("example")); // No TLD | ||
| ClassicAssert.IsFalse(plugin.IsURL(".com")); | ||
| ClassicAssert.IsFalse(plugin.IsURL("http://.com")); | ||
| } |
There was a problem hiding this comment.
Missing test coverage for IPv6 addresses with embedded IPv4 notation (e.g., "::ffff:192.0.2.1" or "64:ff9b::192.0.2.1"). These are valid IPv6 formats that may be used in URLs but are not covered by the current test cases or the regex patterns.
There was a problem hiding this comment.
IPv6 is a disaster.
| "(?:[0-9a-fA-F]{1,4}:){1,6}:[0-9a-fA-F]{1,4}|" + // IPv6 compressed | ||
| "::(?:[0-9a-fA-F]{1,4}:){0,6}[0-9a-fA-F]{1,4}|" + // IPv6 with leading :: | ||
| "(?:(?:[0-9a-fA-F]{1,4}:){1,6}|:):(?:[0-9a-fA-F]{1,4}:){0,5}[0-9a-fA-F]{1,4}|" + // IPv6 with :: in the middle | ||
| "::1)(?!:[0-9])" + // IPv6 loopback (not followed by port) |
There was a problem hiding this comment.
The negative lookahead (?!:[0-9]) used to prevent matching ports with unbracketed IPv6 addresses is insufficient. It only prevents matching when immediately followed by a colon and a single digit, but ports can have multiple digits (e.g., :8080, :3000). This could cause the regex to incorrectly match an IPv6 address that is actually followed by a port number. The pattern should use (?!:[0-9]+) or (?!:\\d) to properly prevent matching any port number.
| "::1)(?!:[0-9])" + // IPv6 loopback (not followed by port) | |
| "::1)(?!:[0-9]+)" + // IPv6 loopback (not followed by port) |
| // IPv6 address with optional brackets (brackets required if followed by port) | ||
| // IPv6 with brackets | ||
| "(?:\\[(?:[0-9a-fA-F]{1,4}:){7}[0-9a-fA-F]{1,4}\\]|" + // standard IPv6 | ||
| "\\[(?:[0-9a-fA-F]{1,4}:){1,7}:\\]|" + // IPv6 with trailing :: | ||
| "\\[(?:[0-9a-fA-F]{1,4}:){1,6}:[0-9a-fA-F]{1,4}\\]|" + // IPv6 compressed | ||
| "\\[::(?:[0-9a-fA-F]{1,4}:){0,6}[0-9a-fA-F]{1,4}\\]|" + // IPv6 with leading :: | ||
| "\\[(?:(?:[0-9a-fA-F]{1,4}:){1,6}|:):(?:[0-9a-fA-F]{1,4}:){0,5}[0-9a-fA-F]{1,4}\\]|" + // IPv6 with :: in the middle | ||
| "\\[::1\\])" + // IPv6 loopback | ||
| "|" + | ||
| // IPv6 without brackets (only when no port follows) | ||
| "(?:(?:[0-9a-fA-F]{1,4}:){7}[0-9a-fA-F]{1,4}|" + // standard IPv6 | ||
| "(?:[0-9a-fA-F]{1,4}:){1,7}:|" + // IPv6 with trailing :: | ||
| "(?:[0-9a-fA-F]{1,4}:){1,6}:[0-9a-fA-F]{1,4}|" + // IPv6 compressed | ||
| "::(?:[0-9a-fA-F]{1,4}:){0,6}[0-9a-fA-F]{1,4}|" + // IPv6 with leading :: | ||
| "(?:(?:[0-9a-fA-F]{1,4}:){1,6}|:):(?:[0-9a-fA-F]{1,4}:){0,5}[0-9a-fA-F]{1,4}|" + // IPv6 with :: in the middle | ||
| "::1)(?!:[0-9])" + // IPv6 loopback (not followed by port) |
There was a problem hiding this comment.
The IPv6 patterns are incomplete and don't cover all valid IPv6 address formats. For example, they don't properly handle IPv6 addresses with embedded IPv4 addresses (e.g., ::ffff:192.0.2.1), various forms of :: compression, or all valid positions where :: can appear. The current patterns have multiple alternatives but miss several valid IPv6 formats that could be used in URLs. Consider using a more comprehensive IPv6 regex pattern or leveraging existing libraries for validation.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Plugins/Flow.Launcher.Plugin.Url/Main.cs (1)
118-125: Simplify IsURL implementation using IsMatch().The current implementation works correctly but contains redundant logic. The
ToLower()call is unnecessary becauseUrlRegexalready hasRegexOptions.IgnoreCase(line 54). Additionally, usingIsMatch()is more idiomatic than comparingMatch().Valueagainst the input string.🔎 Proposed refactor
public bool IsURL(string raw) { - raw = raw.ToLower(); - - if (UrlRegex.Match(raw).Value == raw) return true; - - return false; + return UrlRegex.IsMatch(raw); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Plugins/Flow.Launcher.Plugin.Url/Main.cs
🧰 Additional context used
🧠 Learnings (7)
📚 Learning: 2025-09-04T11:52:29.096Z
Learnt from: jjw24
Repo: Flow-Launcher/Flow.Launcher PR: 3932
File: Flow.Launcher.Core/ExternalPlugins/PluginsManifest.cs:48-55
Timestamp: 2025-09-04T11:52:29.096Z
Learning: In Flow Launcher's PluginsManifest.cs, when dealing with version parsing for the MinimumAppVersion feature, maintainer jjw24 prefers to keep the solution simple rather than implementing comprehensive helper methods for SemVer parsing normalization.
Applied to files:
Plugins/Flow.Launcher.Plugin.Url/Main.cs
📚 Learning: 2025-10-16T10:48:30.573Z
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3854
File: Flow.Launcher/App.xaml.cs:252-275
Timestamp: 2025-10-16T10:48:30.573Z
Learning: In Flow Launcher's App.xaml.cs, the plugin initialization block (lines 252-275) that includes PreStartPluginExecutablePathUpdate, PluginManager.LoadPlugins, PluginManager.InitializePluginsAsync, _mainVM.QueryResults(), and API.SaveAppAllSettings() does not require additional Task.Run wrappers or Dispatcher.InvokeAsync calls according to maintainer Jack251970, as the threading model is already safe as designed.
Applied to files:
Plugins/Flow.Launcher.Plugin.Url/Main.cs
📚 Learning: 2025-07-21T09:19:49.684Z
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3854
File: Flow.Launcher/App.xaml.cs:246-262
Timestamp: 2025-07-21T09:19:49.684Z
Learning: In Flow Launcher's App.xaml.cs, the asynchronous plugin initialization task (containing AbstractPluginEnvironment.PreStartPluginExecutablePathUpdate, PluginManager.LoadPlugins, PluginManager.InitializePluginsAsync, and AutoPluginUpdates) does not require additional try-catch error handling according to maintainer Jack251970, as these operations are designed to handle exceptions internally.
Applied to files:
Plugins/Flow.Launcher.Plugin.Url/Main.cs
📚 Learning: 2025-07-21T09:19:19.012Z
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3854
File: Flow.Launcher.Core/Plugin/PluginManager.cs:280-292
Timestamp: 2025-07-21T09:19:19.012Z
Learning: In Flow Launcher's PluginManager.cs, the post-initialization operations (RegisterResultsUpdatedEvent, UpdatePluginMetadataTranslation, RegisterPluginActionKeywords, DialogJump.InitializeDialogJumpPlugin, and AddPluginToLists) are designed to be exception-safe and do not require additional try-catch error handling according to the maintainer Jack251970.
Applied to files:
Plugins/Flow.Launcher.Plugin.Url/Main.cs
📚 Learning: 2024-11-03T07:40:11.014Z
Learnt from: Yusyuriv
Repo: Flow-Launcher/Flow.Launcher PR: 3057
File: Flow.Launcher.Core/Plugin/JsonRPCPluginSettings.cs:0-0
Timestamp: 2024-11-03T07:40:11.014Z
Learning: In Flow Launcher, when using Windows Forms dialogs (e.g., in `JsonRPCPluginSettings.cs`), path validation is enabled by default in `OpenFileDialog` and `FolderBrowserDialog`, preventing users from selecting invalid paths, but it's possible to opt out of this validation on individual dialogs.
Applied to files:
Plugins/Flow.Launcher.Plugin.Url/Main.cs
📚 Learning: 2025-09-05T11:56:27.267Z
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 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:
Plugins/Flow.Launcher.Plugin.Url/Main.cs
📚 Learning: 2025-07-06T12:21:37.947Z
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3572
File: Flow.Launcher/App.xaml.cs:214-216
Timestamp: 2025-07-06T12:21:37.947Z
Learning: In Flow Launcher, the UpdatePluginManifestAsync method in PluginsManifest.cs already has comprehensive internal try-catch handling that logs exceptions and returns false on failure rather than throwing, making external try-catch wrappers unnecessary.
Applied to files:
Plugins/Flow.Launcher.Plugin.Url/Main.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). (1)
- GitHub Check: build
🔇 Additional comments (5)
Plugins/Flow.Launcher.Plugin.Url/Main.cs (5)
1-3: LGTM: Appropriate using directive for LINQ.The addition of
System.Linqis correctly used for theAny()check on line 79.
58-58: LGTM: Centralized scheme definitions improve maintainability.The
UrlSchemesfield provides a single source of truth for recognized schemes, making the code more maintainable and the scheme check on line 79 more readable.
63-66: LGTM: Early return improves code clarity.Returning an empty list early when the input is not a URL reduces nesting and improves readability.
78-82: LGTM: Scheme detection logic is correct.The case-insensitive scheme check using
UrlSchemes.Any()withStringComparison.OrdinalIgnoreCaseproperly handles URLs with and without explicit schemes.
19-34: The IPv6 middle-compression regex pattern at lines 16 and 24 does correctly match all valid compressed IPv6 addresses with::in the middle. Test verification confirms that addresses likefe80:1:2::3:4:5:6and2001:db8:85a3::8a2e:370:7334match successfully.The pattern works because the leading segment group
(?:[0-9a-fA-F]{1,4}:){1,6}consumes segments with their trailing colons (e.g.,fe80:1:2:), the subsequent:matches the first colon of::, and the trailing pattern(?:[0-9a-fA-F]{1,4}:){0,5}[0-9a-fA-F]{1,4}matches the remaining segments. The concern about the remaining string starting with:3:4:5:6is based on a misunderstanding of how the regex engine processes the pattern sequentially.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
Plugins/Flow.Launcher.Plugin.Url/Main.cs (1)
137-162: Consider simplifying IPv6 bracket-wrapping logic.The nested conditional logic for wrapping bare IPv6 addresses in brackets (lines 137-162) is functionally correct but complex and difficult to follow. The multiple levels of nested conditions make maintenance and debugging challenging.
🔎 Suggested refactor
Extract the IPv6 detection and bracket-wrapping into a helper method:
+ private string PrepareUrlForValidation(string input) + { + // Add scheme if missing + if (UrlSchemes.Any(s => input.StartsWith(s, StringComparison.OrdinalIgnoreCase))) + { + return input; + } + + // Detect bare IPv6: multiple colons, no scheme, no auth + var isBareIpv6 = input.Count(c => c == ':') > 1 + && !input.Contains("://") + && !input.Contains('@'); + + if (isBareIpv6 && !input.StartsWith('[')) + { + // Wrap in brackets unless already in format [ipv6]:port + var hasPortWithBrackets = Regex.IsMatch(input, @"\]:\d{1,5}$"); + if (!hasPortWithBrackets) + { + input = $"[{input}]"; + } + } + + return $"{GetHttpPreference()}://{input}"; + } public bool IsURL(string raw) { // ... initial validation ... - // Prepare URL for Uri.TryCreate validation - var urlToValidate = input; - - // Add scheme if missing - if (!UrlSchemes.Any(s => input.StartsWith(s, StringComparison.OrdinalIgnoreCase))) - { - // Handle bare IPv6 addresses (multiple colons, no @ for auth, no :// for scheme) - if (input.Count(c => c == ':') > 1 && !input.Contains("://") && !input.Contains('@')) - { - // Add brackets if not already present - if (!input.StartsWith('[')) - { - // Check if there's a port at the end (]:port pattern) - var portMatch = Regex.Match(input, @"\]:(\d{1,5})$"); - if (!portMatch.Success) - { - urlToValidate = $"{GetHttpPreference()}://[{input}]"; - } - else - { - urlToValidate = GetHttpPreference() + "://" + input; - } - } - else - { - urlToValidate = GetHttpPreference() + "://" + input; - } - } - else - { - urlToValidate = GetHttpPreference() + "://" + input; - } - } + var urlToValidate = PrepareUrlForValidation(input);This approach also allows reusing the helper in the Action handler to fix the critical issue noted in lines 63-67.
Flow.Launcher.Test/Plugins/UrlPluginTest.cs (1)
18-25: Reflection-based settings injection is acceptable but brittle.Using reflection to set the
internal static Settingsproperty is a pragmatic solution given the current visibility constraints. TheAssert.Failprovides clear feedback if the reflection breaks. However, this approach is brittle - any refactoring that changes the property name or visibility will silently break tests.Consider one of these alternatives for better maintainability:
- Make
Settingsproperty have internal setter with[assembly: InternalsVisibleTo("Flow.Launcher.Test")]in Main.cs- Add a test-only initialization method like
internal static void InitializeForTest(Settings settings)(pollutes production code but is explicit)- Document the current reflection dependency prominently so future refactorings don't break it unknowingly
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Flow.Launcher.Test/Plugins/UrlPluginTest.csPlugins/Flow.Launcher.Plugin.Url/Main.cs
🧰 Additional context used
🧠 Learnings (9)
📚 Learning: 2025-09-04T11:52:29.096Z
Learnt from: jjw24
Repo: Flow-Launcher/Flow.Launcher PR: 3932
File: Flow.Launcher.Core/ExternalPlugins/PluginsManifest.cs:48-55
Timestamp: 2025-09-04T11:52:29.096Z
Learning: In Flow Launcher's PluginsManifest.cs, when dealing with version parsing for the MinimumAppVersion feature, maintainer jjw24 prefers to keep the solution simple rather than implementing comprehensive helper methods for SemVer parsing normalization.
Applied to files:
Plugins/Flow.Launcher.Plugin.Url/Main.csFlow.Launcher.Test/Plugins/UrlPluginTest.cs
📚 Learning: 2024-11-03T07:40:11.014Z
Learnt from: Yusyuriv
Repo: Flow-Launcher/Flow.Launcher PR: 3057
File: Flow.Launcher.Core/Plugin/JsonRPCPluginSettings.cs:0-0
Timestamp: 2024-11-03T07:40:11.014Z
Learning: In Flow Launcher, when using Windows Forms dialogs (e.g., in `JsonRPCPluginSettings.cs`), path validation is enabled by default in `OpenFileDialog` and `FolderBrowserDialog`, preventing users from selecting invalid paths, but it's possible to opt out of this validation on individual dialogs.
Applied to files:
Plugins/Flow.Launcher.Plugin.Url/Main.cs
📚 Learning: 2025-07-01T05:46:13.251Z
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3791
File: Flow.Launcher.Core/Plugin/PluginManager.cs:293-295
Timestamp: 2025-07-01T05:46:13.251Z
Learning: In Flow.Launcher.Core/Plugin/PluginManager.cs, when checking if a plugin is modified within the PluginManager class itself, prefer using the internal static PluginModified(string id) method directly rather than going through API.PluginModified() for better performance and architectural design.
Applied to files:
Plugins/Flow.Launcher.Plugin.Url/Main.csFlow.Launcher.Test/Plugins/UrlPluginTest.cs
📚 Learning: 2025-07-06T12:21:37.947Z
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3572
File: Flow.Launcher/App.xaml.cs:214-216
Timestamp: 2025-07-06T12:21:37.947Z
Learning: In Flow Launcher, the UpdatePluginManifestAsync method in PluginsManifest.cs already has comprehensive internal try-catch handling that logs exceptions and returns false on failure rather than throwing, making external try-catch wrappers unnecessary.
Applied to files:
Plugins/Flow.Launcher.Plugin.Url/Main.cs
📚 Learning: 2025-09-13T12:50:05.306Z
Learnt from: dcog989
Repo: Flow-Launcher/Flow.Launcher PR: 3971
File: Plugins/Flow.Launcher.Plugin.Calculator/Main.cs:67-67
Timestamp: 2025-09-13T12:50:05.306Z
Learning: NumberRegex pattern `-?[\d\.,]+` in Flow.Launcher Calculator will match function arguments like `2,3` in `min(2,3)`. The NormalizeNumber validation that checks for proper 3-digit thousand separator grouping is essential to avoid breaking function calls by incorrectly stripping commas from function arguments.
Applied to files:
Plugins/Flow.Launcher.Plugin.Url/Main.cs
📚 Learning: 2025-10-16T10:48:30.573Z
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3854
File: Flow.Launcher/App.xaml.cs:252-275
Timestamp: 2025-10-16T10:48:30.573Z
Learning: In Flow Launcher's App.xaml.cs, the plugin initialization block (lines 252-275) that includes PreStartPluginExecutablePathUpdate, PluginManager.LoadPlugins, PluginManager.InitializePluginsAsync, _mainVM.QueryResults(), and API.SaveAppAllSettings() does not require additional Task.Run wrappers or Dispatcher.InvokeAsync calls according to maintainer Jack251970, as the threading model is already safe as designed.
Applied to files:
Plugins/Flow.Launcher.Plugin.Url/Main.cs
📚 Learning: 2025-07-21T09:19:49.684Z
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3854
File: Flow.Launcher/App.xaml.cs:246-262
Timestamp: 2025-07-21T09:19:49.684Z
Learning: In Flow Launcher's App.xaml.cs, the asynchronous plugin initialization task (containing AbstractPluginEnvironment.PreStartPluginExecutablePathUpdate, PluginManager.LoadPlugins, PluginManager.InitializePluginsAsync, and AutoPluginUpdates) does not require additional try-catch error handling according to maintainer Jack251970, as these operations are designed to handle exceptions internally.
Applied to files:
Plugins/Flow.Launcher.Plugin.Url/Main.cs
📚 Learning: 2025-07-21T09:19:19.012Z
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3854
File: Flow.Launcher.Core/Plugin/PluginManager.cs:280-292
Timestamp: 2025-07-21T09:19:19.012Z
Learning: In Flow Launcher's PluginManager.cs, the post-initialization operations (RegisterResultsUpdatedEvent, UpdatePluginMetadataTranslation, RegisterPluginActionKeywords, DialogJump.InitializeDialogJumpPlugin, and AddPluginToLists) are designed to be exception-safe and do not require additional try-catch error handling according to the maintainer Jack251970.
Applied to files:
Plugins/Flow.Launcher.Plugin.Url/Main.cs
📚 Learning: 2025-09-05T11:56:27.267Z
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 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:
Plugins/Flow.Launcher.Plugin.Url/Main.cs
🧬 Code graph analysis (2)
Plugins/Flow.Launcher.Plugin.Url/Main.cs (1)
Plugins/Flow.Launcher.Plugin.Url/Settings.cs (1)
Settings(3-52)
Flow.Launcher.Test/Plugins/UrlPluginTest.cs (1)
Plugins/Flow.Launcher.Plugin.Url/Main.cs (2)
Main(10-190)IsURL(103-167)
🪛 GitHub Check: Check Spelling
Flow.Launcher.Test/Plugins/UrlPluginTest.cs
[warning] 32-32:
google is not a recognized word. (unrecognized-spelling)
[warning] 31-31:
google is not a recognized word. (unrecognized-spelling)
[warning] 4-4:
NUnit is not a recognized word. (unrecognized-spelling)
[warning] 32-32:
google is not a recognized word. (unrecognized-spelling)
⏰ 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 (9)
Plugins/Flow.Launcher.Plugin.Url/Main.cs (5)
12-37: Pre-filter regex approach is sound.The simplified regex pattern combined with
Uri.TryCreatevalidation provides a good balance between quick rejection of obviously invalid inputs and comprehensive validation. The loose patterns (e.g., for IPv6 and port ranges) are acceptable since the authoritative validation happens viaUri.TryCreate.
43-43: LGTM - centralized scheme management.The
UrlSchemesarray provides good centralization for scheme recognition across the validation and normalization logic.
48-51: LGTM - clean early-exit pattern.The early return when
!IsURL(raw)improves readability and avoids unnecessary result construction.
105-112: LGTM - solid input validation.The null/whitespace check, input trimming, and regex pre-filtering provide a good defensive validation flow.
165-166: LGTM - robust final validation.Using
Uri.TryCreatewithUriKind.Absoluteand explicit scheme validation provides comprehensive URL validation. This correctly handles all URL edge cases including IPv6, internationalized domains, and port validation.Flow.Launcher.Test/Plugins/UrlPluginTest.cs (4)
11-14: LGTM - appropriate test settings initialization.The
_settingsfield withAlwaysOpenWithHttps = falseprovides a sensible default for testing both http and https scheme normalization.
27-66: Excellent test coverage for valid URLs.The parameterized test cases provide comprehensive coverage including:
- Various schemes (http/https/ftp)
- Bare and fully-qualified domains
- Localhost variations
- IPv4 addresses with optional auth and ports
- IPv6 addresses (both bracketed and bare)
- Case-insensitive inputs
The bare IPv6 test cases (lines 54-56) correctly verify that
IsURLaccepts them, but note that the critical issue in Main.cs lines 63-67 means these URLs won't open correctly in browsers.
74-74: Test aligns with 0.0.0.0 exclusion in implementation.This test case confirms that
0.0.0.0is intentionally rejected. However, as noted in the review of Main.cs (line 126-127), this exclusion may be inconsistent with the PR objective to support development/private IP addresses. If0.0.0.0should be allowed, update both the implementation and this test case.
68-73: LGTM - comprehensive invalid URL test coverage.The invalid test cases appropriately cover:
- Incomplete domains without TLDs
- Plain text that shouldn't match
- Malformed URLs with missing parts
- Invalid IPv4 octets (> 255)
- Invalid domain patterns
These tests ensure the validation doesn't produce false positives.
Also applies to: 75-82
| // not a recognized scheme, add preferred http scheme | ||
| if (!UrlSchemes.Any(scheme => raw.StartsWith(scheme, StringComparison.OrdinalIgnoreCase))) | ||
| { | ||
| raw = GetHttpPreference() + "://" + raw; | ||
| } |
There was a problem hiding this comment.
Critical: Bare IPv6 addresses won't open correctly.
The Action handler adds the scheme to bare IPv6 addresses without wrapping them in brackets. For input "2001:db8::1", this produces "http://2001:db8::1" instead of the required "http://[2001:db8::1]". Browsers will reject these URLs.
The IsURL method correctly handles bracket-wrapping for validation (lines 137-157), but the Action handler needs the same logic.
🔎 Proposed fix
{
// not a recognized scheme, add preferred http scheme
if (!UrlSchemes.Any(scheme => raw.StartsWith(scheme, StringComparison.OrdinalIgnoreCase)))
{
- raw = GetHttpPreference() + "://" + raw;
+ // Wrap bare IPv6 addresses in brackets
+ if (raw.Count(c => c == ':') > 1 && !raw.Contains("://") && !raw.Contains('@') && !raw.StartsWith('['))
+ {
+ // Check if there's already a port pattern (]:port)
+ var portMatch = Regex.Match(raw, @"\]:(\d{1,5})$");
+ if (!portMatch.Success)
+ {
+ raw = $"{GetHttpPreference()}://[{raw}]";
+ }
+ else
+ {
+ raw = GetHttpPreference() + "://" + raw;
+ }
+ }
+ else
+ {
+ raw = GetHttpPreference() + "://" + raw;
+ }
}Alternatively, extract the bracket-wrapping logic into a helper method to avoid duplication with IsURL.
🤖 Prompt for AI Agents
In Plugins/Flow.Launcher.Plugin.Url/Main.cs around lines 63 to 67, the code
prepends the HTTP scheme to strings but does not bracket-wrap bare IPv6
addresses; change the logic so that when adding the scheme you detect a bare
IPv6 (contains ':' and does not start with '[' and is not already a recognized
scheme) and wrap it with '[' and ']' before prefixing the scheme (or extract the
existing bracket-wrapping logic from IsURL into a shared helper and call that
here) so resulting URLs become "http://[2001:db8::1]" instead of
"http://2001:db8::1".
| if (ipv4Match.Groups[1].Value == "0.0.0.0") | ||
| return false; |
There was a problem hiding this comment.
Reconsider excluding 0.0.0.0.
The exclusion of 0.0.0.0 appears inconsistent with the PR objective to support private and development IP addresses. 0.0.0.0 is commonly used in development environments as a bind address (servers listening on all interfaces). Unless there's a specific security or usability reason to exclude it, consider allowing it to align with the PR goal.
🔎 Proposed fix if 0.0.0.0 should be allowed
if (!byte.TryParse(octet, out _))
return false;
}
-
- if (ipv4Match.Groups[1].Value == "0.0.0.0")
- return false;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (ipv4Match.Groups[1].Value == "0.0.0.0") | |
| return false; | |
| if (!byte.TryParse(octet, out _)) | |
| return false; | |
| } | |
| } |
🤖 Prompt for AI Agents
In Plugins/Flow.Launcher.Plugin.Url/Main.cs around lines 126 to 127, the code
explicitly rejects the IPv4 address "0.0.0.0" which conflicts with the PR goal
to support private/development addresses; remove (or comment out) the
conditional that returns false for ipv4Match.Groups[1].Value == "0.0.0.0" so
that addresses bound to all interfaces are treated as valid, and add a brief
comment explaining that 0.0.0.0 is allowed for development/bind scenarios (or
keep the check only if there is a documented, specific security reason).
Pull request was closed
Update the regex that determines if a string is URL in the URL plugin, so private addresses are no longer filtered. Fixes #4184.