support for the http_request prop #19421
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
dannyroosevelt
moved this to Ready for PR Review
in Component (Source and Action) Backlog
WalkthroughAdds an HTTP-request prop type to the SDK, a new ControlHttpRequest React component (UI for URL, method, headers, params, and body), integrates and exports the control in connect-react, updates control styles, adds a 2.4.0 changelog entry, and bumps the connect-react package version. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
packages/connect-react/CHANGELOG.md(1 hunks)packages/connect-react/src/components/Control.tsx(2 hunks)packages/connect-react/src/components/ControlHttpRequest.tsx(1 hunks)packages/connect-react/src/hooks/customization-context.tsx(2 hunks)packages/connect-react/src/index.ts(1 hunks)packages/sdk/src/shared/component.ts(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: jverce
Repo: PipedreamHQ/pipedream PR: 18187
File: packages/connect-react/src/components/ControlSelect.tsx:138-145
Timestamp: 2025-08-27T16:47:38.177Z
Learning: In the connect-react package, changes should be non-behavioral during the SDK migration. The LoadMore functionality in ControlSelect should maintain its current runtime behavior.
Learnt from: jverce
Repo: PipedreamHQ/pipedream PR: 18187
File: packages/connect-react/src/components/Errors.tsx:16-19
Timestamp: 2025-08-27T16:48:48.776Z
Learning: In the connect-react package refactoring PR, maintain current runtime behavior even when type patterns are not ideal - prioritize behavioral consistency over type safety improvements when explicitly requested by the maintainer.
📚 Learning: 2025-08-27T16:47:38.177Z
Learnt from: jverce
Repo: PipedreamHQ/pipedream PR: 18187
File: packages/connect-react/src/components/ControlSelect.tsx:138-145
Timestamp: 2025-08-27T16:47:38.177Z
Learning: In the connect-react package, changes should be non-behavioral during the SDK migration. The LoadMore functionality in ControlSelect should maintain its current runtime behavior.
Applied to files:
packages/connect-react/src/components/Control.tsxpackages/connect-react/src/components/ControlHttpRequest.tsxpackages/connect-react/src/index.tspackages/connect-react/CHANGELOG.md
🧬 Code graph analysis (1)
packages/connect-react/src/components/Control.tsx (2)
packages/connect-react/src/components/ControlHttpRequest.tsx (1)
ControlHttpRequest(61-462)packages/connect-react/src/index.ts (1)
ControlHttpRequest(9-9)
🪛 Biome (2.1.2)
packages/connect-react/src/components/ControlHttpRequest.tsx
[error] 368-368: A form label must be associated with an input.
Consider adding a for or htmlFor attribute to the label element or moving the input element to inside the label element.
(lint/a11y/noLabelWithoutControl)
[error] 381-381: A form label must be associated with an input.
Consider adding a for or htmlFor attribute to the label element or moving the input element to inside the label element.
(lint/a11y/noLabelWithoutControl)
[error] 395-395: A form label must be associated with an input.
Consider adding a for or htmlFor attribute to the label element or moving the input element to inside the label element.
(lint/a11y/noLabelWithoutControl)
[error] 440-440: A form label must be associated with an input.
Consider adding a for or htmlFor attribute to the label element or moving the input element to inside the label element.
(lint/a11y/noLabelWithoutControl)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: pnpm publish
- GitHub Check: Lint Code Base
🔇 Additional comments (4)
packages/connect-react/CHANGELOG.md (1)
5-10: Changelog entry correctly describes the new http_request supportThe 2.4.0 notes accurately capture the addition of the
http_requestprop type and the corresponding builder UI; no further changes needed here.packages/connect-react/src/index.ts (1)
9-9: Exporting ControlHttpRequest from the barrel is correctExposing
ControlHttpRequesthere is consistent with how other controls are surfaced and matches its use via the customization context.packages/connect-react/src/components/Control.tsx (1)
12-12: Switch branch for http_request integrates cleanlyThe new import and
case "http_request": return <ControlHttpRequest />;branch plug into the existing control routing without affecting other prop types. This is the right place to wire the new control.Also applies to: 67-87
packages/connect-react/src/hooks/customization-context.tsx (1)
34-36: Customization wiring for controlHttpRequest is consistent with existing controlsThe
controlHttpRequestentry inCustomizablePropsand the newControlHttpRequestimport align with the existing customization pattern (Control* + FormFieldContext intersection), so theming/overrides should work as expected.Also applies to: 76-97
| type HttpRequestState = { | ||
| url: string; | ||
| method: string; | ||
| headers: KeyValuePair[]; | ||
| params: KeyValuePair[]; | ||
| body: string; | ||
| bodyContentType: string; | ||
| }; | ||
|
|
||
| // This matches the schema expected by the Pipedream component runtime | ||
| // See: lambda-v2/packages/component-runtime/src/prepareProps/httpRequest.js | ||
| type HttpRequestValue = { | ||
| url: string; | ||
| method: string; | ||
| headers: Array<{ name: string; value: string; disabled?: boolean }>; | ||
| params: Array<{ name: string; value: string; disabled?: boolean }>; | ||
| body: { | ||
| contentType: string; | ||
| type: "raw" | "fields"; | ||
| mode: "raw" | "fields"; | ||
| raw?: string; | ||
| fields?: Array<{ name: string; value: string }>; | ||
| }; | ||
| auth?: { | ||
| type: "none" | "basic" | "bearer"; | ||
| username?: string; | ||
| password?: string; | ||
| token?: string; | ||
| }; | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider either exposing params in the UI or removing them from state for now
HttpRequestState and HttpRequestValue both include params, and you:
- Initialize
paramsfromvalue/prop.default(initializeParams,useEffect). - Serialize
state.paramstooutput.paramsinserializeToOutput.
However, there is currently no UI for adding/editing params, so:
- Users can’t change
paramsin the form. - Existing/default
paramsare preserved and round‑tripped, but the state management is partially dead code from a UX perspective.
This is not a functional bug, but it’s a bit confusing and may mislead future maintainers into thinking params are user-editable.
Two options:
- Short term (simplest): Drop
paramsfromHttpRequestState,initializeParams, the effect, andserializeToOutputuntil you’re ready to expose a params editor. - Medium term: Add a “Query params” section mirroring the Headers section, wired to
state.paramsandserializeToOutput, so the UI fully reflects the serialized shape.
Given the comment about matching prepareProps/httpRequest.js, I’d lean toward option 2 if there’s a near‑term plan to support query params in the builder.
Also applies to: 84-127, 145-157, 164-205, 210-248
🤖 Prompt for AI Agents
packages/connect-react/src/components/ControlHttpRequest.tsx around lines 23-52
(and also affecting blocks at 84-127, 145-157, 164-205, 210-248): remove query
params from the component state and serialization paths so the UI no longer
carries dead params wiring. Specifically, delete params from HttpRequestState,
remove initializeParams and the useEffect that sets state.params from
props/defaults, and remove any serialization of state.params to output in
serializeToOutput; leave HttpRequestValue (the runtime shape) unchanged so
incoming values can be accepted if needed, but do not render or manage params in
state or UI until you implement a params editor. Ensure you also remove any
imports/types or helper functions that become unused after removing params
handling.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
packages/connect-react/src/components/ControlHttpRequest.tsx (2)
23-30: Params are still carried in state/serialization but not editable in the UI
HttpRequestStateandHttpRequestValueincludeparams, and you:
- Initialize them from
value/prop.default(initializeParams,initializeState,useEffect).- Serialize
state.paramsback out inserializeToOutput.But there is no UI for viewing or editing query params, so this wiring is effectively invisible to users.
You have two clear options:
- Short term: Drop
paramsfromHttpRequestState,initializeParams, theuseEffectsync, andserializeToOutputuntil you’re ready to expose a params editor, keeping onlyHttpRequestValue.paramsin the type so incoming values are accepted.- Medium term: Add a “Query params” section mirroring the Headers section, wired to
state.paramsandserializeToOutput, so the UI fully reflects what’s being emitted.Right now the behavior isn’t wrong, but it is misleading for maintainers and makes the component look more capable than the UI actually is.
Also applies to: 118-139, 161-168, 194-205, 222-237, 255-255
352-363: Fix label–control associations to satisfy Biome and improve a11yBiome’s
a11y/noLabelWithoutControlerrors for the URL, Headers, and Body labels are valid: all three<label>elements lack an associated control.You can fix this with minimal behavioral impact:
- URL label → associate with the URL input
Add stable IDs and link the label to the input:
@@ 73-80 export function ControlHttpRequest() { const formFieldContextProps = useFormFieldContext(); const { onChange, prop, value, } = formFieldContextProps; @@ const { getProps, theme, } = useCustomize(); + + const urlInputId = `${prop.name}-url`; + const bodyContentTypeSelectId = `${prop.name}-body-content-type`; @@ 400-421 - <div style={sectionStyles}> - <label style={labelStyles}>URL</label> + <div style={sectionStyles}> + <label style={labelStyles} htmlFor={urlInputId}>URL</label> <div style={urlRowStyles}> <select value={state.method} onChange={(e) => handleMethodChange(e.target.value)} style={methodSelectStyles} > @@ </select> <input type="text" + id={urlInputId} value={state.url} onChange={(e) => handleUrlChange(e.target.value)} placeholder="https://api.example.com/endpoint" style={urlInputStyles} required={!prop.optional} />(Optionally give the method
<select>anaria-label="HTTP method"if you want explicit labeling there as well.)
- Headers label → make it a non-label section heading
Since “Headers” describes a group of inputs rather than a single control, use a
<div>or<span>instead of<label>:- <div style={sectionStyles}> - <label style={labelStyles}>Headers</label> + <div style={sectionStyles}> + <div style={labelStyles}>Headers</div>
- Body label → associate with the content-type select
Tie the “Body” label to the select:
- <div style={sectionStyles}> - <label style={labelStyles}>Body</label> - <select + <div style={sectionStyles}> + <label style={labelStyles} htmlFor={bodyContentTypeSelectId}>Body</label> + <select + id={bodyContentTypeSelectId} value={state.bodyContentType} onChange={(e) => handleBodyContentTypeChange(e.target.value)} style={selectStyles} >This should clear the Biome errors and make the control significantly more accessible to screen readers.
Also applies to: 398-421, 424-467, 470-492
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
packages/connect-react/package.json(1 hunks)packages/connect-react/src/components/ControlHttpRequest.tsx(1 hunks)packages/connect-react/src/styles/control-styles.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-27T16:47:38.177Z
Learnt from: jverce
Repo: PipedreamHQ/pipedream PR: 18187
File: packages/connect-react/src/components/ControlSelect.tsx:138-145
Timestamp: 2025-08-27T16:47:38.177Z
Learning: In the connect-react package, changes should be non-behavioral during the SDK migration. The LoadMore functionality in ControlSelect should maintain its current runtime behavior.
Applied to files:
packages/connect-react/src/components/ControlHttpRequest.tsxpackages/connect-react/src/styles/control-styles.ts
📚 Learning: 2025-08-27T17:25:10.425Z
Learnt from: jverce
Repo: PipedreamHQ/pipedream PR: 18187
File: packages/connect-react/src/utils/type-guards.ts:23-33
Timestamp: 2025-08-27T17:25:10.425Z
Learning: In the connect-react package, the isOptionWithLabel type guard intentionally restricts value types to string|number for runtime filtering purposes, even though LabelValueOption<T> allows any T. This runtime behavior should be preserved over type safety improvements.
Applied to files:
packages/connect-react/src/components/ControlHttpRequest.tsx
🪛 Biome (2.1.2)
packages/connect-react/src/components/ControlHttpRequest.tsx
[error] 402-402: A form label must be associated with an input.
Consider adding a for or htmlFor attribute to the label element or moving the input element to inside the label element.
(lint/a11y/noLabelWithoutControl)
[error] 426-426: A form label must be associated with an input.
Consider adding a for or htmlFor attribute to the label element or moving the input element to inside the label element.
(lint/a11y/noLabelWithoutControl)
[error] 471-471: A form label must be associated with an input.
Consider adding a for or htmlFor attribute to the label element or moving the input element to inside the label element.
(lint/a11y/noLabelWithoutControl)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: pnpm publish
- GitHub Check: Lint Code Base
🔇 Additional comments (2)
packages/connect-react/package.json (1)
3-3: Version bump to 2.4.0 is appropriateMinor version bump matches the addition of a new public ControlHttpRequest component and related exports; no issues here.
packages/connect-react/src/styles/control-styles.ts (1)
4-30: Input/button background updates look consistent with themingUsing
theme.colors.neutral20for inputs and a transparent background for buttons aligns them better with the themed surfaces and borders. Just confirm in the UI that:
- Input text still has sufficient contrast on
neutral20.- Transparent buttons remain visually distinguishable against all container backgrounds you support.
Otherwise, this looks good.
| type HttpRequestState = { | ||
| url: string; | ||
| method: string; | ||
| headers: KeyValuePair[]; | ||
| params: KeyValuePair[]; | ||
| body: string; | ||
| bodyContentType: string; | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider consolidating state initialization logic to avoid drift
initializeHeaders, initializeParams, initializeBody, initializeBodyContentType, and initializeState all reconstruct state from value + prop.default, and very similar logic is repeated again inside the useEffect reset.
To reduce the chance of future drift between “initial mount” and “value resync”, you could centralize this into a single buildStateFromValue(value?: HttpRequestValue) helper and use it both:
- In the
useStateinitializer. - Inside the
useEffectwhenvaluechanges.
This keeps the merge semantics (value over defaults over hard-coded fallbacks) in one place.
Also applies to: 82-139, 156-169
🤖 Prompt for AI Agents
In packages/connect-react/src/components/ControlHttpRequest.tsx around lines
23-30 (and also covering the repeated logic at ~82-139 and ~156-169), multiple
functions duplicate the same merge/initialization logic for headers, params,
body, bodyContentType and overall state; create a single
buildStateFromValue(value?: HttpRequestValue) helper that returns the complete
HttpRequestState by applying the merge semantics (value fields override
prop.default values which fallback to hard-coded defaults), then use that helper
both as the initial value for useState and inside the useEffect that resets
state when value changes, removing the individual initialize* helpers and
duplicated code so all construction logic lives in one place.
| type HttpRequestValue = { | ||
| url: string; | ||
| method: string; | ||
| headers: Array<{ name: string; value: string; disabled?: boolean }>; | ||
| params: Array<{ name: string; value: string; disabled?: boolean }>; | ||
| body: { | ||
| contentType: string; | ||
| type: "raw" | "fields"; | ||
| mode: "raw" | "fields"; | ||
| raw?: string; | ||
| fields?: Array<{ name: string; value: string }>; | ||
| }; | ||
| auth?: { | ||
| type: "none" | "basic" | "bearer"; | ||
| username?: string; | ||
| password?: string; | ||
| token?: string; | ||
| }; | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Clarify handling of auth in the emitted HttpRequestValue
HttpRequestValue includes an optional auth block, but:
- You never read
authfrom the incomingvalue. - You don’t include
authwhen building theoutputinserializeToOutput.
Depending on how the parent form merges values, this may mean that any existing auth configuration is dropped as soon as the user edits URL/headers/body via this control.
If the intention is:
- To preserve
auth: Consider threadingauththrough state (or at least preserving it from the currentvaluewhen constructingoutput). - To ignore
authentirely here: Consider removingauthfromHttpRequestValuein this component (or documenting thatauthis managed elsewhere) so the local type doesn’t suggest behavior you don’t actually implement.
This is mainly about avoiding surprising loss of configuration.
Also applies to: 219-260
🤖 Prompt for AI Agents
In packages/connect-react/src/components/ControlHttpRequest.tsx around lines
34-52 and 219-260, the HttpRequestValue type declares an optional auth block but
the component neither reads auth from the incoming value nor includes auth when
constructing the serialized output, which can silently drop existing auth
config; fix by preserving auth from the incoming prop value when building output
(or thread auth into local state if the control should edit it) — specifically,
read value.auth at initialization/when serializing and merge it into the output
object you emit; alternatively, if auth is intentionally out of scope for this
control, remove auth from the local HttpRequestValue type and document that auth
is managed elsewhere to avoid misleading the type.
| // Update state when external value changes | ||
| useEffect(() => { | ||
| const currentValue = value as HttpRequestValue | undefined; | ||
| const defaultConfig = getDefaultConfig(); | ||
|
|
||
| const newHeaders = currentValue?.headers ?? defaultConfig?.headers ?? []; | ||
| const headers: KeyValuePair[] = newHeaders.length > 0 | ||
| ? newHeaders.map((h) => ({ | ||
| key: h.name ?? "", | ||
| value: h.value ?? "", | ||
| })) | ||
| : [ | ||
| { | ||
| key: "", | ||
| value: "", | ||
| }, | ||
| ]; | ||
|
|
||
| const newParams = currentValue?.params ?? defaultConfig?.params ?? []; | ||
| const params: KeyValuePair[] = newParams.length > 0 | ||
| ? newParams.map((p) => ({ | ||
| key: p.name ?? "", | ||
| value: p.value ?? "", | ||
| })) | ||
| : [ | ||
| { | ||
| key: "", | ||
| value: "", | ||
| }, | ||
| ]; | ||
|
|
||
| setState({ | ||
| url: currentValue?.url ?? defaultConfig?.url ?? "", | ||
| method: currentValue?.method ?? defaultConfig?.method ?? "GET", | ||
| headers, | ||
| params, | ||
| body: currentValue?.body?.raw ?? defaultConfig?.body?.raw ?? "", | ||
| bodyContentType: currentValue?.body?.contentType ?? defaultConfig?.body?.contentType ?? "application/json", | ||
| }); | ||
| }, [ | ||
| value, | ||
| ]); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Confirm dependency assumptions for the useEffect resync
The resync effect depends only on value, but its logic also indirectly depends on prop.default via getDefaultConfig():
useEffect(() => {
const currentValue = value as HttpRequestValue | undefined;
const defaultConfig = getDefaultConfig();
// ...
}, [value]);If prop (or its .default) is effectively immutable over the lifetime of this component, this is fine. If there’s any chance that defaults can change while mounted, state will not re-sync to those new defaults.
Two options:
- Add
prop(or a stable derivation likeprop.default) to the dependency array, or - Add a brief comment noting that
propis stable so future refactors don’t accidentally rely on mutable defaults.
Given how important the default merge behavior is here, I’d at least document the assumption.
🤖 Prompt for AI Agents
packages/connect-react/src/components/ControlHttpRequest.tsx around lines 176 to
217: the useEffect resync currently only lists `value` in its dependency array
but also reads `getDefaultConfig()` (which depends on `prop`/`prop.default`), so
state won't resync if defaults change; either include the stable dependency
(e.g., `prop.default` or `getDefaultConfig()` result via memo) in the dependency
array to ensure correct resync, or add a short inline comment documenting that
`prop`/`prop.default` is guaranteed immutable for the component lifetime and
therefore intentionally omitted from deps; implement one of these fixes and
update the dependency array or add the explanatory comment accordingly.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/connect-react/src/components/ControlHttpRequest.tsx (1)
219-260: Preserveauthwhen serializing to avoid silently dropping authentication configRight now
HttpRequestValueincludesauth, butserializeToOutputnever carries it through. Any existing/defaultauthwill be lost as soon as the user edits URL/headers/body andonChangeis called, which is a behavioral bug if component authors rely on default or previously-saved auth.You can preserve
authwithout changing the UI by merging it from the currentvalueorprop.defaultwhen building the output:- const serializeToOutput = (currentState: HttpRequestState): HttpRequestValue => { - // Filter out empty headers but keep the array structure + const serializeToOutput = (currentState: HttpRequestState): HttpRequestValue => { + const currentValue = value as HttpRequestValue | undefined; + const defaultConfig = getDefaultConfig(); + const auth = currentValue?.auth ?? defaultConfig?.auth; + + // Filter out empty headers but keep the array structure const validHeaders = currentState.headers .filter((h) => h.key.trim() !== "") .map((h) => ({ name: h.key, value: h.value, })); @@ - const output: HttpRequestValue = { - url: currentState.url, - method: currentState.method, - headers: validHeaders, // Always an array, even if empty - params: validParams, // Always an array, even if empty - body, - }; + const output: HttpRequestValue = { + url: currentState.url, + method: currentState.method, + headers: validHeaders, // Always an array, even if empty + params: validParams, // Always an array, even if empty + body, + ...(auth ? { auth } : {}), + };This keeps existing/default auth intact whenever the user edits other aspects of the request.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
packages/connect-react/src/components/ControlHttpRequest.tsx(1 hunks)packages/sdk/src/shared/component.ts(3 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: jverce
Repo: PipedreamHQ/pipedream PR: 18187
File: packages/connect-react/src/components/ControlSelect.tsx:138-145
Timestamp: 2025-08-27T16:47:38.177Z
Learning: In the connect-react package, changes should be non-behavioral during the SDK migration. The LoadMore functionality in ControlSelect should maintain its current runtime behavior.
📚 Learning: 2025-08-27T16:48:48.776Z
Learnt from: jverce
Repo: PipedreamHQ/pipedream PR: 18187
File: packages/connect-react/src/components/Errors.tsx:16-19
Timestamp: 2025-08-27T16:48:48.776Z
Learning: In the connect-react package refactoring PR, maintain current runtime behavior even when type patterns are not ideal - prioritize behavioral consistency over type safety improvements when explicitly requested by the maintainer.
Applied to files:
packages/sdk/src/shared/component.tspackages/connect-react/src/components/ControlHttpRequest.tsx
📚 Learning: 2025-08-27T16:47:38.177Z
Learnt from: jverce
Repo: PipedreamHQ/pipedream PR: 18187
File: packages/connect-react/src/components/ControlSelect.tsx:138-145
Timestamp: 2025-08-27T16:47:38.177Z
Learning: In the connect-react package, changes should be non-behavioral during the SDK migration. The LoadMore functionality in ControlSelect should maintain its current runtime behavior.
Applied to files:
packages/connect-react/src/components/ControlHttpRequest.tsx
📚 Learning: 2025-08-27T17:25:10.425Z
Learnt from: jverce
Repo: PipedreamHQ/pipedream PR: 18187
File: packages/connect-react/src/utils/type-guards.ts:23-33
Timestamp: 2025-08-27T17:25:10.425Z
Learning: In the connect-react package, the isOptionWithLabel type guard intentionally restricts value types to string|number for runtime filtering purposes, even though LabelValueOption<T> allows any T. This runtime behavior should be preserved over type safety improvements.
Applied to files:
packages/connect-react/src/components/ControlHttpRequest.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: pnpm publish
- GitHub Check: Lint Code Base
🔇 Additional comments (1)
packages/sdk/src/shared/component.ts (1)
151-177: HttpRequestValue and http_request prop wiring look consistent and completeThe new
HttpRequestValuetype,ConfigurablePropHttpRequest, and the updates toConfigurableProp/PropValue<"http_request">form a coherent contract and align with the described runtime schema. The mapping ensureshttp_requestprops are first-class citizens in the SDK without obvious gaps.Also applies to: 208-223, 233-253
| // This matches the schema expected by the Pipedream component runtime | ||
| // See: lambda-v2/packages/component-runtime/src/prepareProps/httpRequest.js | ||
| type HttpRequestValue = { | ||
| url: string; | ||
| method: string; | ||
| headers: Array<{ name: string; value: string; disabled?: boolean }>; | ||
| params: Array<{ name: string; value: string; disabled?: boolean }>; | ||
| body: { | ||
| contentType: string; | ||
| type: "raw" | "fields"; | ||
| mode: "raw" | "fields"; | ||
| raw?: string; | ||
| fields?: Array<{ name: string; value: string }>; | ||
| }; | ||
| auth?: { | ||
| type: "none" | "basic" | "bearer"; | ||
| username?: string; | ||
| password?: string; | ||
| token?: string; | ||
| }; | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Avoid duplicating the HttpRequestValue runtime schema in multiple places
This local HttpRequestValue type mirrors the schema already defined in the SDK. Keeping two near-identical definitions (here and in packages/sdk/src/shared/component.ts) increases the risk of subtle drift if the runtime shape evolves.
If the package dependency graph allows it, consider importing and reusing the shared HttpRequestValue type from the SDK instead of redefining it here, so there’s a single source of truth for the http_request payload shape.
🤖 Prompt for AI Agents
In packages/connect-react/src/components/ControlHttpRequest.tsx around lines 32
to 52, the local HttpRequestValue type duplicates the runtime schema already
defined in packages/sdk/src/shared/component.ts; replace the local duplicate by
importing and reusing the shared HttpRequestValue type from the SDK package
(update imports and remove the local type declaration), ensure TypeScript
paths/dependencies are adjusted if necessary so the SDK type is resolvable, and
run type checks to confirm no other references need small adapter changes.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
packages/connect-react/src/components/ControlHttpRequest.tsx (4)
450-475: Body section wiring and accessibility look solidThe
label/htmlForpairing for the body type select plus thearia-labelon the textarea resolve the earlier noLabelWithoutControl/a11y concerns while keeping the UI straightforward. The conditional render forstate.bodyContentType !== "none"also cleanly avoids an unnecessary textarea.
35-55: AlignHttpRequestValuetype andauthhandling with actual behaviorThis local
HttpRequestValueduplicates the runtime schema from the SDK and includes anauthblock that the component never reads or emits inserializeToOutput. That can mislead consumers into thinkingauthis preserved when any edit via this control will drop it.I’d either:
- Import and reuse the shared HttpRequestValue type from the SDK and explicitly thread
auththrough state/serialization if you intend to preserve it, or- If
authis intentionally out of scope for this control, removeauthfrom this local type (or clearly document that it isn’t handled here) so the types match the actual behavior.Based on learnings, keeping a single, behavior-accurate schema helps avoid subtle runtime drift.
117-202: Consolidate state initialization/resync logic and clarifyuseEffectdependencies
initializeHeaders/Params/Body/BodyContentType,initializeState, and theuseEffectreset all reimplement the same “valueoverprop.defaultover hard-coded fallback” merge. This duplication makes it easy for mount vs. resync behavior to diverge.Consider extracting a single helper like
buildStateFromSources(currentValue: HttpRequestValue | undefined)(internally callinggetDefaultConfig) and using it both:
- As the
useStateinitializer, and- Inside the
useEffectwhenvaluechanges.Also, the effect depends only on
valuebut indirectly onprop.defaultviagetDefaultConfig(). Ifprop/prop.defaultare guaranteed immutable, a short comment here would prevent future refactors from assuming changing defaults will resync state; otherwise, consider adding a stable derivation (e.g.,prop.default) to the deps.Based on learnings, centralizing this merge logic reduces the risk of subtle behavioral differences between initial render and later updates.
24-31:paramsare preserved but not user-editable—consider documenting this
HttpRequestStateandserializeToOutputboth handleparams, but there’s no UI wired tostate.params, so:
- Existing/default params are normalized into state and round-tripped back out, preserving them at runtime.
- Users can’t actually see or modify them in this control.
Functionally this is okay (and preferable to silently dropping params), but it can confuse future maintainers into thinking query params are editable here.
I’d either:
- Add a short comment near
HttpRequestState.paramsorserializeToOutputexplaining that params are currently pass-through only, or- If a params editor is planned soon, treat this as an intentional interim and track it explicitly.
Based on learnings, preserving existing runtime behavior while clearly documenting intentional gaps is preferable to removing fields from serialization.
Also applies to: 210-231
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
packages/connect-react/src/components/ControlHttpRequest.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: jverce
Repo: PipedreamHQ/pipedream PR: 18187
File: packages/connect-react/src/components/ControlSelect.tsx:138-145
Timestamp: 2025-08-27T16:47:38.177Z
Learning: In the connect-react package, changes should be non-behavioral during the SDK migration. The LoadMore functionality in ControlSelect should maintain its current runtime behavior.
📚 Learning: 2025-08-27T16:47:38.177Z
Learnt from: jverce
Repo: PipedreamHQ/pipedream PR: 18187
File: packages/connect-react/src/components/ControlSelect.tsx:138-145
Timestamp: 2025-08-27T16:47:38.177Z
Learning: In the connect-react package, changes should be non-behavioral during the SDK migration. The LoadMore functionality in ControlSelect should maintain its current runtime behavior.
Applied to files:
packages/connect-react/src/components/ControlHttpRequest.tsx
📚 Learning: 2025-08-27T17:25:10.425Z
Learnt from: jverce
Repo: PipedreamHQ/pipedream PR: 18187
File: packages/connect-react/src/utils/type-guards.ts:23-33
Timestamp: 2025-08-27T17:25:10.425Z
Learning: In the connect-react package, the isOptionWithLabel type guard intentionally restricts value types to string|number for runtime filtering purposes, even though LabelValueOption<T> allows any T. This runtime behavior should be preserved over type safety improvements.
Applied to files:
packages/connect-react/src/components/ControlHttpRequest.tsx
📚 Learning: 2025-08-27T16:48:48.776Z
Learnt from: jverce
Repo: PipedreamHQ/pipedream PR: 18187
File: packages/connect-react/src/components/Errors.tsx:16-19
Timestamp: 2025-08-27T16:48:48.776Z
Learning: In the connect-react package refactoring PR, maintain current runtime behavior even when type patterns are not ideal - prioritize behavioral consistency over type safety improvements when explicitly requested by the maintainer.
Applied to files:
packages/connect-react/src/components/ControlHttpRequest.tsx
📚 Learning: 2024-10-31T23:57:17.282Z
Learnt from: jverce
Repo: PipedreamHQ/pipedream PR: 14106
File: packages/sdk/src/server/__tests__/server.test.ts:174-192
Timestamp: 2024-10-31T23:57:17.282Z
Learning: In `packages/sdk/src/server/__tests__/server.test.ts`, when `ts-ignore` annotations are used in mock implementations, they are acceptable, and replacing them with type casting or mock types is unnecessary.
Applied to files:
packages/connect-react/src/components/ControlHttpRequest.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Lint Code Base
| const addHeader = () => { | ||
| const newHeaders = [ | ||
| ...state.headers, | ||
| { | ||
| key: "", | ||
| value: "", | ||
| }, | ||
| ]; | ||
| setState({ | ||
| ...state, | ||
| headers: newHeaders, | ||
| }); | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider firing onChange when adding a header row for consistency
addHeader mutates local state.headers but doesn’t call onChange, whereas edit/remove do both setState and onChange(serializeToOutput(...)).
Right now, an added empty header serializes to no-op (since empty keys are filtered), so this doesn’t change the emitted HttpRequestValue. Still, for consistency and to avoid surprises if upstream ever relies on change events, it may be worth also calling:
const newState = { ...state, headers: newHeaders };
setState(newState);
onChange(serializeToOutput(newState));🤖 Prompt for AI Agents
In packages/connect-react/src/components/ControlHttpRequest.tsx around lines 271
to 283, addHeader currently updates local state.headers via setState but does
not invoke onChange like the edit/remove handlers do; update addHeader to create
newState = { ...state, headers: newHeaders }, call setState(newState) and then
call onChange(serializeToOutput(newState)) so the component emits the same
change event when a header row is added.
github-project-automation
bot
moved this to Done
in Component (Source and Action) Backlog
WHY
Summary by CodeRabbit
New Features
Style
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.