fix(spans-migration): add selection params to explore url in frontend #101185
Conversation
github-actions
bot
added
the
Scope: Frontend
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| }); | ||
|
|
||
| return null; | ||
| return params.length > 0 ? `${baseUrl}&${params.join('&')}` : baseUrl; |
There was a problem hiding this comment.
Bug: URL Construction Fails Without Existing Query
The createExploreUrl function constructs invalid URLs because it always appends query parameters with &. When the baseUrl doesn't already have a query string, this results in an improperly formatted URL.
![seer-by-sentry[bot]](https://avatars.githubusercontent.com/in/801464?s=60&v=4){kind=small}
| params.push('utc=true'); | ||
| } | ||
|
|
||
| selection.projects.forEach(project => { | ||
| params.push(`project=${project}`); | ||
| }); | ||
|
|
||
| selection.environments.forEach(environment => { | ||
| params.push(`environment=${environment}`); | ||
| }); | ||
|
|
There was a problem hiding this comment.
Potential bug: URL parameters are not encoded, leading to broken links for environment names with special characters.
-
Description: The
createExploreUrlfunction constructs URL parameters without encoding them. Since environment names can contain special characters like '&', '=', or spaces, concatenating them directly into the URL string will result in malformed and broken URLs. This impacts the functionality of the 'Explore' links for users with such environment names. -
Suggested fix: Use
encodeURIComponenton the values of the parameters before adding them to the URL string. For example, changeparams.push(environment=${environment});toparams.push(environment=${encodeURIComponent(environment)});.
severity: 3.0, confidence: 5.0
Did we get this right? 👍 / 👎 to inform future reviews.
| if (selection.datetime?.start) { | ||
| params.push(`start=${getUtcDateString(selection.datetime.start)}`); | ||
| } | ||
| if (selection.datetime?.end) { | ||
| params.push(`end=${getUtcDateString(selection.datetime.end)}`); | ||
| } | ||
| if (selection.datetime?.period) { | ||
| params.push(`statsPeriod=${selection.datetime.period}`); | ||
| } | ||
| if (selection.datetime?.utc) { | ||
| params.push('utc=true'); | ||
| } | ||
|
|
||
| selection.projects.forEach(project => { | ||
| params.push(`project=${project}`); | ||
| }); | ||
|
|
||
| selection.environments.forEach(environment => { | ||
| params.push(`environment=${environment}`); | ||
| }); |
There was a problem hiding this comment.
this feels a little janky to me, could we instead parse this URL, something like
const parsedURL = safeURL(baseUrl);
const queryParams = qs.parse(parsedURL?.search ?? '');
// append all the page filters to queryParams - perhaps use an existing util like pageFiltersToQueryParams
return getExploreUrl(queryParams)
add the page params to the transaction widget warning urls on the frontend so they can see the same thing if they haven't submitted their dashboard filters. NOTE: i noticed that now the entire dashboard reloads when changing dashboard filters because the urls have to be re-rendered. This kinda sucks but it will be temporary