Conversation
# Conflicts: # src/BootstrapBlazor/BootstrapBlazor.csproj
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdds support for pasting full URLs or strings containing an IPv4 address into the IpAddress component by extracting a valid IPv4 substring with regex and populating the IPv4 cells from that match, while simplifying parsing and removing value clamping. Sequence diagram for handling pasted URL in IpAddress componentsequenceDiagram
actor User
participant Browser
participant IpAddressInput as IpAddressInput
participant IpAddressJs as IpAddressRazorJs
User->>Browser: Paste string containing URL or IPv4
Browser->>IpAddressInput: paste event with raw value
IpAddressInput->>IpAddressJs: handlePaste(raw)
IpAddressJs->>IpAddressJs: match = raw.match(ipRegex)
IpAddressJs->>IpAddressJs: parts = match ? match[0] : null
alt No IPv4 match
IpAddressJs-->>IpAddressInput: return (ignore paste)
else IPv4 match found
IpAddressJs->>IpAddressJs: octets = parts.split('.')
loop For each octet p (max 4)
IpAddressJs->>IpAddressJs: num = parseInt(p, 10)
IpAddressJs->>IpAddressInput: set cell value to num
IpAddressJs->>IpAddressJs: update prevValues[pos]
end
IpAddressJs-->>IpAddressInput: paste handling complete
end
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Consider keeping the clamping logic (
Math.max(0, Math.min(255, ...))) when parsing octets so the component is more defensive against any unexpected inputs or future changes to the regex. - The
partsvariable now changes type fromstringto being treated like an array (parts.split('.')); renaming it (e.g.,ipString) or splitting immediately after the match would make the code’s intent and data flow clearer.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider keeping the clamping logic (`Math.max(0, Math.min(255, ...))`) when parsing octets so the component is more defensive against any unexpected inputs or future changes to the regex.
- The `parts` variable now changes type from `string` to being treated like an array (`parts.split('.')`); renaming it (e.g., `ipString`) or splitting immediately after the match would make the code’s intent and data flow clearer.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Pull request overview
This PR enhances the IP address component to support pasting URLs that include port numbers. Previously, the paste handler used simple string manipulation to extract digits and dots, which would fail when pasting values like "192.168.1.1:8080". The new implementation uses a regex pattern to extract valid IPv4 addresses from the pasted content.
Key Changes:
- Replaced string manipulation (
replace(/[^\d.]/g, '')) with regex pattern matching to extract valid IPv4 addresses - Changed from filtering parts array to extracting the first matched IP address from the pasted text
- Removed defensive validation (Math.max/Math.min bounds checking) from the parseInt operation
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return; | ||
| } | ||
| const num = Math.max(0, Math.min(255, parseInt(p, 10) || 0)); | ||
| const num = parseInt(p, 10); |
There was a problem hiding this comment.
Removing the Math.max and Math.min validation is unsafe. While the regex validates that matched IPs have octets in the 0-255 range, the parseInt on line 113 should still include fallback validation. The original code protected against any unexpected parsing issues by ensuring values are always bounded. Without the '|| 0' fallback, parseInt could theoretically return NaN in edge cases, and without the Math.min(255, ...) check, there's no defensive programming against future code changes. This defensive validation should be retained.
| const num = parseInt(p, 10); | |
| const parsed = parseInt(p, 10); | |
| const num = Math.max(0, Math.min(255, isNaN(parsed) ? 0 : parsed)); |
| } | ||
| const parts = raw.replace(/[^\d.]/g, '').split('.').filter(p => p.length); | ||
|
|
||
| const ipRegex = /\b((25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\.){3}(25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\b/; |
There was a problem hiding this comment.
The IP address regex pattern is complex and should be extracted to a constant at the module level with a descriptive name like IP_ADDRESS_PATTERN. This improves code maintainability by making the pattern reusable and easier to test or modify. The regex could also use a comment explaining its purpose of extracting valid IPv4 addresses from text that may contain additional content like ports.
| const ipRegex = /\b((25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\.){3}(25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\b/; | ||
| const match = raw.match(ipRegex); | ||
| const parts = match ? match[0] : null; | ||
| if (parts === null) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
The new paste behavior that extracts IP addresses from URLs with ports lacks test coverage. Tests should be added to verify that pasting values like "192.168.1.1:8080" correctly extracts "192.168.1.1", and that edge cases like malformed URLs, URLs without IPs, or multiple IPs are handled appropriately.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7377 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 748 748
Lines 32766 32766
Branches 4545 4545
=========================================
Hits 32766 32766
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Link issues
fixes #7376
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
New Features: