Fixed issue with null params sent with =#4440
Merged
rohitneharabrowserstack merged 4 commits intomasterfrom Mar 6, 2026
Merged
Fixed issue with null params sent with =#4440rohitneharabrowserstack merged 4 commits intomasterfrom
rohitneharabrowserstack merged 4 commits intomasterfrom
Conversation
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThe diff changes query-parameter handling for HTTP requests. app/src/features/apiClient/helpers/httpRequestExecutor/httpRequestExecutor.ts now clones the request into a preparedEntryRequest, serializes its queryParams into the request URL via queryParamsToURLString (with encoding enabled) and clears the queryParams before calling makeRequest. app/src/features/apiClient/screens/apiClient/utils.ts updates queryParamsToURLString to accept an optional shouldEncode boolean (default false), treats null like no-value parameters, and encodes keys and values when shouldEncode is true. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/features/apiClient/screens/apiClient/utils.ts (1)
618-635:⚠️ Potential issue | 🟠 MajorPreserve existing URL query when
queryParamsis empty.This helper currently drops any query already present in
inputStringbecause it always rebuilds fromqueryParams. With the new executor usage, that can silently remove user-specified URL params.Proposed fix
export const queryParamsToURLString = (queryParams: KeyValuePair[], inputString: string, shouldEncode = false) => { + if (queryParams.length === 0) { + return inputString; + } const baseUrl = split(inputString, "?")[0]; const enabledParams = queryParams.filter((param) => param.isEnabled ?? true);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/features/apiClient/screens/apiClient/utils.ts` around lines 618 - 635, The helper queryParamsToURLString currently rebuilds the URL and drops any existing query when no params are provided; to fix this, in queryParamsToURLString (use symbols baseUrl, inputString, enabledParams) check if enabledParams.length === 0 (or there are no truthy keys to add) and if so simply return inputString as-is to preserve its existing query string; otherwise proceed with the current mapping/encoding logic to build and return `${baseUrl}?${queryString}` when queryString exists.
🧹 Nitpick comments (1)
app/src/features/apiClient/helpers/httpRequestExecutor/httpRequestExecutor.ts (1)
291-308: Log the same request object that is actually executed.After introducing
preparedEntryRequest, telemetry can diverge from real network behavior because logging still usespreparedEntry.request. For debugging/replay fidelity, logpreparedEntryRequest.Proposed refactor
- APIEventLogger.logRequest({ - request: preparedEntry.request, - workspaceId: this.ctx.workspaceId, - tag: { - recordId, - iteration: iterationContext.iteration, - executionId: execId, - }, - }); - const preparedEntryRequest = { ...preparedEntry.request, url: queryParamsToURLString(preparedEntry.request.queryParams, preparedEntry.request.url, true), queryParams: [], }; + APIEventLogger.logRequest({ + request: preparedEntryRequest, + workspaceId: this.ctx.workspaceId, + tag: { + recordId, + iteration: iterationContext.iteration, + executionId: execId, + }, + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/features/apiClient/helpers/httpRequestExecutor/httpRequestExecutor.ts` around lines 291 - 308, The telemetry currently logs preparedEntry.request but the code actually executes preparedEntryRequest; compute/build preparedEntryRequest (the object with url from queryParamsToURLString and queryParams: []) before calling APIEventLogger.logRequest and pass preparedEntryRequest to APIEventLogger.logRequest instead of preparedEntry.request so the logged request matches what is passed to makeRequest (refer to preparedEntryRequest, preparedEntry.request, APIEventLogger.logRequest, and makeRequest).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@app/src/features/apiClient/screens/apiClient/utils.ts`:
- Around line 618-635: The helper queryParamsToURLString currently rebuilds the
URL and drops any existing query when no params are provided; to fix this, in
queryParamsToURLString (use symbols baseUrl, inputString, enabledParams) check
if enabledParams.length === 0 (or there are no truthy keys to add) and if so
simply return inputString as-is to preserve its existing query string; otherwise
proceed with the current mapping/encoding logic to build and return
`${baseUrl}?${queryString}` when queryString exists.
---
Nitpick comments:
In
`@app/src/features/apiClient/helpers/httpRequestExecutor/httpRequestExecutor.ts`:
- Around line 291-308: The telemetry currently logs preparedEntry.request but
the code actually executes preparedEntryRequest; compute/build
preparedEntryRequest (the object with url from queryParamsToURLString and
queryParams: []) before calling APIEventLogger.logRequest and pass
preparedEntryRequest to APIEventLogger.logRequest instead of
preparedEntry.request so the logged request matches what is passed to
makeRequest (refer to preparedEntryRequest, preparedEntry.request,
APIEventLogger.logRequest, and makeRequest).
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/src/features/apiClient/helpers/httpRequestExecutor/httpRequestExecutor.tsapp/src/features/apiClient/screens/apiClient/utils.ts
nafees87n
approved these changes
Feb 27, 2026
app/src/features/apiClient/helpers/httpRequestExecutor/httpRequestExecutor.ts
Show resolved
Hide resolved
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes issue:
📜 Summary of changes:
🎥 Demo Video:
Video/Demo:
✅ Checklist:
🧪 Test instructions:
🔗 Other references:
Summary by CodeRabbit
Bug Fixes
New Features