Conversation
|
🥷 Code experts: jjw24, Jack251970 jjw24, 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: ✨ Comment |
|
Be a legend 🏆 by adding a before and after screenshot of the changes you made, especially if they are around UI/UX. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughReplaces legacy URL tests with data-driven NUnit tests (reflection-based setup) and rewrites plugin URL detection: removes regex, uses scheme list plus Uri/host/IP/TLD validation, accepts IPv4/IPv6/localhost, prepends a default scheme only when needed, and returns no results for non-URLs. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 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 |
![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 (3)
Flow.Launcher.Test/Plugins/UrlPluginTest.cs (2)
10-19: Reflection-based setup is acceptable but consider null check.The reflection approach to set the private
Settingsfield is reasonable for testing. However,settingsFieldcould theoretically be null if the field is renamed or refactored.🔎 Consider adding a guard or assertion
[OneTimeSetUp] public void OneTimeSetup() { var settingsField = typeof(Main).GetField("Settings", BindingFlags.NonPublic | BindingFlags.Static); + Assert.That(settingsField, Is.Not.Null, "Settings field not found - check if field name changed"); settingsField?.SetValue(null, new Settings()); plugin = new Main(); }
57-71: Invalid URL test cases look reasonable; consider adding a few edge cases.Current coverage is good. You may want to add tests for these edge cases if they're relevant:
"http://256.256.256.256"(invalid IPv4 with scheme)"[::1"(unclosed bracket)"http://[::1"(unclosed bracket with scheme)These would help ensure the regex properly rejects malformed inputs.
Plugins/Flow.Launcher.Plugin.Url/Main.cs (1)
118-125:IsURLmethod could be simplified.The method works correctly but has a minor redundancy.
🔎 Simplify the return statement
public bool IsURL(string raw) { raw = raw.ToLower(); - - if (UrlRegex.Match(raw).Value == raw) return true; - - return false; + return UrlRegex.Match(raw).Value == raw; }Additionally, since the regex uses
RegexOptions.IgnoreCase, theToLower()call may be redundant. Consider removing it for minor performance improvement:public bool IsURL(string raw) { - raw = raw.ToLower(); - return UrlRegex.Match(raw).Value == raw; + return UrlRegex.Match(raw).Value.Equals(raw, StringComparison.OrdinalIgnoreCase); }
📜 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 (8)
📚 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: 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-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.csFlow.Launcher.Test/Plugins/UrlPluginTest.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-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:
Flow.Launcher.Test/Plugins/UrlPluginTest.cs
🧬 Code graph analysis (1)
Flow.Launcher.Test/Plugins/UrlPluginTest.cs (1)
Plugins/Flow.Launcher.Plugin.Url/Main.cs (2)
Main(10-148)IsURL(118-125)
🪛 GitHub Check: Check Spelling
Flow.Launcher.Test/Plugins/UrlPluginTest.cs
[warning] 26-26:
google is not a recognized word. (unrecognized-spelling)
[warning] 25-25:
google is not a recognized word. (unrecognized-spelling)
[warning] 26-26:
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). (6)
- GitHub Check: Agent
- GitHub Check: CodeQL analysis (csharp)
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: build
🔇 Additional comments (6)
Flow.Launcher.Test/Plugins/UrlPluginTest.cs (1)
21-55: Good coverage of valid URL formats including IPv6 and case variations.The parameterized test cases comprehensively cover:
- Standard HTTP/HTTPS/FTP schemes
- Bare domains and hostnames
- localhost with and without ports
- IPv4 addresses including private ranges (192.168.x.x)
- IPv6 in both bracketed and unbracketed forms
- Case-insensitive matching
Plugins/Flow.Launcher.Plugin.Url/Main.cs (5)
19-34: IPv6 regex patterns are comprehensive but complex.The IPv6 patterns cover many common forms. The
(?!:[0-9])negative lookahead on line 34 correctly prevents matching unbracketed IPv6 followed by a port (since ports require brackets for IPv6).One observation: the patterns don't cover all valid IPv6 compressed forms (e.g.,
2001::db8:1with::in arbitrary positions), but this aligns with the PR description noting that some IPv6 edge cases are not handled.Consider adding a comment noting which IPv6 forms are intentionally unsupported for future maintainers:
+ // Note: Not all IPv6 compressed forms are supported (see issue #4185) // IPv6 address with optional brackets (brackets required if followed by port)
36-37: IPv4 pattern excludes 0.0.0.0 as intended.The pattern correctly requires the first octet to be
[1-9](not starting with 0), which excludes0.0.0.0. The remaining octets allow[1-9]?\dpermitting values 0-99, and the25[0-5]|2[0-4]\d|1\d\dcovers 100-255.However, I notice a potential issue: the pattern
[1-9]?\dfor octets 2-4 would match values like00or09(single digit with optional leading digit). This seems intentional but may produce unexpected matches.
58-58: Good extraction of URL schemes into a reusable array.Using a static readonly array for schemes improves maintainability and ensures consistency between the regex pattern and the scheme check in
Query.
63-66: Early return for non-URLs is a good refactor.Returning an empty list early when the input is not a URL simplifies the control flow and makes the intent clear. This is a cleaner approach than the previous structure.
78-82: Scheme check using LINQ is correct.The
Anycheck withStringComparison.OrdinalIgnoreCaseproperly handles case-insensitive scheme matching, consistent with the test cases like"HTTP://EXAMPLE.COM".
There was a problem hiding this comment.
Pull request overview
This PR fixes URL matching issues in the URL plugin by refactoring the regex pattern to support IPv6 addresses, removing exclusions for private IP addresses, and consolidating localhost handling into the main regex pattern.
Key Changes:
- Added comprehensive IPv6 support with both bracketed and unbracketed formats
- Removed private IP address exclusions (10.x.x.x, 192.168.x.x, 172.16-31.x.x) to allow matching private network addresses
- Refactored tests from a single test method with assertions to parameterized test cases using TestCase attributes
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| Plugins/Flow.Launcher.Plugin.Url/Main.cs | Replaced IPv4-only address matching with comprehensive IPv6 support, removed private network exclusions, added UrlSchemes array, adjusted port pattern to allow single digits, and refactored Query method control flow |
| Flow.Launcher.Test/Plugins/UrlPluginTest.cs | Converted from ClassicAssert to modern NUnit Assert.That syntax, added OneTimeSetUp for test initialization, and expanded test coverage with parameterized test cases for IPv6 addresses and edge cases |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
Flow.Launcher.Test/Plugins/UrlPluginTest.cs (2)
12-19: Reflection-based setup is pragmatic but brittle — will silently break if the property is renamed or made non-static.
settingsProperty?.SetValue(null, new Settings())— the null-conditional means if the reflection lookup fails (e.g., the property is renamed or its visibility changes),pluginwill be constructed withSettings == null, and every test will either NPE or pass vacuously. Consider adding a guard assertion:Proposed fix
var settingsProperty = typeof(Main).GetProperty("Settings", BindingFlags.NonPublic | BindingFlags.Static); + Assert.That(settingsProperty, Is.Not.Null, "Could not find Main.Settings property via reflection"); settingsProperty?.SetValue(null, new Settings());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Flow.Launcher.Test/Plugins/UrlPluginTest.cs` around lines 12 - 19, The reflection lookup for Main.Settings in OneTimeSetup can silently fail due to renames/visibility changes; update OneTimeSetup to validate that settingsProperty is non-null before calling SetValue and constructing plugin by asserting or throwing with a clear message (e.g., Assert.NotNull or Assert.Fail referencing "Main.Settings") so test authors immediately see the reflection lookup failure; keep using typeof(Main).GetProperty(...) and SettingsProperty.SetValue(null, new Settings()) but add the guard/assert that references the Main class, Settings property, and OneTimeSetup so failures are explicit.
21-55: Consider adding test cases for key edge-case paths inIsURL.The current coverage is solid, but a few inputs would exercise important branches that aren't currently tested:
Input Expected Branch exercised "1.2345"falsedecimal.TryParseguard"1234"falsePlain integer "192.168.1.1/admin"trueBare IP with path "google.com?q=test"trueDomain with query string "0.0.0.0:8080"falseIPAddress.Anywith portAlso applies to: 57-71
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Flow.Launcher.Test/Plugins/UrlPluginTest.cs` around lines 21 - 55, Add unit cases that exercise IsURL's edge branches by extending the test data for the test method WhenValidUrlThenIsUrlReturnsTrue (and the companion invalid-url test around lines 57-71): add inputs "1.2345" (expect false), "1234" (false), "192.168.1.1/admin" (true), "google.com?q=test" (true), and "0.0.0.0:8080" (false) and assert plugin.IsURL(url) matches the expected boolean to cover the decimal.TryParse guard, integer-only input, bare IPs with paths, domains with query strings, and IPAddress.Any with ports branches.Plugins/Flow.Launcher.Plugin.Url/Main.cs (1)
35-38: Duplicated scheme-detection logic betweenQueryaction andIsURL.The
UrlSchemes.Any(scheme => raw.StartsWith(scheme, ...))pattern appears both here (line 36) and inIsURL(line 92). Consider extracting a small helper (e.g.,HasKnownScheme(string input)) to keep them in sync and avoid subtle divergence.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Plugins/Flow.Launcher.Plugin.Url/Main.cs` around lines 35 - 38, Extract the duplicated scheme-detection into a single helper method (e.g., bool HasKnownScheme(string input)) that checks UrlSchemes.Any(scheme => input.StartsWith(scheme, StringComparison.OrdinalIgnoreCase)), then replace the inline checks in the Query flow (where raw is prefixed with GetHttpPreference()) and in IsURL with calls to HasKnownScheme; ensure the helper is placed in the same class as Query and IsURL and uses the existing UrlSchemes collection and OrdinalIgnoreCase comparison so both callers remain consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Plugins/Flow.Launcher.Plugin.Url/Main.cs`:
- Around line 75-89: In IsURL, the decimal parsing guard uses
decimal.TryParse(input, out _) which is culture-sensitive; update the call to
use an invariant culture (e.g., decimal.TryParse with
CultureInfo.InvariantCulture and appropriate NumberStyles) so inputs like
"1.2345" are parsed the same regardless of locale; modify the decimal.TryParse
call in the IsURL method to include NumberStyles.Number and
CultureInfo.InvariantCulture (retain the out discard) to ensure consistent
numeric detection before the IPEndPoint.TryParse check.
- Around line 86-89: The code currently treats an endpoint as invalid only when
endpoint.Address equals IPAddress.Any (0.0.0.0); update the validation in the
IPEndPoint.TryParse branches (the logic using ipPart, IPEndPoint.TryParse and
endpoint.Address) to also exclude IPAddress.IPv6Any (::). In other words, change
the check that compares endpoint.Address to IPAddress.Any so it rejects both
IPAddress.Any and IPAddress.IPv6Any (e.g.,
endpoint.Address.Equals(IPAddress.Any) ||
endpoint.Address.Equals(IPAddress.IPv6Any)), and apply the same modification to
the similar check later in the file (the other IPEndPoint.TryParse usage).
---
Nitpick comments:
In `@Flow.Launcher.Test/Plugins/UrlPluginTest.cs`:
- Around line 12-19: The reflection lookup for Main.Settings in OneTimeSetup can
silently fail due to renames/visibility changes; update OneTimeSetup to validate
that settingsProperty is non-null before calling SetValue and constructing
plugin by asserting or throwing with a clear message (e.g., Assert.NotNull or
Assert.Fail referencing "Main.Settings") so test authors immediately see the
reflection lookup failure; keep using typeof(Main).GetProperty(...) and
SettingsProperty.SetValue(null, new Settings()) but add the guard/assert that
references the Main class, Settings property, and OneTimeSetup so failures are
explicit.
- Around line 21-55: Add unit cases that exercise IsURL's edge branches by
extending the test data for the test method WhenValidUrlThenIsUrlReturnsTrue
(and the companion invalid-url test around lines 57-71): add inputs "1.2345"
(expect false), "1234" (false), "192.168.1.1/admin" (true), "google.com?q=test"
(true), and "0.0.0.0:8080" (false) and assert plugin.IsURL(url) matches the
expected boolean to cover the decimal.TryParse guard, integer-only input, bare
IPs with paths, domains with query strings, and IPAddress.Any with ports
branches.
In `@Plugins/Flow.Launcher.Plugin.Url/Main.cs`:
- Around line 35-38: Extract the duplicated scheme-detection into a single
helper method (e.g., bool HasKnownScheme(string input)) that checks
UrlSchemes.Any(scheme => input.StartsWith(scheme,
StringComparison.OrdinalIgnoreCase)), then replace the inline checks in the
Query flow (where raw is prefixed with GetHttpPreference()) and in IsURL with
calls to HasKnownScheme; ensure the helper is placed in the same class as Query
and IsURL and uses the existing UrlSchemes collection and OrdinalIgnoreCase
comparison so both callers remain consistent.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 13 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Check if it's a bare IP address with optional port and path | ||
| var ipPart = input.Split('/')[0]; // Remove path | ||
| if (IPEndPoint.TryParse(ipPart, out var endpoint) && !endpoint.Address.Equals(IPAddress.Any)) | ||
| return true; |
There was a problem hiding this comment.
The validation accepts bare IPv6 addresses without brackets (e.g., "2001:db8::1", "::1") which is problematic because when these are passed to the Action method, they will be prepended with "http://" or "https://" resulting in invalid URLs like "http://2001:db8::1". IPv6 addresses in URLs must be enclosed in brackets. The Action method at line 38 simply prepends the protocol without adding brackets, which will create malformed URLs that browsers cannot open.
| public void WhenValidUrlThenIsUrlReturnsTrue(string url) | ||
| { | ||
| Assert.That(plugin.IsURL(url), Is.True); | ||
| } |
There was a problem hiding this comment.
The test cases should include scenarios with query parameters and fragments to ensure the URL validation handles them correctly. For example, add test cases like "192.168.1.1?query=value", "localhost:8080?test=123", "example.com#fragment", etc. These are common URL patterns that should be supported.
| public void WhenValidUrlThenIsUrlReturnsTrue(string url) | ||
| { | ||
| Assert.That(plugin.IsURL(url), Is.True); | ||
| } |
There was a problem hiding this comment.
Missing test coverage for URLs with paths. The current test cases don't include scenarios like "example.com/path", "192.168.1.1/path/to/resource", or "localhost:8080/api/endpoint". These are common patterns that should be validated to ensure the path handling works correctly in both the validation logic and the Action method.
| [TestCase("HTTP://EXAMPLE.COM")] | ||
| [TestCase("HTTPS://EXAMPLE.COM")] | ||
| [TestCase("EXAMPLE.COM")] | ||
| [TestCase("LOCALHOST")] |
There was a problem hiding this comment.
The test cases include uppercase variants (HTTP://EXAMPLE.COM, HTTPS://EXAMPLE.COM, EXAMPLE.COM, LOCALHOST) which is good. However, it would be beneficial to also test mixed case scenarios and ensure that the validation and URL opening work correctly regardless of case. The code uses StringComparison.OrdinalIgnoreCase which should handle this, but explicit test coverage would be valuable.
| [TestCase("LOCALHOST")] | |
| [TestCase("LOCALHOST")] | |
| [TestCase("Http://Example.Com")] | |
| [TestCase("hTTps://ExAmPlE.CoM")] | |
| [TestCase("Example.Com")] | |
| [TestCase("LocalHost")] |
| public void WhenInvalidUrlThenIsUrlReturnsFalse(string url) | ||
| { | ||
| Assert.That(plugin.IsURL(url), Is.False); | ||
| } |
There was a problem hiding this comment.
The test cases for invalid URLs should include more edge cases to ensure robustness. Consider adding tests for: URLs with spaces (e.g., "example .com"), URLs with invalid characters, extremely long TLDs, domains starting with dots (e.g., "..example.com"), multiple consecutive dots (e.g., "example..com"), and malformed IPv6 addresses (e.g., "2001:db8:::1" with three colons).
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 2 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.Url/Main.cs">
<violation number="1" location="Plugins/Flow.Launcher.Plugin.Url/Main.cs:83">
P1: `decimal.TryParse` is culture-sensitive and uses `CultureInfo.CurrentCulture` by default. In locales where the decimal separator is a comma (e.g., `de-DE`), an input like `"1.2345"` won't parse as a decimal, will fall through to `IPEndPoint.TryParse`, and .NET will interpret it as a valid IP address (octet shorthand), causing a false positive. Use `CultureInfo.InvariantCulture` to ensure the guard works consistently across all locales.</violation>
<violation number="2" location="Plugins/Flow.Launcher.Plugin.Url/Main.cs:88">
P2: IPv6 "any" address (`::`) is not excluded, unlike its IPv4 counterpart `0.0.0.0`. Typing `::` would be incorrectly recognized as a valid URL. Add a check for `IPAddress.IPv6Any` for consistency.</violation>
<violation number="3" location="Plugins/Flow.Launcher.Plugin.Url/Main.cs:111">
P2: Same issue as the `IPEndPoint` check above: the IPv6 "any" address (`::`) is not excluded here either. `IPAddress.IPv6Any` should also be rejected to maintain symmetry with the `0.0.0.0` exclusion.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
coderabbitai
bot
removed
bug
prlabeler
bot
added
bug
|
@VictoriousRaptor Is it good to go? I know almost nothing about IP, so it is hard for me to review it by myself |
I think it's good to go but let's see if @jjw24 has any comment. |
Some cultures uses comma as decimal separator which escapes this decimal check. Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>
coderabbitai
bot
removed
bug
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
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.Url/Main.cs">
<violation number="1" location="Plugins/Flow.Launcher.Plugin.Url/Main.cs:83">
P2: Using InvariantCulture here ignores locale-specific decimal separators, so numeric inputs in comma-based cultures can slip through as URLs. Use the current culture to keep the numeric exclusion consistent with user input.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| ) | ||
| { | ||
| // Exclude numbers (e.g. 1.2345) | ||
| if (decimal.TryParse(input, System.Globalization.NumberStyles.Any, System.Globalization.CultureInfo.InvariantCulture, out _)) |
There was a problem hiding this comment.
P2: Using InvariantCulture here ignores locale-specific decimal separators, so numeric inputs in comma-based cultures can slip through as URLs. Use the current culture to keep the numeric exclusion consistent with user input.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Plugins/Flow.Launcher.Plugin.Url/Main.cs, line 83:
<comment>Using InvariantCulture here ignores locale-specific decimal separators, so numeric inputs in comma-based cultures can slip through as URLs. Use the current culture to keep the numeric exclusion consistent with user input.</comment>
<file context>
@@ -80,7 +80,7 @@ public bool IsURL(string raw)
// Exclude numbers (e.g. 1.2345)
- if (decimal.TryParse(input, out _))
+ if (decimal.TryParse(input, System.Globalization.NumberStyles.Any, System.Globalization.CultureInfo.InvariantCulture, out _))
return false;
</file context>
Fixes #4184. A rework of #4185 as it's very tricky to solve all IPv6 related problems so I just leave it.
What's done:
(though some edge cases in comment of Update UrlPattern in URL plugin to include private addresses and update test cases #4185 are not handled).