Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
WalkthroughAdds first‑class data: URL handling for file streaming, tightens Axios/OAuth1 typings and request signing/error paths, updates utility function types, bumps platform package version and TypeScript range, enables Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant CreateWrapper as Create
participant CallAxios as callAxios
participant OAuthSigner as getOauthSignature
participant AxiosLib as axios
participant Exporter as stepExport
Client->>CreateWrapper: invoke create(config, signConfig?)
CreateWrapper->>CallAxios: execute request (step may be undefined)
alt signConfig present
CallAxios->>OAuthSigner: transform config and request signature
OAuthSigner-->>CallAxios: returns signature
CallAxios->>AxiosLib: send signed request
else no signing
CallAxios->>AxiosLib: send request
end
AxiosLib-->>CallAxios: response / error
CallAxios->>Exporter: on debug/error call stepExport(step | undefined, message, key)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
platform/package.json (1)
3-31: Regenerate lockfile to match updated package.json specifiers.The pnpm lockfile contains outdated specifiers that don't match
platform/package.json. Most critically, TypeScript is pinned to v3.9.10 (^3.5.3) when the package now requires ^4.9.5, and axios is pinned to 1.13.2 instead of the requested ^1.12.0. Runpnpm installat the repository root to regenerate the lockfile and resolve the mismatch.
🤖 Fix all issues with AI agents
In `@platform/lib/axios.ts`:
- Line 1: Replace non-idiomatic casts to AxiosHeaders by creating or mutating an
actual AxiosHeaders instance: where the code does config.headers = {} as
AxiosHeaders, construct a new AxiosHeaders with the desired header (e.g., new
AxiosHeaders({ Authorization: oauthSignature })) or, if inside an interceptor
and config.headers is already an AxiosHeaders instance, call
config.headers.set("Authorization", oauthSignature). Update all occurrences
(including the similar pattern around the config.headers use at the
config.headers lines referenced) so you no longer cast plain objects to
AxiosHeaders but instead use the AxiosHeaders constructor or its .set() method.
In `@platform/lib/file-stream.ts`:
- Around line 74-116: The data URL parsing in parseDataUrl is too strict: update
parseDataUrl to accept full media-type with optional parameters (e.g.,
"text/plain;charset=UTF-8") and to detect a "base64" parameter anywhere in the
parameter list instead of only allowing ";base64" directly; if the mediatype
portion is empty (e.g., "data:,...") return the default
"text/plain;charset=US-ASCII"; return the mediaType string including its
parameters (excluding the "base64" token) and an isBase64 boolean; ensure
getDataUrlStream and getDataUrlStreamAndMetadata use parseDataUrl's mediaType
and isBase64 (decode percent-encoding only for non-base64) so metadata
(contentType and size and filename extension via mime.extension) remains
correct.
| function parseDataUrl(dataUrl: string): ParsedDataUrl { | ||
| // Format: data:[<mediatype>][;base64],<data> | ||
| const match = dataUrl.match(/^data:([^;,]*)?(?:;(base64))?,(.*)$/); | ||
| if (!match) { | ||
| throw new Error("Invalid data URL format"); | ||
| } | ||
| const [ | ||
| , | ||
| mediaType = "text/plain;charset=US-ASCII", | ||
| base64Flag, | ||
| data, | ||
| ] = match; | ||
| return { | ||
| mediaType, | ||
| isBase64: base64Flag === "base64", | ||
| data, | ||
| }; | ||
| } | ||
|
|
||
| function getDataUrlStream(dataUrl: string): Readable { | ||
| const parsed = parseDataUrl(dataUrl); | ||
| const buffer = parsed.isBase64 | ||
| ? Buffer.from(parsed.data, "base64") | ||
| : Buffer.from(decodeURIComponent(parsed.data), "utf-8"); | ||
| return Readable.from(buffer); | ||
| } | ||
|
|
||
| function getDataUrlStreamAndMetadata(dataUrl: string): { stream: Readable; metadata: FileMetadata } { | ||
| const parsed = parseDataUrl(dataUrl); | ||
| const buffer = parsed.isBase64 | ||
| ? Buffer.from(parsed.data, "base64") | ||
| : Buffer.from(decodeURIComponent(parsed.data), "utf-8"); | ||
|
|
||
| const ext = mime.extension(parsed.mediaType); | ||
| const name = ext | ||
| ? `file.${ext}` | ||
| : "file"; | ||
|
|
||
| const metadata: FileMetadata = { | ||
| size: buffer.length, | ||
| contentType: parsed.mediaType || undefined, | ||
| name, | ||
| }; |
There was a problem hiding this comment.
Data URL parser rejects valid media-type parameters and loses default type.
The current regex only allows ;base64 and fails for valid URLs like data:text/plain;charset=UTF-8,hello, and it treats data:;base64,... as an empty media type instead of defaulting to text/plain;charset=US-ASCII. That breaks valid data URLs and metadata extraction.
🐛 Proposed fix (more spec‑compliant parsing)
function parseDataUrl(dataUrl: string): ParsedDataUrl {
// Format: data:[<mediatype>][;base64],<data>
- const match = dataUrl.match(/^data:([^;,]*)?(?:;(base64))?,(.*)$/);
- if (!match) {
- throw new Error("Invalid data URL format");
- }
- const [
- ,
- mediaType = "text/plain;charset=US-ASCII",
- base64Flag,
- data,
- ] = match;
- return {
- mediaType,
- isBase64: base64Flag === "base64",
- data,
- };
+ if (!dataUrl.startsWith("data:")) {
+ throw new Error("Invalid data URL format");
+ }
+ const commaIndex = dataUrl.indexOf(",");
+ if (commaIndex === -1) {
+ throw new Error("Invalid data URL format");
+ }
+ const meta = dataUrl.slice(5, commaIndex); // after "data:"
+ const data = dataUrl.slice(commaIndex + 1);
+ const isBase64 = /;base64$/i.test(meta);
+ const mediaTypeRaw = isBase64 ? meta.replace(/;base64$/i, "") : meta;
+ const mediaType = mediaTypeRaw ? mediaTypeRaw : "text/plain;charset=US-ASCII";
+ return {
+ mediaType,
+ isBase64,
+ data,
+ };
}🤖 Prompt for AI Agents
In `@platform/lib/file-stream.ts` around lines 74 - 116, The data URL parsing in
parseDataUrl is too strict: update parseDataUrl to accept full media-type with
optional parameters (e.g., "text/plain;charset=UTF-8") and to detect a "base64"
parameter anywhere in the parameter list instead of only allowing ";base64"
directly; if the mediatype portion is empty (e.g., "data:,...") return the
default "text/plain;charset=US-ASCII"; return the mediaType string including its
parameters (excluding the "base64" token) and an isBase64 boolean; ensure
getDataUrlStream and getDataUrlStreamAndMetadata use parseDataUrl's mediaType
and isBase64 (decode percent-encoding only for non-base64) so metadata
(contentType and size and filename extension via mime.extension) remains
correct.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@platform/lib/axios.ts`:
- Line 1: The code currently assigns the Authorization header directly
(bypassing Axios header normalization); change those assignments to use
AxiosHeaders.set() instead: import/keep AxiosHeaders (already imported) and in
the request-building code or request interceptor where you currently do
something like headers.Authorization = `Bearer ${token}` or
config.headers['Authorization'] = value, replace it with new
AxiosHeaders(config.headers).set('Authorization', value) or call
AxiosHeaders.set on the existing AxiosHeaders instance (e.g., const headers =
new AxiosHeaders(config.headers); headers.set('Authorization', `Bearer
${token}`); config.headers = headers). Update all occurrences (including the
spots around the current Authorization assignments referenced) to use
AxiosHeaders.set for proper normalization and merging.
In `@platform/package.json`:
- Line 3: Add ts-jest to platform/package.json's devDependencies (e.g.,
"ts-jest": "29.1.2") so the platform package declares the Jest TypeScript
transformer explicitly; update the "devDependencies" object in
platform/package.json to include that entry and run npm/yarn install. Also
address the TypeScript version mismatch between platform's "typescript":
"^4.9.5" and the root's ">=5.5.0 <5.7.0" by either aligning platform's
"typescript" to the root range or adding a comment/README note in the platform
package explaining the intentional divergence.
platform/lib/axios.ts
Outdated
| @@ -1,4 +1,4 @@ | |||
| import axios from "axios"; | |||
| import axios, { AxiosHeaders } from "axios"; | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "axios.ts" | head -20Repository: PipedreamHQ/pipedream
Length of output: 88
🏁 Script executed:
cat platform/lib/axios.ts | head -50Repository: PipedreamHQ/pipedream
Length of output: 1650
🏁 Script executed:
sed -n '160,175p' platform/lib/axios.tsRepository: PipedreamHQ/pipedream
Length of output: 565
🌐 Web query:
AxiosHeaders set method axios 1.x documentation
💡 Result:
In Axios 1.x, AxiosHeaders#set is documented as the primary way to set/merge headers on an AxiosHeaders instance. (axios.rest)
Signature (overloads)
set(headerName, value, rewrite?: boolean);
set(headerName, value, rewrite?: (this: AxiosHeaders, value: string, name: string, headers: RawAxiosHeaders) => boolean);
set(headers?: RawAxiosHeaders | AxiosHeaders | string, rewrite?: boolean);rewrite behavior
false: don’t overwrite if the header already has a value (notundefined)undefined(default): overwrite unless the existing value isfalsetrue: always overwrite- can also be a function to decide per header/value whether to overwrite (github.com)
Notes
- Accepts a single header, an object of headers, another
AxiosHeaders, or a raw header string (newline-separated). - Returns
thisfor chaining. (axios.rest)
Citations:
- 1: https://axios.rest/pages/advanced/header-methods?utm_source=openai
- 2: https://axios.rest/pages/advanced/header-methods?utm_source=openai
- 3: https://github.com/axios/axios?utm_source=openai
- 4: https://axios.rest/pages/advanced/header-methods?utm_source=openai
Use AxiosHeaders.set() for Authorization header
In Axios 1.x, AxiosHeaders.set() is the documented primary way to set headers and properly handles header normalization and merge semantics. Direct property assignment bypasses this logic.
♻️ Proposed change
- if (!config.headers) config.headers = new AxiosHeaders();
- config.headers.Authorization = oauthSignature;
+ if (!config.headers) config.headers = new AxiosHeaders();
+ config.headers.set("Authorization", oauthSignature);Also applies to: 165-168
🤖 Prompt for AI Agents
In `@platform/lib/axios.ts` at line 1, The code currently assigns the
Authorization header directly (bypassing Axios header normalization); change
those assignments to use AxiosHeaders.set() instead: import/keep AxiosHeaders
(already imported) and in the request-building code or request interceptor where
you currently do something like headers.Authorization = `Bearer ${token}` or
config.headers['Authorization'] = value, replace it with new
AxiosHeaders(config.headers).set('Authorization', value) or call
AxiosHeaders.set on the existing AxiosHeaders instance (e.g., const headers =
new AxiosHeaders(config.headers); headers.set('Authorization', `Bearer
${token}`); config.headers = headers). Update all occurrences (including the
spots around the current Authorization assignments referenced) to use
AxiosHeaders.set for proper normalization and merging.
| { | ||
| "name": "@pipedream/platform", | ||
| "version": "3.1.1", | ||
| "version": "3.2.0", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n platform/package.jsonRepository: PipedreamHQ/pipedream
Length of output: 1128
🏁 Script executed:
find platform -name "jest.config.*" -o -name "jest.setup.*" | head -20Repository: PipedreamHQ/pipedream
Length of output: 47
🏁 Script executed:
rg "ts-jest" platform/ 2>/dev/null || echo "No ts-jest references found"Repository: PipedreamHQ/pipedream
Length of output: 92
🏁 Script executed:
rg "preset" platform/ --type json 2>/dev/null || echo "No preset configuration found"Repository: PipedreamHQ/pipedream
Length of output: 94
🏁 Script executed:
ls -la | grep -E "jest|test"Repository: PipedreamHQ/pipedream
Length of output: 134
🏁 Script executed:
find platform -type f \( -name "*.test.ts" -o -name "*.spec.ts" -o -name "*.test.js" -o -name "*.spec.js" \) | head -20Repository: PipedreamHQ/pipedream
Length of output: 47
🏁 Script executed:
cat jest.config.js 2>/dev/null || cat jest.config.json 2>/dev/null || echo "No root jest config found"Repository: PipedreamHQ/pipedream
Length of output: 556
🏁 Script executed:
head -60 package.jsonRepository: PipedreamHQ/pipedream
Length of output: 2235
🌐 Web query:
ts-jest 29.1.2 TypeScript 4.9 compatibility support
💡 Result:
ts-jest 29.1.2 (released Jan 22, 2024) should be compatible with TypeScript 4.9, because ts-jest 29.x declares a TypeScript peer dependency of >=4.3 <6, which includes 4.9.x. [1][2]
Sources
[1] https://app.unpkg.com/ts-jest%4029.2.2/files/package.json
[2] https://github.com/kulshekhar/ts-jest/tags?after=v29.1.5
🏁 Script executed:
cat package.json | grep -A 100 '"devDependencies"' | grep -E "ts-jest|typescript" | head -10Repository: PipedreamHQ/pipedream
Length of output: 246
Ensure ts-jest is listed in platform/package.json devDependencies
TypeScript 4.9.5 is compatible with ts-jest 29.1.2 (ts-jest 29.x supports TypeScript >=4.3 <6). However, ts-jest is missing from platform's devDependencies—it's currently only in the root package.json. Add it to platform to ensure explicit dependency declaration and consistency in the monorepo structure.
Also note the TypeScript version mismatch: root requires >=5.5.0 <5.7.0 while platform specifies ^4.9.5. Align these versions or document the intentional difference.
🤖 Prompt for AI Agents
In `@platform/package.json` at line 3, Add ts-jest to platform/package.json's
devDependencies (e.g., "ts-jest": "29.1.2") so the platform package declares the
Jest TypeScript transformer explicitly; update the "devDependencies" object in
platform/package.json to include that entry and run npm/yarn install. Also
address the TypeScript version mismatch between platform's "typescript":
"^4.9.5" and the root's ">=5.5.0 <5.7.0" by either aligning platform's
"typescript" to the root range or adding a comment/README note in the platform
package explaining the intentional divergence.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
platform/lib/axios.ts (1)
71-94: Use AxiosHeaders API for Content-Type detection to avoid OAuth signature mismatches.When
transformConfigForOauth()is called within the request interceptor,config.headersis anAxiosHeadersinstance. Usingfor...into enumerate its properties is unreliable sinceAxiosHeadersis a class with non-enumerable properties, not a plain object. This can cause Content-Type detection to fail silently, producing an invalid OAuth signature.Suggested fix
- for (const k in config.headers || {}) { - if (/content-type/i.test(k)) { - hasContentType = true; - formEncodedContentType = config.headers?.[k] === "application/x-www-form-urlencoded"; - break; - } - } + const headers = AxiosHeaders.from(config.headers); + const contentType = headers?.get?.("Content-Type"); + if (contentType != null) { + hasContentType = true; + formEncodedContentType = contentType === "application/x-www-form-urlencoded"; + }
js07
left a comment
js07
left a comment
There was a problem hiding this comment.
Looks good to me overall. I just made one comment about recompiling the TS files.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@platform/package.json`:
- Line 31: The TypeScript semver in platform/package.json ("typescript":
">=5.5.0 <5.7.0") conflicts with workspace packages packages/sdk and packages/ai
which require "^5.6"; update the platform constraint to "^5.6" (or ">=5.6.0
<5.7.0") so the floor matches those packages, or alternatively confirm and
document that packages/sdk and packages/ai accept 5.5.x and adjust their
package.json entries instead; modify the TypeScript entry in
platform/package.json to the agreed version so all workspace constraints align.
| "jest": "^29.1.2", | ||
| "type-fest": "^4.15.0", | ||
| "typescript": "^3.5.3" | ||
| "typescript": ">=5.5.0 <5.7.0" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# List TypeScript constraints and TS tooling versions across the repo.
echo "=== TypeScript version constraints ==="
rg -n -g 'package.json' '"typescript"\s*:' --multiline-dotall
echo -e "\n=== TypeScript-related tooling ==="
rg -n -g 'package.json' '"@typescript-eslint/|ts-node|ts-jest|tsup|tslib|babel-preset-typescript'Repository: PipedreamHQ/pipedream
Length of output: 1037
Address TypeScript version mismatch in workspace packages.
The platform/package.json constraint of >=5.5.0 <5.7.0 is incompatible with packages/sdk and packages/ai, both of which require ^5.6 (meaning >=5.6.0). This means TypeScript 5.5.x would violate the constraints of these workspace packages. Either raise the floor to ^5.6 or verify that these packages will accept the lower bound before merging.
🤖 Prompt for AI Agents
In `@platform/package.json` at line 31, The TypeScript semver in
platform/package.json ("typescript": ">=5.5.0 <5.7.0") conflicts with workspace
packages packages/sdk and packages/ai which require "^5.6"; update the platform
constraint to "^5.6" (or ">=5.6.0 <5.7.0") so the floor matches those packages,
or alternatively confirm and document that packages/sdk and packages/ai accept
5.5.x and adjust their package.json entries instead; modify the TypeScript entry
in platform/package.json to the agreed version so all workspace constraints
align.
There was a problem hiding this comment.
Used the same TS version range here as for the root package.json for the repo
2 participants
This adds dedicated file reading from base64 data URLs.
The current implementation only partially handled it, and did not properly extract the metadata. Also, some components require a filename and rely on the file handling method to provide it, so I added a default
file.${ext}filename to the metadata here.Additionally upgraded the Typescript version here a bit since I was getting some build errors.
Here's an example for testing:
Summary by CodeRabbit
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.