Update Url Processing #970
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds a new keep configuration option to preserve specific URL query parameters from being truncated during URL scrubbing. The feature reorders query strings to move "kept" parameters to the front, ensuring they remain within the maxUrlLength limit when truncation occurs.
Key changes:
- Added
keepparameter to the Config interface alongside existingdropparameter - Implemented logic to reorder query parameters, placing kept parameters first
- Initialized default
keepconfiguration as an empty array
Reviewed Changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/clarity-js/types/core.d.ts | Added keep?: string[] to Config interface |
| packages/clarity-js/src/core/config.ts | Initialized default keep configuration as empty array |
| packages/clarity-js/src/core/scrub.ts | Implemented query parameter reordering logic to prioritize kept parameters before truncation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (kept.length > 0) { | ||
| result = path + "?" + kept.concat(others).join("&"); | ||
| } |
There was a problem hiding this comment.
[nitpick] The condition if (kept.length > 0) means that if no kept parameters are found, the result remains unchanged even though the keep configuration was specified. This could lead to confusion. Consider removing this check and always reconstructing the query string when keep is configured, or document this behavior explicitly.
| if (kept.length > 0) { | |
| result = path + "?" + kept.concat(others).join("&"); | |
| } | |
| // Always reconstruct the query string when keep is specified, even if no kept parameters are found | |
| const newQuery = kept.concat(others).join("&"); | |
| result = newQuery.length > 0 ? path + "?" + newQuery : path; |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 2 out of 3 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| }); | ||
| if (kept.length > 0) { | ||
| result = path + "?" + kept.concat(others).join("&"); |
There was a problem hiding this comment.
wont this mess up the param following keep?
Niyibitanga
force-pushed
the
ninosa/UrlUpdate
branch
from
4719674 to
be8a4d7
Compare
| } | ||
|
|
||
| if (truncate) { | ||
| result = result.substring(0, maxUrlLength); |
There was a problem hiding this comment.
Should keep override truncate?
| let keep = config.keep; | ||
| if (drop && drop.length > 0 && input && input.indexOf("?") > 0) { | ||
| let [path, query] = input.split("?"); | ||
| let swap = Data.Constant.Dropped; |
There was a problem hiding this comment.
Might be simpler and less code to merge the drop of keep logic into a single process of args.
Update url processing to keep certain parameters