Fix intra-link warning when using query parameters #2789
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2789 +/- ##
==========================================
- Coverage 71.67% 71.66% -0.01%
==========================================
Files 134 134
Lines 7286 7285 -1
Branches 1609 1490 -119
==========================================
- Hits 5222 5221 -1
- Misses 1937 2018 +81
+ Partials 127 46 -81 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This pull request fixes an invalid intra-link warning that was incorrectly triggered when links contained URL query parameters. The issue occurred because the validation logic was treating the entire URL (including query parameters) as the path to validate, rather than just the pathname portion.
Changes:
- Modified
validateIntraLinkfunction to strip query parameters from URLs before validation - Added comprehensive test coverage for links with query parameters in various scenarios
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/core/src/html/linkProcessor.ts | Added logic to remove query parameters from resource paths before validation, preventing false positive warnings |
| packages/core/test/unit/html/linkProcessor.test.ts | Added 6 new test cases covering query parameters with .html extensions, no extensions, hash fragments, and combinations thereof |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } else if (resourcePathUrl.query) { | ||
| // remove query parameters if present | ||
| resourcePath = resourcePathUrl.pathname; // eslint-disable-line no-param-reassign |
There was a problem hiding this comment.
The use of else if here creates confusion because both conditions handle query parameters differently. When a hash is present, the query parameter is implicitly removed by pathname (line 186). When only a query parameter is present (no hash), it's explicitly handled here. This logic is correct but unnecessarily complex. Consider combining these conditions into a single check that always uses pathname when either hash or query parameters are present, making the intent clearer.
gerteck
left a comment
gerteck
left a comment
There was a problem hiding this comment.
Thank you for your help on this 👍
| hash = resourcePathUrl.hash.substring(1); | ||
| // remove hash portion (if any) in the resourcePath | ||
| resourcePath = resourcePathUrl.pathname; // eslint-disable-line no-param-reassign | ||
| } else if (resourcePathUrl.query) { |
There was a problem hiding this comment.
- hashes (#) and queries (?) can both exist in the same URL, with the query string usually coming before the hash, e.g.
https://example.com/page?param=value#section
so using else-if is probably not the best idea, since it implies mutual exclusion
Since url-parse library already separates pathname, query, hash, we can just set the resourcePath to pathname.
const hash = resourcePathUrl.hash ? resourcePathUrl.hash.substring(1) : undefined;
resourcePath = resourcePathUrl.pathname;
There was a problem hiding this comment.
Ah yea, makes sense. Incorporated changes as per your suggestion!
2 participants
What is the purpose of this pull request?
Resolves #2768
Overview of changes:
Prevents the invalid intra-site warning by preventing query parameters from being validated. Adds linkProcessor tests for this case.
Anything you'd like to highlight/discuss:
Testing instructions:
Proposed commit message: (wrap lines at 72 characters)
Fix invalid intra-link warning when using query parameters
Checklist: ☑️
Reviewer checklist:
Indicate the SEMVER impact of the PR:
At the end of the review, please label the PR with the appropriate label:
r.Major,r.Minor,r.Patch.Breaking change release note preparation (if applicable):