chore(ui): Handle encoded and unencoded params in horizontal nav#16074
chore(ui): Handle encoded and unencoded params in horizontal nav#16074
Conversation
|
This change is part of the following stack: Change managed by git-spice. |
|
Skipping CI for Draft Pull Request. |
|
Caution There are some errors in your PipelineRun template.
|
|
Images are ready for the commit at 01cc05a. To use with deploy scripts, first |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #16074 +/- ##
==========================================
- Coverage 48.68% 48.68% -0.01%
==========================================
Files 2603 2604 +1
Lines 191590 191663 +73
==========================================
+ Hits 93285 93302 +17
- Misses 90976 91028 +52
- Partials 7329 7333 +4
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:
|
47beff7 to
9311b05
Compare
There was a problem hiding this comment.
Hey @dvail - I've reviewed your changes - here's some feedback:
- Instead of regex on the search string, consider using URLSearchParams to parse and compare decoded parameter values for more robust handling.
- Add unit tests for the new containsPatternIgnoringEncoding util to cover edge cases like special characters or multiple parameters.
- Escape the pattern or avoid interpolating arbitrary strings into the RegExp to prevent unintended regex behavior when pattern contains special characters.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Instead of regex on the search string, consider using URLSearchParams to parse and compare decoded parameter values for more robust handling.
- Add unit tests for the new containsPatternIgnoringEncoding util to cover edge cases like special characters or multiple parameters.
- Escape the pattern or avoid interpolating arbitrary strings into the RegExp to prevent unintended regex behavior when pattern contains special characters.
## Individual Comments
### Comment 1
<location> `ui/apps/platform/src/Containers/MainPage/Navigation/HorizontalSubnav.tsx:35` </location>
<code_context>
import './HorizontalSubnav.css';
+// Check if the search string contains the pattern or the encoded version of the pattern
+function containsPatternIgnoringEncoding(search: string, pattern: string) {
+ return new RegExp(`${pattern}|${encodeURI(pattern)}`, 'i').test(search);
+}
</code_context>
<issue_to_address>
The regex in containsPatternIgnoringEncoding may match unintended substrings.
Matching with the current regex may cause false positives if the pattern appears within another parameter or value. Parsing the query string and comparing parameter values directly would provide more accurate results.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
ui/apps/platform/src/Containers/MainPage/Navigation/HorizontalSubnav.tsx
Outdated
Show resolved
Hide resolved
e976e9c to
01cc05a
Compare
|
@dvail: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
Description
In the react-router v5->v6 change the handling of active states in the Violations page navigation was broken and caught during a bug bash. Since this will be an issue again when moving to the rr-compat library, I've made these checks independent of parameter encoding and added tests to prevent breaking again during the next change.
User-facing documentation
Testing and quality
Automated testing
How I validated my change
npm run testnpm run test-component