Conversation
- Replace supportsRequestBody with supportsGetRequestBody based on app mode - Allow GET requests to have a body in desktop mode only - Remove unnecessary request body checks in sanitizeEntry - Update HttpRecordEntity to only clear body for HEAD requests
Show a message and desktop app download link when users try to send a body with a GET request in the web version, which is not supported.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds app-mode (desktop vs web) awareness to request-body handling: method-based body gating was removed and replaced with app-mode–based checks. The Send action is runtime-gated on web when request bodies or multipart data are required, wrapped in a Tooltip that shows a "Download Requestly" CTA and tracks promo clicks. Request methods no longer auto-clear bodies or content-type on change. A new RequestBodyRedirectScreen component and styles were added for web users; utils were updated (removed supportsRequestBody(method), added supportsRequestBodyForAllMethods(appMode)), and UI tabs now conditionally render the body editor or the redirect screen. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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.
Actionable comments posted: 2
🧹 Nitpick comments (2)
app/src/features/apiClient/screens/apiClient/components/clientView/components/GetRequestBodyRedirectScreen/index.scss (1)
25-41: Consider using design-system CSS variables for font sizes.The rest of the codebase (e.g.,
httpClientView.scss) usesvar(--requestly-font-size-xs)andvar(--requestly-font-weight-medium). Hardcoded14px,11px,500,400values here could drift from the design system.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/features/apiClient/screens/apiClient/components/clientView/components/GetRequestBodyRedirectScreen/index.scss` around lines 25 - 41, Replace hardcoded font sizes and weights in .get-request-body-redirect-screen-title and .get-request-body-redirect-screen-description with the design-system CSS variables used elsewhere (e.g., var(--requestly-font-size-xs), var(--requestly-font-weight-medium), and the subtle text color var already used), so update the font-size and font-weight declarations to reference the shared variables instead of 14px/11px and 500/400 to keep typography consistent with httpClientView.scss.app/src/features/apiClient/screens/apiClient/components/clientView/components/GetRequestBodyRedirectScreen/index.tsx (1)
26-32: Consider extracting the download URL to a shared constant.The URL
https://requestly.com/downloads/desktop/is duplicated here and inHttpClientView.tsx(line 629). A shared constant would prevent drift if the URL changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/features/apiClient/screens/apiClient/components/clientView/components/GetRequestBodyRedirectScreen/index.tsx` around lines 26 - 32, The download URL literal "https://requestly.com/downloads/desktop/" is duplicated; extract it to a shared constant (e.g., DESKTOP_DOWNLOAD_URL) in a common module (constants or urls) and replace the inline string in GetRequestBodyRedirectScreen's RQButton onClick/href and the occurrence in HttpClientView (the other duplicated usage) to import and use that constant; ensure imports are added and tests/build still pass.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@app/src/features/apiClient/screens/apiClient/components/views/http/components/HttpRequestTabs/HttpRequestTabs.tsx`:
- Around line 77-82: The useMemo that builds the request tabs references method
=== RequestMethod.HEAD (so RequestMethod and method are used) but method is not
in the dependency array; update the useMemo (in HttpRequestTabs) to include
method in its dependency list so changes to method (e.g., switching to/from
HEAD) re-run the memo and satisfy ESLint exhaustive-deps—locate the useMemo that
returns the tabs (the block creating the RequestTab.BODY entry) and add method
to its dependencies alongside the existing deps like isRequestBodySupported and
bodyLength.
In
`@app/src/features/apiClient/screens/apiClient/components/views/http/HttpClientView.tsx`:
- Around line 145-148: Replace the hardcoded "DESKTOP" string check in the
isMultipartInNonDesktop expression with the shared constant
(CONSTANTS.APP_MODES.DESKTOP) to match supportsGetRequestBody(appMode) and avoid
silent breakage if the constant changes; alternatively, add a small helper
supportsMultipartFormData(appMode) (alongside supportsGetRequestBody) and use
that in isMultipartInNonDesktop so the gating logic for contentType ===
RequestContentType.MULTIPART_FORM and appMode is centralized and consistent
(update references: isMultipartInNonDesktop, isGetWithBodyInNonDesktop,
isSendDisabledDueToDesktopOnly, contentType, RequestContentType.MULTIPART_FORM,
appMode).
---
Nitpick comments:
In
`@app/src/features/apiClient/screens/apiClient/components/clientView/components/GetRequestBodyRedirectScreen/index.scss`:
- Around line 25-41: Replace hardcoded font sizes and weights in
.get-request-body-redirect-screen-title and
.get-request-body-redirect-screen-description with the design-system CSS
variables used elsewhere (e.g., var(--requestly-font-size-xs),
var(--requestly-font-weight-medium), and the subtle text color var already
used), so update the font-size and font-weight declarations to reference the
shared variables instead of 14px/11px and 500/400 to keep typography consistent
with httpClientView.scss.
In
`@app/src/features/apiClient/screens/apiClient/components/clientView/components/GetRequestBodyRedirectScreen/index.tsx`:
- Around line 26-32: The download URL literal
"https://requestly.com/downloads/desktop/" is duplicated; extract it to a shared
constant (e.g., DESKTOP_DOWNLOAD_URL) in a common module (constants or urls) and
replace the inline string in GetRequestBodyRedirectScreen's RQButton
onClick/href and the occurrence in HttpClientView (the other duplicated usage)
to import and use that constant; ensure imports are added and tests/build still
pass.
...lient/screens/apiClient/components/views/http/components/HttpRequestTabs/HttpRequestTabs.tsx
Show resolved
Hide resolved
app/src/features/apiClient/screens/apiClient/components/views/http/HttpClientView.tsx
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
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)
747-764:⚠️ Potential issue | 🟡 MinorUse a null/undefined check instead of a truthy check for HAR body export.
Line 747 currently skips
postDatawhen body is an empty string (""), which is still a valid payload state.🔧 Proposed fix
- if (apiRequest.body) { + if (apiRequest.body !== null && apiRequest.body !== undefined) {🤖 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 747 - 764, The conditional that sets harRequest.postData incorrectly uses a truthy check (if (apiRequest.body)) which skips valid empty-string bodies; change it to explicitly check for null/undefined (e.g., if (apiRequest.body != null) or apiRequest.body !== null && apiRequest.body !== undefined) before populating harRequest.postData so empty-string bodies are exported; update the existing branches that reference apiRequest.contentType / RequestContentType (RAW, JSON, FORM) and leave the postData construction logic unchanged.
♻️ Duplicate comments (1)
app/src/features/apiClient/screens/apiClient/components/views/http/HttpClientView.tsx (1)
145-149:⚠️ Potential issue | 🟡 MinorReplace hardcoded app-mode string with shared constant/helper.
Line 148 still checks
appMode !== "DESKTOP"directly; this should stay aligned with the shared app-mode constants/utilities.🔧 Proposed fix
- const isMultipartInNonDesktop = contentType === RequestContentType.MULTIPART_FORM && appMode !== "DESKTOP"; + const isMultipartInNonDesktop = + contentType === RequestContentType.MULTIPART_FORM && !supportsRequestBodyForAllMethods(appMode);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/features/apiClient/screens/apiClient/components/views/http/HttpClientView.tsx` around lines 145 - 149, Replace the hardcoded "DESKTOP" string comparison by using the shared app-mode constant or helper; specifically, update where isMultipartInNonDesktop is computed (and any other checks using appMode !== "DESKTOP") to use the shared symbol (for example an exported AppMode.DESKTOP constant or an isDesktop(appMode) helper) so the logic remains consistent across the app; import the constant/helper at the top and use it in the expression that defines isMultipartInNonDesktop (and similarly update isNonBodyMethodWithBodyInNonDesktop if needed).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@app/src/features/apiClient/screens/apiClient/components/clientView/components/RequestBodyRedirectScreen/index.tsx`:
- Around line 26-30: The external link opened by the RQButton in
RequestBodyRedirectScreen uses target="_blank" but is missing the rel attribute;
update the RQButton invocation (the element that includes onClick, href and
target) to add rel="noopener noreferrer" so it matches other external links and
prevents tabnabbing (ensure you add the rel prop on the same RQButton element).
In
`@app/src/features/apiClient/screens/apiClient/components/views/http/HttpClientView.tsx`:
- Around line 614-654: Replace the Ant Design Tooltip with Popover around the
promo content: move the interactive promo block (the div with className
"send-disabled-tooltip" containing the RQButton that calls
trackDesktopAppPromoClicked and links to the desktop download) into a Popover's
content prop so interactive elements work; ensure the Popover placement matches
the original. Wrap the primary "Send" RQButton in a non-disabled <span> when
disabled (i.e., when !url || isSendDisabledDueToDesktopOnly) so the Popover can
be triggered even for a disabled button, keeping enableHotkey and
onSendButtonClick behavior intact. Also add rel="noopener noreferrer" to the
promo RQButton that has href, and preserve references to
isSendDisabledDueToDesktopOnly, isNonBodyMethodWithBodyInNonDesktop,
trackDesktopAppPromoClicked, and onSendButtonClick to locate the changes.
---
Outside diff comments:
In `@app/src/features/apiClient/screens/apiClient/utils.ts`:
- Around line 747-764: The conditional that sets harRequest.postData incorrectly
uses a truthy check (if (apiRequest.body)) which skips valid empty-string
bodies; change it to explicitly check for null/undefined (e.g., if
(apiRequest.body != null) or apiRequest.body !== null && apiRequest.body !==
undefined) before populating harRequest.postData so empty-string bodies are
exported; update the existing branches that reference apiRequest.contentType /
RequestContentType (RAW, JSON, FORM) and leave the postData construction logic
unchanged.
---
Duplicate comments:
In
`@app/src/features/apiClient/screens/apiClient/components/views/http/HttpClientView.tsx`:
- Around line 145-149: Replace the hardcoded "DESKTOP" string comparison by
using the shared app-mode constant or helper; specifically, update where
isMultipartInNonDesktop is computed (and any other checks using appMode !==
"DESKTOP") to use the shared symbol (for example an exported AppMode.DESKTOP
constant or an isDesktop(appMode) helper) so the logic remains consistent across
the app; import the constant/helper at the top and use it in the expression that
defines isMultipartInNonDesktop (and similarly update
isNonBodyMethodWithBodyInNonDesktop if needed).
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/src/features/apiClient/screens/apiClient/components/clientView/components/RequestBodyRedirectScreen/index.scssapp/src/features/apiClient/screens/apiClient/components/clientView/components/RequestBodyRedirectScreen/index.tsxapp/src/features/apiClient/screens/apiClient/components/views/http/HttpClientView.tsxapp/src/features/apiClient/screens/apiClient/components/views/http/components/HttpRequestTabs/HttpRequestTabs.tsxapp/src/features/apiClient/screens/apiClient/utils.tsapp/src/features/apiClient/slices/entities/http.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/features/apiClient/slices/entities/http.ts
...lient/screens/apiClient/components/clientView/components/RequestBodyRedirectScreen/index.tsx
Show resolved
Hide resolved
Comment on lines
+614
to
+654
| <Tooltip | ||
| title={ | ||
| isSendDisabledDueToDesktopOnly ? ( | ||
| <div className="send-disabled-tooltip"> | ||
| <span> | ||
| {isNonBodyMethodWithBodyInNonDesktop | ||
| ? "Sending request body with this method requires the desktop app." | ||
| : "Sending multipart/form-data request requires the desktop app."} | ||
| </span> | ||
| <RQButton | ||
| size="small" | ||
| onClick={() => | ||
| trackDesktopAppPromoClicked( | ||
| isNonBodyMethodWithBodyInNonDesktop ? "request_body_send" : "multipart_form_data_send", | ||
| "web_app" | ||
| ) | ||
| } | ||
| href="https://requestly.com/downloads/desktop/" | ||
| target="_blank" | ||
| > | ||
| Download Requestly | ||
| </RQButton> | ||
| </div> | ||
| ) : null | ||
| } | ||
| placement="bottom" | ||
| > | ||
| Send | ||
| </RQButton> | ||
| <> | ||
| <RQButton | ||
| showHotKeyText | ||
| onClick={onSendButtonClick} | ||
| hotKey={KEYBOARD_SHORTCUTS.API_CLIENT.SEND_REQUEST!.hotKey} | ||
| type="primary" | ||
| className="text-bold" | ||
| enableHotKey={enableHotkey && !isSendDisabledDueToDesktopOnly} | ||
| disabled={!url || isSendDisabledDueToDesktopOnly} | ||
| > | ||
| Send | ||
| </RQButton> | ||
| </> | ||
| </Tooltip> |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the file and verify it exists
find . -type f -name "HttpClientView.tsx" | head -20Repository: requestly/requestly
Length of output: 150
🏁 Script executed:
# Read the file at the specified lines with context
sed -n '600,670p' ./app/src/features/apiClient/screens/apiClient/components/views/http/HttpClientView.tsx | cat -nRepository: requestly/requestly
Length of output: 3495
🏁 Script executed:
# Check the import statements at the top of the file
head -50 ./app/src/features/apiClient/screens/apiClient/components/views/http/HttpClientView.tsx | grep -E "^import|^from"Repository: requestly/requestly
Length of output: 1948
🌐 Web query:
Ant Design Tooltip disabled button interactive content Popover
💡 Result:
-
If you need interactive overlay content (buttons/links/forms), use
Popover, notTooltip. AntD explicitly notes Tooltip “doesn't support complex text or operations”. [1] -
Disabled
<Button>and hover tooltips/popovers- Historically, disabled elements don’t fire mouse events, so the common workaround is wrapping the disabled button in a non-disabled element (e.g.
span) and attachingTooltip/Popoverto that wrapper. - In newer AntD versions, Tooltip changed its internal “wrap
spanfor disabled” behavior and is intended to “always work … on disabled element” (release note). [2] - Practical takeaway: if your tooltip/popover doesn’t trigger on a disabled button, wrap it (this works across versions).
<Popover content={<div><a onClick={e => e.preventDefault()}>Action</a></div>} trigger="hover"> <span style={{ display: 'inline-block' }}> <Button disabled>Disabled</Button> </span> </Popover>
- Historically, disabled elements don’t fire mouse events, so the common workaround is wrapping the disabled button in a non-disabled element (e.g.
-
Keeping a Popover open / controlling close behavior
- Use controlled visibility (
open/visible+onOpenChange/onVisibleChange) and close it only from your own UI (e.g., a “Close” link/button in the content). Example approach shown here withvisiblestate. [3]
- Use controlled visibility (
-
If popups inside popups disappear (Select/DatePicker/etc. inside Popover):
- Render inner popups into the trigger’s parent via
getPopupContainer={trigger => trigger.parentElement}(or configure globally viaConfigProvider). [4]
- Render inner popups into the trigger’s parent via
Sources: [1] [2] [3] [4]
🏁 Script executed:
# Check if there are similar patterns in the codebase with Tooltip + disabled button + interactive content
rg -B2 -A2 'Tooltip.*\n.*disabled=.*true' ./app/src/features/apiClient --type tsx --type ts | head -40Repository: requestly/requestly
Length of output: 254
Replace Tooltip with Popover for interactive content and wrap the disabled button in a <span> wrapper.
The current implementation uses Tooltip with an interactive button containing an onClick handler and href link. According to Ant Design documentation, Tooltip doesn't support complex interactive content; Popover is required. Additionally, disabled buttons may not reliably trigger mouse events—wrapping the button in a non-disabled <span> element ensures consistent interaction across browser versions. The external link is also missing rel="noopener noreferrer".
🔧 Proposed fix
-import { notification, Select, Space, Tooltip } from "antd";
+import { notification, Popover, Select, Space } from "antd";
- <Tooltip
- title={
+ <Popover
+ content={
isSendDisabledDueToDesktopOnly ? (
<div className="send-disabled-tooltip">
<span>
{isNonBodyMethodWithBodyInNonDesktop
? "Sending request body with this method requires the desktop app."
: "Sending multipart/form-data request requires the desktop app."}
</span>
<RQButton
size="small"
onClick={() =>
trackDesktopAppPromoClicked(
isNonBodyMethodWithBodyInNonDesktop ? "request_body_send" : "multipart_form_data_send",
"web_app"
)
}
href="https://requestly.com/downloads/desktop/"
target="_blank"
+ rel="noopener noreferrer"
>
Download Requestly
</RQButton>
</div>
) : null
}
placement="bottom"
+ trigger={isSendDisabledDueToDesktopOnly ? "hover" : []}
>
- <>
+ <span>
<RQButton
showHotKeyText
onClick={onSendButtonClick}
hotKey={KEYBOARD_SHORTCUTS.API_CLIENT.SEND_REQUEST!.hotKey}
type="primary"
className="text-bold"
enableHotKey={enableHotkey && !isSendDisabledDueToDesktopOnly}
disabled={!url || isSendDisabledDueToDesktopOnly}
>
Send
</RQButton>
- </>
- </Tooltip>
+ </span>
+ </Popover>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <Tooltip | |
| title={ | |
| isSendDisabledDueToDesktopOnly ? ( | |
| <div className="send-disabled-tooltip"> | |
| <span> | |
| {isNonBodyMethodWithBodyInNonDesktop | |
| ? "Sending request body with this method requires the desktop app." | |
| : "Sending multipart/form-data request requires the desktop app."} | |
| </span> | |
| <RQButton | |
| size="small" | |
| onClick={() => | |
| trackDesktopAppPromoClicked( | |
| isNonBodyMethodWithBodyInNonDesktop ? "request_body_send" : "multipart_form_data_send", | |
| "web_app" | |
| ) | |
| } | |
| href="https://requestly.com/downloads/desktop/" | |
| target="_blank" | |
| > | |
| Download Requestly | |
| </RQButton> | |
| </div> | |
| ) : null | |
| } | |
| placement="bottom" | |
| > | |
| Send | |
| </RQButton> | |
| <> | |
| <RQButton | |
| showHotKeyText | |
| onClick={onSendButtonClick} | |
| hotKey={KEYBOARD_SHORTCUTS.API_CLIENT.SEND_REQUEST!.hotKey} | |
| type="primary" | |
| className="text-bold" | |
| enableHotKey={enableHotkey && !isSendDisabledDueToDesktopOnly} | |
| disabled={!url || isSendDisabledDueToDesktopOnly} | |
| > | |
| Send | |
| </RQButton> | |
| </> | |
| </Tooltip> | |
| <Popover | |
| content={ | |
| isSendDisabledDueToDesktopOnly ? ( | |
| <div className="send-disabled-tooltip"> | |
| <span> | |
| {isNonBodyMethodWithBodyInNonDesktop | |
| ? "Sending request body with this method requires the desktop app." | |
| : "Sending multipart/form-data request requires the desktop app."} | |
| </span> | |
| <RQButton | |
| size="small" | |
| onClick={() => | |
| trackDesktopAppPromoClicked( | |
| isNonBodyMethodWithBodyInNonDesktop ? "request_body_send" : "multipart_form_data_send", | |
| "web_app" | |
| ) | |
| } | |
| href="https://requestly.com/downloads/desktop/" | |
| target="_blank" | |
| rel="noopener noreferrer" | |
| > | |
| Download Requestly | |
| </RQButton> | |
| </div> | |
| ) : null | |
| } | |
| placement="bottom" | |
| trigger={isSendDisabledDueToDesktopOnly ? "hover" : []} | |
| > | |
| <span> | |
| <RQButton | |
| showHotKeyText | |
| onClick={onSendButtonClick} | |
| hotKey={KEYBOARD_SHORTCUTS.API_CLIENT.SEND_REQUEST!.hotKey} | |
| type="primary" | |
| className="text-bold" | |
| enableHotKey={enableHotkey && !isSendDisabledDueToDesktopOnly} | |
| disabled={!url || isSendDisabledDueToDesktopOnly} | |
| > | |
| Send | |
| </RQButton> | |
| </span> | |
| </Popover> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@app/src/features/apiClient/screens/apiClient/components/views/http/HttpClientView.tsx`
around lines 614 - 654, Replace the Ant Design Tooltip with Popover around the
promo content: move the interactive promo block (the div with className
"send-disabled-tooltip" containing the RQButton that calls
trackDesktopAppPromoClicked and links to the desktop download) into a Popover's
content prop so interactive elements work; ensure the Popover placement matches
the original. Wrap the primary "Send" RQButton in a non-disabled <span> when
disabled (i.e., when !url || isSendDisabledDueToDesktopOnly) so the Popover can
be triggered even for a disabled button, keeping enableHotkey and
onSendButtonClick behavior intact. Also add rel="noopener noreferrer" to the
promo RQButton that has href, and preserve references to
isSendDisabledDueToDesktopOnly, isNonBodyMethodWithBodyInNonDesktop,
trackDesktopAppPromoClicked, and onSendButtonClick to locate the changes.
Contributor
Author
There was a problem hiding this comment.
Skipping — this is a pre-existing pattern. The Tooltip with interactive content works in practice. Changing to Popover is out of scope for this PR.
Contributor
There was a problem hiding this comment.
@srbh777, understood! I appreciate you clarifying the context. Since this pattern is already established in your codebase and works for your use case, it makes sense to keep it out of scope for this PR. Feel free to revisit it in a future refactor if needed. 👍
🧠 Learnings used
Learnt from: rohitneharabrowserstack
Repo: requestly/requestly PR: 4360
File: app/src/features/settings/components/DesktopSettings/index.jsx:71-102
Timestamp: 2026-02-18T14:42:23.012Z
Learning: In the Desktop Settings URL change feature (app/src/features/settings/components/DesktopSettings/index.jsx), the validation for URL input (isValidUrl and containsRequiredKeywords) is intentionally performed after the Popconfirm dialog confirmation, not before. This means validation error toasts appear after the user confirms, rather than disabling the button for invalid inputs.
Learnt from: mustafa-sayyed
Repo: requestly/requestly PR: 3723
File: app/src/features/apiClient/screens/apiClient/components/views/http/components/HttpRequestTabs/HttpRequestTabs.tsx:146-151
Timestamp: 2025-10-22T22:57:45.959Z
Learning: In the API Client Scripts tab (app/src/features/apiClient/screens/apiClient/components/views/http/components/HttpRequestTabs/HttpRequestTabs.tsx), the dot indicator should only appear when scripts are present (count > 0). When no scripts exist, the dot should not be rendered. This is the intended behavior per issue `#3731`.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
app/src/features/apiClient/screens/apiClient/components/views/http/HttpClientView.tsx (2)
615-655: Keeping this on the existing review thread.This hunk already has active feedback in prior review comments; consolidating discussion there.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/features/apiClient/screens/apiClient/components/views/http/HttpClientView.tsx` around lines 615 - 655, Duplicate review feedback was posted for the Tooltip/RQButton send-button block; remove the redundant comment and keep the original review thread active. Locate the Tooltip containing RQButton, onSendButtonClick, KEYBOARD_SHORTCUTS.API_CLIENT.SEND_REQUEST, and isSendDisabledDueToDesktopOnly, and delete the duplicate review note so only the original feedback remains associated with that code block.
146-150:⚠️ Potential issue | 🟡 MinorReplace hardcoded app mode string with shared constant.
Line 149 uses
"DESKTOP"directly, which can drift from centralized app-mode constants and helper logic.🔧 Proposed fix
+import { CONSTANTS } from "@requestly/requestly-core"; ... - const isMultipartInNonDesktop = contentType === RequestContentType.MULTIPART_FORM && appMode !== "DESKTOP"; + const isMultipartInNonDesktop = + contentType === RequestContentType.MULTIPART_FORM && appMode !== CONSTANTS.APP_MODES.DESKTOP;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/features/apiClient/screens/apiClient/components/views/http/HttpClientView.tsx` around lines 146 - 150, The code checks appMode === "DESKTOP" with a hardcoded string; replace that literal with the shared app-mode constant (e.g., AppMode.DESKTOP or the existing exported APP_MODE_DESKTOP) to avoid drift. Update the reference in isMultipartInNonDesktop and any other comparisons (symbols: isMultipartInNonDesktop, isSendDisabledDueToDesktopOnly, appMode, RequestContentType.MULTIPART_FORM) and add the necessary import for the shared constant at the top of HttpClientView.tsx. Ensure the comparison uses the named constant the same way supportsRequestBodyForAllMethods is used.
🧹 Nitpick comments (1)
app/src/features/apiClient/screens/apiClient/utils.ts (1)
253-254: TightenappModetyping to avoid string-literal drift.Using
stringhere makes accidental values easy to pass and hard to catch at compile time.♻️ Proposed typing refinement
-export const supportsRequestBodyForAllMethods = (appMode: string): boolean => { +export const supportsRequestBodyForAllMethods = ( + appMode: (typeof CONSTANTS.APP_MODES)[keyof typeof CONSTANTS.APP_MODES] +): boolean => { return appMode === CONSTANTS.APP_MODES.DESKTOP; };🤖 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 253 - 254, The function supportsRequestBodyForAllMethods currently accepts a plain string which permits invalid values; change the parameter typing to a tighter AppMode union derived from the known modes (e.g., an AppMode type that equals the keys/values of CONSTANTS.APP_MODES or an explicit union like 'DESKTOP'|'WEB' etc.) and update the signature of supportsRequestBodyForAllMethods(appMode: AppMode): boolean to use that type; ensure any callers are updated to pass the typed AppMode (or cast at callsites) so accidental string-literal drift is caught at compile time and the comparison against CONSTANTS.APP_MODES.DESKTOP remains valid.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@app/src/features/apiClient/screens/apiClient/components/views/http/HttpClientView.tsx`:
- Around line 615-655: Duplicate review feedback was posted for the
Tooltip/RQButton send-button block; remove the redundant comment and keep the
original review thread active. Locate the Tooltip containing RQButton,
onSendButtonClick, KEYBOARD_SHORTCUTS.API_CLIENT.SEND_REQUEST, and
isSendDisabledDueToDesktopOnly, and delete the duplicate review note so only the
original feedback remains associated with that code block.
- Around line 146-150: The code checks appMode === "DESKTOP" with a hardcoded
string; replace that literal with the shared app-mode constant (e.g.,
AppMode.DESKTOP or the existing exported APP_MODE_DESKTOP) to avoid drift.
Update the reference in isMultipartInNonDesktop and any other comparisons
(symbols: isMultipartInNonDesktop, isSendDisabledDueToDesktopOnly, appMode,
RequestContentType.MULTIPART_FORM) and add the necessary import for the shared
constant at the top of HttpClientView.tsx. Ensure the comparison uses the named
constant the same way supportsRequestBodyForAllMethods is used.
---
Nitpick comments:
In `@app/src/features/apiClient/screens/apiClient/utils.ts`:
- Around line 253-254: The function supportsRequestBodyForAllMethods currently
accepts a plain string which permits invalid values; change the parameter typing
to a tighter AppMode union derived from the known modes (e.g., an AppMode type
that equals the keys/values of CONSTANTS.APP_MODES or an explicit union like
'DESKTOP'|'WEB' etc.) and update the signature of
supportsRequestBodyForAllMethods(appMode: AppMode): boolean to use that type;
ensure any callers are updated to pass the typed AppMode (or cast at callsites)
so accidental string-literal drift is caught at compile time and the comparison
against CONSTANTS.APP_MODES.DESKTOP remains valid.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/src/features/apiClient/screens/apiClient/components/views/http/HttpClientView.tsxapp/src/features/apiClient/screens/apiClient/utils.tsapp/src/features/apiClient/slices/entities/http.ts
💤 Files with no reviewable changes (1)
- app/src/features/apiClient/slices/entities/http.ts
nafees87n
approved these changes
Feb 25, 2026
Comment on lines
+59
to
+62
| const METHODS_WITHOUT_BODY = [RequestMethod.GET, RequestMethod.HEAD]; | ||
| const needsDesktopForBody = METHODS_WITHOUT_BODY.includes(method); | ||
| const isRequestBodySupported = !needsDesktopForBody || supportsRequestBodyForAllMethods(appMode); | ||
| const isNonBodyMethodInNonDesktop = needsDesktopForBody && !supportsRequestBodyForAllMethods(appMode); |
Contributor
There was a problem hiding this comment.
Nitpick for better readability
Suggested change
| const METHODS_WITHOUT_BODY = [RequestMethod.GET, RequestMethod.HEAD]; | |
| const needsDesktopForBody = METHODS_WITHOUT_BODY.includes(method); | |
| const isRequestBodySupported = !needsDesktopForBody || supportsRequestBodyForAllMethods(appMode); | |
| const isNonBodyMethodInNonDesktop = needsDesktopForBody && !supportsRequestBodyForAllMethods(appMode); | |
| const METHODS_WITHOUT_BODY = [RequestMethod.GET, RequestMethod.HEAD]; | |
| const supportsRequestBody=supportsRequestBodyForAllMethods(appMode); | |
| const needsDesktopForBody = METHODS_WITHOUT_BODY.includes(method); | |
| const canHaveRequestBody = supportsRequestBody || !needsDesktopForBody; |
isRequestBodySupported and isNonBodyMethodInNonDesktop are just negations of each other. Having both is making it difficult to read
Contributor
Author
There was a problem hiding this comment.
Good call — simplified to a single canHaveRequestBody boolean and removed the duplicate negated variable.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
app/src/features/apiClient/screens/apiClient/components/views/http/HttpClientView.tsx (1)
616-656:⚠️ Potential issue | 🟠 MajorReplace
TooltipwithPopoverfor interactive content and add security attributes.This segment has multiple issues that were flagged in a previous review:
Interactive content in Tooltip: The tooltip contains an
RQButtonwithonClickandhref. Per Ant Design documentation,Tooltipdoesn't support interactive content—usePopoverinstead.Disabled button hover reliability: Disabled buttons may not reliably trigger mouse events. Wrap the Send button in a
<span>instead of an empty fragment<>to ensure consistent hover behavior.Missing
relattribute: The external link (line 633-634) withtarget="_blank"should includerel="noopener noreferrer"for security.Proposed fix
-import { notification, Select, Space, Tooltip } from "antd"; +import { notification, Popover, Select, Space } from "antd"; // ... at lines 616-656: - <Tooltip - title={ + <Popover + content={ isSendDisabledDueToDesktopOnly ? ( <div className="send-disabled-tooltip"> <span> {isNonBodyMethodWithBodyInNonDesktop ? "Sending request body with this method requires the desktop app." : "Sending multipart/form-data request requires the desktop app."} </span> <RQButton size="small" onClick={() => trackDesktopAppPromoClicked( isNonBodyMethodWithBodyInNonDesktop ? "request_body_send" : "multipart_form_data_send", "web_app" ) } href="https://requestly.com/downloads/desktop/" target="_blank" + rel="noopener noreferrer" > Download Requestly </RQButton> </div> ) : null } placement="bottom" + trigger={isSendDisabledDueToDesktopOnly ? "hover" : []} > - <> + <span style={{ display: 'inline-block' }}> <RQButton showHotKeyText onClick={onSendButtonClick} hotKey={KEYBOARD_SHORTCUTS.API_CLIENT.SEND_REQUEST!.hotKey} type="primary" className="text-bold" enableHotKey={enableHotkey && !isSendDisabledDueToDesktopOnly} disabled={!url || isSendDisabledDueToDesktopOnly} > Send </RQButton> - </> - </Tooltip> + </span> + </Popover>,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/features/apiClient/screens/apiClient/components/views/http/HttpClientView.tsx` around lines 616 - 656, Replace the non-interactive Tooltip with Ant Design's Popover for the block that renders the send UI (replace the Tooltip component wrapping the RQButton), move the conditional content into Popover.content so the RQButton inside the promo is interactive, and on the external-download RQButton add rel="noopener noreferrer" alongside target="_blank"; also ensure the visible Send RQButton (onSendButtonClick, hotKey handling via KEYBOARD_SHORTCUTS.API_CLIENT.SEND_REQUEST, enableHotkey, disabled prop using isSendDisabledDueToDesktopOnly) is wrapped in a <span> (not an empty fragment) so hover events fire reliably even when disabled.
🧹 Nitpick comments (2)
app/src/features/apiClient/screens/apiClient/components/views/http/HttpClientView.tsx (1)
146-146: Consider extractingMETHODS_WITHOUT_BODYto a shared constant.This same constant is defined in both
HttpRequestTabs.tsxand here. Extracting it to a shared location (e.g.,utils.tsalongsidesupportsRequestBodyForAllMethods) would reduce duplication and ensure consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/features/apiClient/screens/apiClient/components/views/http/HttpClientView.tsx` at line 146, Extract the duplicate METHODS_WITHOUT_BODY array into a single exported constant in a shared module (e.g., create or update utils.ts alongside supportsRequestBodyForAllMethods), then import and use that constant in both HttpClientView (currently declaring METHODS_WITHOUT_BODY) and HttpRequestTabs so both files reference the same symbol; update any references to RequestMethod.GET/HEAD usage to use the new shared constant name and remove the local duplicate declarations.app/src/features/apiClient/screens/apiClient/components/views/http/components/HttpRequestTabs/HttpRequestTabs.tsx (1)
59-62: Consider movingMETHODS_WITHOUT_BODYoutside the component.The constant array
METHODS_WITHOUT_BODYis recreated on every render. Since it's a static value, moving it outside the component scope improves performance slightly and signals intent more clearly.Suggested refactor
+const METHODS_WITHOUT_BODY = [RequestMethod.GET, RequestMethod.HEAD]; + const HttpRequestTabs: React.FC<Props> = ({ error, entity, // ... }) => { // ... - const METHODS_WITHOUT_BODY = [RequestMethod.GET, RequestMethod.HEAD]; const supportsRequestBody = supportsRequestBodyForAllMethods(appMode);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/features/apiClient/screens/apiClient/components/views/http/components/HttpRequestTabs/HttpRequestTabs.tsx` around lines 59 - 62, The constant METHODS_WITHOUT_BODY is being recreated on every render; move its declaration outside the component scope in HttpRequestTabs.tsx (e.g., top-level module scope) so it is instantiated once. Update any references (METHODS_WITHOUT_BODY, supportsRequestBodyForAllMethods, supportsRequestBody, needsDesktopForBody, canHaveRequestBody) to use the top-level constant; no other logic changes are needed — keep method and appMode usage as-is.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@app/src/features/apiClient/screens/apiClient/components/views/http/HttpClientView.tsx`:
- Around line 616-656: Replace the non-interactive Tooltip with Ant Design's
Popover for the block that renders the send UI (replace the Tooltip component
wrapping the RQButton), move the conditional content into Popover.content so the
RQButton inside the promo is interactive, and on the external-download RQButton
add rel="noopener noreferrer" alongside target="_blank"; also ensure the visible
Send RQButton (onSendButtonClick, hotKey handling via
KEYBOARD_SHORTCUTS.API_CLIENT.SEND_REQUEST, enableHotkey, disabled prop using
isSendDisabledDueToDesktopOnly) is wrapped in a <span> (not an empty fragment)
so hover events fire reliably even when disabled.
---
Nitpick comments:
In
`@app/src/features/apiClient/screens/apiClient/components/views/http/components/HttpRequestTabs/HttpRequestTabs.tsx`:
- Around line 59-62: The constant METHODS_WITHOUT_BODY is being recreated on
every render; move its declaration outside the component scope in
HttpRequestTabs.tsx (e.g., top-level module scope) so it is instantiated once.
Update any references (METHODS_WITHOUT_BODY, supportsRequestBodyForAllMethods,
supportsRequestBody, needsDesktopForBody, canHaveRequestBody) to use the
top-level constant; no other logic changes are needed — keep method and appMode
usage as-is.
In
`@app/src/features/apiClient/screens/apiClient/components/views/http/HttpClientView.tsx`:
- Line 146: Extract the duplicate METHODS_WITHOUT_BODY array into a single
exported constant in a shared module (e.g., create or update utils.ts alongside
supportsRequestBodyForAllMethods), then import and use that constant in both
HttpClientView (currently declaring METHODS_WITHOUT_BODY) and HttpRequestTabs so
both files reference the same symbol; update any references to
RequestMethod.GET/HEAD usage to use the new shared constant name and remove the
local duplicate declarations.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/src/features/apiClient/screens/apiClient/components/clientView/components/RequestBodyRedirectScreen/index.tsxapp/src/features/apiClient/screens/apiClient/components/views/http/HttpClientView.tsxapp/src/features/apiClient/screens/apiClient/components/views/http/components/HttpRequestTabs/HttpRequestTabs.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/features/apiClient/screens/apiClient/components/clientView/components/RequestBodyRedirectScreen/index.tsx
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
app/src/features/apiClient/screens/apiClient/components/views/http/components/HttpRequestTabs/HttpRequestTabs.tsx (1)
78-82:⚠️ Potential issue | 🟡 MinorAdd
methodto the useMemo dependency array.The
methodvariable is used directly at line 81 (passed toRequestBodyRedirectScreen), but it's not in the dependency array (lines 127-141). If a user switches between two methods that both require desktop (e.g., GET → HEAD),canHaveRequestBodyremainsfalse, the memo won't re-run, andRequestBodyRedirectScreenwould receive a stalemethodprop.🐛 Proposed fix
}, [ queryParams.length, pathVariables?.length, entity, bodyLength, canHaveRequestBody, + method, headersLength, auth, handleAuthChange,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/features/apiClient/screens/apiClient/components/views/http/components/HttpRequestTabs/HttpRequestTabs.tsx` around lines 78 - 82, The memo that constructs the RequestTab for RequestTab.BODY uses the local variable method when rendering RequestBodyRedirectScreen but method is missing from the useMemo dependency array; update the useMemo dependencies (the one that returns the array including the RequestTab.BODY entry) to include method so the memo re-runs when method changes, ensuring RequestBodyRedirectScreen receives the current method prop.app/src/features/apiClient/screens/apiClient/components/views/http/HttpClientView.tsx (1)
626-638:⚠️ Potential issue | 🟡 MinorAdd
rel="noopener noreferrer"to the external link.The button opens an external URL in a new tab but is missing the
relattribute. This is a minor security best practice to prevent the new page from accessingwindow.opener.🛡️ Proposed fix
<RQButton size="small" onClick={() => trackDesktopAppPromoClicked( isNonBodyMethodWithBodyInNonDesktop ? "request_body_send" : "multipart_form_data_send", "web_app" ) } href="https://requestly.com/downloads/desktop/" target="_blank" + rel="noopener noreferrer" > Download Requestly </RQButton>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/features/apiClient/screens/apiClient/components/views/http/HttpClientView.tsx` around lines 626 - 638, The external link in the RQButton lacks the rel attribute which is a security best practice; update the RQButton instance (the one that calls trackDesktopAppPromoClicked and uses href="https://requestly.com/downloads/desktop/") to include rel="noopener noreferrer" alongside target="_blank" so the opened page cannot access window.opener; ensure you add the rel prop to the same RQButton element that renders the "Download Requestly" button.
🧹 Nitpick comments (1)
app/src/features/apiClient/screens/apiClient/components/views/http/HttpClientView.tsx (1)
146-151: ExtractMETHODS_WITHOUT_BODYto a shared constant.This array is duplicated in
HttpRequestTabs.tsx(line 59). Consider extracting it toutils.tsalongsidesupportsRequestBodyForAllMethodsto maintain a single source of truth.♻️ Suggested refactor in utils.ts
// In utils.ts export const METHODS_WITHOUT_BODY = [RequestMethod.GET, RequestMethod.HEAD];Then import and use in both files.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/features/apiClient/screens/apiClient/components/views/http/HttpClientView.tsx` around lines 146 - 151, Extract the duplicated METHODS_WITHOUT_BODY array into a shared export in the existing utils module (next to supportsRequestBodyForAllMethods) and replace the local declarations in both HttpClientView (where METHODS_WITHOUT_BODY is used to compute isNonBodyMethodWithBodyInNonDesktop) and HttpRequestTabs with an import of that constant; update imports and remove the local constant definitions so both files reference the single METHODS_WITHOUT_BODY symbol from utils.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@app/src/features/apiClient/screens/apiClient/components/views/http/components/HttpRequestTabs/HttpRequestTabs.tsx`:
- Around line 78-82: The memo that constructs the RequestTab for RequestTab.BODY
uses the local variable method when rendering RequestBodyRedirectScreen but
method is missing from the useMemo dependency array; update the useMemo
dependencies (the one that returns the array including the RequestTab.BODY
entry) to include method so the memo re-runs when method changes, ensuring
RequestBodyRedirectScreen receives the current method prop.
In
`@app/src/features/apiClient/screens/apiClient/components/views/http/HttpClientView.tsx`:
- Around line 626-638: The external link in the RQButton lacks the rel attribute
which is a security best practice; update the RQButton instance (the one that
calls trackDesktopAppPromoClicked and uses
href="https://requestly.com/downloads/desktop/") to include rel="noopener
noreferrer" alongside target="_blank" so the opened page cannot access
window.opener; ensure you add the rel prop to the same RQButton element that
renders the "Download Requestly" button.
---
Nitpick comments:
In
`@app/src/features/apiClient/screens/apiClient/components/views/http/HttpClientView.tsx`:
- Around line 146-151: Extract the duplicated METHODS_WITHOUT_BODY array into a
shared export in the existing utils module (next to
supportsRequestBodyForAllMethods) and replace the local declarations in both
HttpClientView (where METHODS_WITHOUT_BODY is used to compute
isNonBodyMethodWithBodyInNonDesktop) and HttpRequestTabs with an import of that
constant; update imports and remove the local constant definitions so both files
reference the single METHODS_WITHOUT_BODY symbol from utils.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/src/features/apiClient/screens/apiClient/components/clientView/components/RequestBodyRedirectScreen/index.scssapp/src/features/apiClient/screens/apiClient/components/clientView/components/RequestBodyRedirectScreen/index.tsxapp/src/features/apiClient/screens/apiClient/components/views/http/HttpClientView.tsxapp/src/features/apiClient/screens/apiClient/components/views/http/components/HttpRequestTabs/HttpRequestTabs.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- app/src/features/apiClient/screens/apiClient/components/clientView/components/RequestBodyRedirectScreen/index.tsx
- app/src/features/apiClient/screens/apiClient/components/clientView/components/RequestBodyRedirectScreen/index.scss
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
New Features
Refactor
Style