feat: Update file processor support #1254
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
👍 Looks good to me! Reviewed everything up to e5115aa in 1 minute and 56 seconds
More details
- Looked at
8734lines of code in28files - Skipped
0files when reviewing. - Skipped posting
13drafted comments based on config settings.
1. js/src/utils/processor/file.ts:17
- Draft comment:
Consider adding explicit types for function parameters for clarity. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. js/src/utils/processor/file.ts:55
- Draft comment:
Wrap the FILE_INPUT_PROCESSOR logic with error handling for better debuggability. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. js/src/utils/processor/fileUtils.ts:20
- Draft comment:
Consider using axios instead of fetch for consistency with other HTTP requests. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. js/src/utils/processor/fileUtils.ts:47
- Draft comment:
Wrap the axios.put call with try/catch to handle upload errors explicitly. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
5. js/src/sdk/models/backendClient.ts:55
- Draft comment:
Consider validating the base URL is reachable before usage, beyond checking its prefix. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
6. js/src/sdk/models/triggers.ts:172
- Draft comment:
Avoid using 'unknown as SingleTriggerRes' by refining the type explicitly. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
7. js/src/sdk/models/integrations.ts:218
- Draft comment:
Delay throwing error for missing uniqueKey until after checking for its value from apps. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
8. js/src/sdk/utils/errors/src/composioError.ts:55
- Draft comment:
Consider logging a summary of the original error in production as well, not only in debug. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
9. js/src/sdk/testUtils/getBackendClient.ts:4
- Draft comment:
Provide specific error messages for missing COMPOSIO_API_KEY and BACKEND_HERMES_URL to aid in debugging. - Reason this comment was not posted:
Comment was on unchanged code.
10. js/src/utils/logger.ts:30
- Draft comment:
Consider using structured (e.g., JSON) logging for better machine parsing in production. - Reason this comment was not posted:
Confidence changes required:50%
None
11. js/src/utils/logger.ts:31
- Draft comment:
Consider using a safe JSON serializer for objects (e.g. one that handles circular references) in the formatErrorMessage function. This may prevent potential errors when logging complex objects. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
12. js/src/utils/logger.ts:67
- Draft comment:
Note that the logger instance is computed once at module load via getLogger(). If runtime log level changes are needed, consider providing a way to update or reinitialize the logger. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
13. js/src/utils/logger.ts:16
- Draft comment:
The getLogLevel function uses a simple check with 'envLevel in LOG_LEVELS'. Consider using Object.prototype.hasOwnProperty.call for clarity and robustness. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_GB4j9xdDS9FXUtmU
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
![entelligence-ai-pr-reviews[bot]](https://avatars.githubusercontent.com/in/932812?s=60&v=4){kind=small}
js/config/test.config.local.json
Outdated
| "drive":{ | ||
| "download_file_id":"18rcI9N7cJRG15E2qyWXtNSFeDg4Rj-T3" | ||
| } |
There was a problem hiding this comment.
Hardcoded Google Drive file ID in config file poses security risk and reduces flexibility. Should be moved to environment variables or secure configuration management.
📝 Committable Code Suggestion
‼️ Ensure you review the code suggestion before committing it to the branch. Make sure it replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| "drive":{ | |
| "download_file_id":"18rcI9N7cJRG15E2qyWXtNSFeDg4Rj-T3" | |
| } | |
| "drive":{ | |
| "download_file_id":"${GOOGLE_DRIVE_FILE_ID}" | |
| } |
|
This comment was generated by github-actions[bot]! JS SDK Coverage Report📊 Coverage report for JS SDK can be found at the following URL: 📁 Test report folder can be found at the following URL: |
| @@ -1,4 +1,7 @@ | |||
| { | |||
| "COMPOSIO_API_KEY": "pv7s0lpq7z5vu27cikyls", | |||
There was a problem hiding this comment.
API key pv7s0lpq7z5vu27cikyls is hardcoded in the production config file. Sensitive credentials should be loaded from environment variables or a secure key management system.
📝 Committable Code Suggestion
‼️ Ensure you review the code suggestion before committing it to the branch. Make sure it replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| "COMPOSIO_API_KEY": "pv7s0lpq7z5vu27cikyls", | |
| "COMPOSIO_API_KEY": process.env.COMPOSIO_API_KEY, |
| // @ts-ignore | ||
| expect(executionResult.data.file.uri.length).toBeGreaterThan(0); |
There was a problem hiding this comment.
Using @ts-ignore to suppress TypeScript errors masks potential runtime issues. Should properly type executionResult.data or handle potential undefined values.
📝 Committable Code Suggestion
‼️ Ensure you review the code suggestion before committing it to the branch. Make sure it replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| // @ts-ignore | |
| expect(executionResult.data.file.uri.length).toBeGreaterThan(0); | |
| expect(executionResult.data?.file?.uri?.length).toBeGreaterThan(0); |
| // Dirty way to avoid copy | ||
| let dataToReturn = JSON.parse(JSON.stringify(data)); |
There was a problem hiding this comment.
Deep cloning with JSON.parse(JSON.stringify()) can fail for objects with functions, circular references, or special values like undefined. Should use structured cloning or a deep clone library.
📝 Committable Code Suggestion
‼️ Ensure you review the code suggestion before committing it to the branch. Make sure it replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| // Dirty way to avoid copy | |
| let dataToReturn = JSON.parse(JSON.stringify(data)); | |
| // Use structured clone for deep copy | |
| let dataToReturn = structuredClone(data); |
| testConnectors: { | ||
| items: { | ||
| type: "object", | ||
| }, | ||
| type: "array", | ||
| $ref: "#/components/schemas/TestConnector", | ||
| description: "The authentication schemes of the app", | ||
| }, |
There was a problem hiding this comment.
The testConnectors field in $AppInfoResponseDto has an incorrect type definition - it defines both items and $ref which is invalid OpenAPI schema syntax. Should use only $ref.
📝 Committable Code Suggestion
‼️ Ensure you review the code suggestion before committing it to the branch. Make sure it replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| testConnectors: { | |
| items: { | |
| type: "object", | |
| }, | |
| type: "array", | |
| $ref: "#/components/schemas/TestConnector", | |
| description: "The authentication schemes of the app", | |
| }, | |
| testConnectors: { | |
| $ref: "#/components/schemas/TestConnector", | |
| description: "The authentication schemes of the app", | |
| }, |
| path: string | ||
| ): Promise<{ content: string; mimeType: string }> => { | ||
| try { | ||
| const content = require("fs").readFileSync(path, "utf-8"); |
There was a problem hiding this comment.
The readFileContent function should implement proper file size validation and streaming for large files. Current implementation loads entire file into memory which could cause issues with large files.
Suggested implementation:
const readFileContent = async (path: string): Promise<{ content: string; mimeType: string }> => {
try {
const stats = fs.statSync(path);
const MAX_SIZE = 10 * 1024 * 1024; // 10MB
if (stats.size > MAX_SIZE) {
throw new Error(`File size ${stats.size} exceeds limit ${MAX_SIZE}`);
}
// For large files, use streaming
if (stats.size > 1024 * 1024) { // 1MB
return new Promise((resolve, reject) => {
const chunks: Buffer[] = [];
const stream = fs.createReadStream(path, { highWaterMark: 64 * 1024 });
stream.on('data', chunk => chunks.push(chunk));
stream.on('end', () => resolve({
content: Buffer.concat(chunks).toString('utf-8'),
mimeType: 'text/plain'
}));
stream.on('error', reject);
});
}
const content = fs.readFileSync(path, 'utf-8');
return { content, mimeType: 'text/plain' };
} catch (error) {
throw new ComposioError({
code: 'FILE_READ_ERROR',
message: `Failed to read file at ${path}`,
details: { path, error: error.message }
});
}
};| const readFileContentFromURL = async ( | ||
| path: string | ||
| ): Promise<{ content: string; mimeType: string }> => { | ||
| const response = await fetch(path); |
There was a problem hiding this comment.
The readFileContentFromURL function needs security improvements to prevent SSRF attacks and validate URLs. Consider adding:
- URL validation
- Domain allowlist
- Timeout limits
- Response size limits
Suggested implementation:
const readFileContentFromURL = async (path: string): Promise<{ content: string; mimeType: string }> => {
// Validate URL format
try {
new URL(path);
} catch {
throw new Error('Invalid URL format');
}
// Check against allowed domains
const url = new URL(path);
const ALLOWED_DOMAINS = ['trusted-domain.com']; // Configure via env/config
if (!ALLOWED_DOMAINS.includes(url.hostname)) {
throw new Error(`Domain ${url.hostname} not allowed`);
}
// Fetch with timeout and size limits
const MAX_SIZE = 10 * 1024 * 1024; // 10MB
const TIMEOUT = 10000; // 10 seconds
const controller = new AbortController();
const timeout = setTimeout(() => controller.abort(), TIMEOUT);
try {
const response = await fetch(path, {
signal: controller.signal,
headers: {
'Accept': 'text/plain,application/json'
}
});
const contentLength = response.headers.get('content-length');
if (contentLength && parseInt(contentLength) > MAX_SIZE) {
throw new Error('Response too large');
}
const content = await response.text();
if (content.length > MAX_SIZE) {
throw new Error('Response too large');
}
return {
content,
mimeType: response.headers.get('content-type') || 'text/plain'
};
} finally {
clearTimeout(timeout);
}
};
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 88cdf04 in 37 seconds
More details
- Looked at
38lines of code in2files - Skipped
0files when reviewing. - Skipped posting
6drafted comments based on config settings.
1. js/src/sdk/base.toolset.ts:172
- Draft comment:
Renamed variable to appActions improves clarity; ensure all references are adjusted accordingly. - Reason this comment was not posted:
Confidence changes required:0%
None
2. js/src/sdk/base.toolset.ts:195
- Draft comment:
Concatenation of action arrays now uses appActions; implementation is clear and succinct. - Reason this comment was not posted:
Confidence changes required:0%
None
3. js/src/sdk/utils/errors/src/composioError.ts:83
- Draft comment:
Adjusted indentation of the closing block improves code readability. - Reason this comment was not posted:
Confidence changes required:0%
None
4. js/src/sdk/base.toolset.ts:169
- Draft comment:
Renamed 'apps' to 'appActions' for clarity; update consistent references to the actions list. - Reason this comment was not posted:
Comment did not seem useful: This comment is purely informative, as it just states what was done without providing any actionable feedback or suggestions. According to the rules, purely informative comments should be removed.
5. js/src/sdk/base.toolset.ts:191
- Draft comment:
Reformatted the 'toolsActions' array for improved readability; no functional change. - Reason this comment was not posted:
Comment did not seem useful: This comment is purely informative and does not provide any actionable feedback or suggestions. It simply states that a reformatting was done for readability, which does not align with the rules provided.
6. js/src/sdk/utils/errors/src/composioError.ts:83
- Draft comment:
Adjusted indentation of the closing brace to match the project's style guidelines. - Reason this comment was not posted:
Comment did not seem useful: This comment is purely informative and does not provide any actionable feedback or suggestions. It simply states what was done without offering any guidance or questions for the PR author.
Workflow ID: wflow_YY7wi1xnzp57bYrx
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| appName: string, | ||
| mimeType: string | ||
| ): Promise<string> => { | ||
| const { data } = await apiClient.actionsV2.createFileUploadUrl({ |
There was a problem hiding this comment.
The uploadFileToS3 function needs retry logic and better error handling for reliability. Consider:
- Implementing exponential backoff retries
- Validating upload response
- Adding proper error context
Suggested implementation:
const uploadFileToS3 = async (
content: string,
actionName: string,
appName: string,
mimeType: string,
retries = 3
): Promise<string> => {
// Get signed URL
const { data } = await apiClient.actionsV2.createFileUploadUrl({
body: {
action: actionName,
app: appName,
filename: `${actionName}_${Date.now()}`,
mimetype: mimeType,
md5: crypto.createHash("md5").update(content).digest("hex"),
},
path: { fileType: "request" },
});
const signedURL = data!.url;
// Implement retry with exponential backoff
for (let attempt = 0; attempt < retries; attempt++) {
try {
const response = await axios.put(signedURL, content, {
headers: {
'Content-Type': mimeType,
'Content-MD5': crypto.createHash("md5").update(content).digest("base64")
}
});
// Validate upload success
if (response.status !== 200) {
throw new Error(`Upload failed with status ${response.status}`);
}
return signedURL;
} catch (error) {
if (attempt === retries - 1) {
throw new ComposioError({
code: 'S3_UPLOAD_ERROR',
message: 'Failed to upload file to S3',
details: {
attempt,
error: error.message,
actionName,
appName
}
});
}
// Exponential backoff
await new Promise(resolve =>
setTimeout(resolve, Math.pow(2, attempt) * 1000)
);
}
}
throw new Error('Upload failed after retries');
};
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 1413d3d in 35 seconds
More details
- Looked at
26lines of code in2files - Skipped
0files when reviewing. - Skipped posting
4drafted comments based on config settings.
1. js/src/frameworks/openai.spec.ts:30
- Draft comment:
Test case re-enabled from it.failing. Ensure that this test now consistently passes. If it still fails, update or keep it as failing. - Reason this comment was not posted:
Comment did not seem useful: The comment is asking the PR author to ensure that a test consistently passes, which falls under the rule of not asking the author to ensure or verify something. Therefore, this comment is not approved.
2. js/src/sdk/base.toolset.spec.ts:202
- Draft comment:
Re-enabled test for 'should get tools with usecase limit'. Confirm that the expected outcome now passes reliably. - Reason this comment was not posted:
Marked as duplicate.
3. js/src/frameworks/openai.spec.ts:30
- Draft comment:
Removed the 'failing' marker from the usecase limit test. Ensure that this feature now passes reliably and add documentation if this change reflects a fixed bug. - Reason this comment was not posted:
Marked as duplicate.
4. js/src/sdk/base.toolset.spec.ts:202
- Draft comment:
Updated the previously failing test to a normal test for the usecase limit. Confirm that this change is valid and the functionality works as intended. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_UZxZRnFaP8SgY8RU
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
himanshu-dixit
changed the title
| s3Url: string; | ||
| mimeType: string; | ||
| }) => { | ||
| const response = await axios.get(s3Url); |
There was a problem hiding this comment.
The downloadFileFromS3 function should implement proper error handling and validation. Consider:
- Validating response content type
- Adding size limits
- Implementing retry logic
- Proper cleanup of temp files
Suggested implementation:
const downloadFileFromS3 = async ({
actionName,
s3Url,
mimeType,
}: {
actionName: string;
s3Url: string;
mimeType: string;
}): Promise<{
name: string;
mimeType: string;
s3Key: string;
filePath: string;
}> => {
const MAX_SIZE = 10 * 1024 * 1024; // 10MB
const ALLOWED_TYPES = ['text/plain', 'application/json']; // Configure via env
if (!ALLOWED_TYPES.includes(mimeType)) {
throw new Error(`Unsupported mime type: ${mimeType}`);
}
try {
const response = await axios.get(s3Url, {
responseType: 'text',
maxContentLength: MAX_SIZE,
validateStatus: (status) => status === 200
});
// Validate response
const actualType = response.headers['content-type'];
if (actualType !== mimeType) {
throw new Error(`Mime type mismatch: expected ${mimeType}, got ${actualType}`);
}
const extension = mimeType.split("/")[1] || "txt";
const fileName = `${actionName}_${Date.now()}.${extension}`;
const filePath = saveFile(fileName, response.data, true);
if (!filePath) {
throw new Error('Failed to save file');
}
return {
name: fileName,
mimeType: mimeType,
s3Key: s3Url,
filePath: filePath,
};
} catch (error) {
// Cleanup any partially downloaded files
try {
if (error.filePath) {
fs.unlinkSync(error.filePath);
}
} catch {} // Ignore cleanup errors
throw new ComposioError({
code: 'S3_DOWNLOAD_ERROR',
message: 'Failed to download file from S3',
details: {
url: s3Url,
mimeType,
error: error.message
}
});
}
};
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 1a07bd1 in 38 seconds
More details
- Looked at
63lines of code in5files - Skipped
0files when reviewing. - Skipped posting
10drafted comments based on config settings.
1. js/config/getTestConfig.ts:8
- Draft comment:
Renamed 'download_file_id' to 'downloadable_file_id' for consistency. Verify that all dependent code uses the new key. - Reason this comment was not posted:
Comment did not seem useful: The comment starts with 'Verify that...', which is a clear indication that it violates the rule against asking the PR author to confirm or ensure something. The first part of the comment is purely informative, which is also not allowed. Therefore, this comment should not be approved.
2. js/config/test.config.local.json:5
- Draft comment:
Ensure JSON config uses the new key 'downloadable_file_id'. - Reason this comment was not posted:
Marked as duplicate.
3. js/config/test.config.prod.json:5
- Draft comment:
Ensure prod config JSON uses the new key 'downloadable_file_id'. - Reason this comment was not posted:
Marked as duplicate.
4. js/config/test.config.staging.json:5
- Draft comment:
Ensure staging config JSON uses the new key 'downloadable_file_id'. - Reason this comment was not posted:
Marked as duplicate.
5. js/src/sdk/base.toolset.spec.ts:193
- Draft comment:
Updated call to use testConfig.drive.downloadable_file_id; verify proper mapping in the action. - Reason this comment was not posted:
Marked as duplicate.
6. js/config/getTestConfig.ts:8
- Draft comment:
Renamed property to 'downloadable_file_id' for consistency. Ensure all references are updated. - Reason this comment was not posted:
Marked as duplicate.
7. js/config/test.config.local.json:5
- Draft comment:
Updated key to 'downloadable_file_id' to match the new naming convention. - Reason this comment was not posted:
Marked as duplicate.
8. js/config/test.config.prod.json:5
- Draft comment:
Key renamed to 'downloadable_file_id'; ensure downstream usage aligns with this update. - Reason this comment was not posted:
Marked as duplicate.
9. js/config/test.config.staging.json:5
- Draft comment:
Renamed key to 'downloadable_file_id' for consistency with other environments. - Reason this comment was not posted:
Marked as duplicate.
10. js/src/sdk/base.toolset.spec.ts:193
- Draft comment:
Updated reference to use 'downloadable_file_id' per the renamed config key. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_ZBcVLVFFLEw15Im3
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
Code Review SummaryThe PR introduces important file handling functionality but has several areas that need improvement: Security Issues
Reliability Issues
Performance Issues
Code Quality
Testing
I've left detailed comments on specific files with suggested improvements. The core functionality is good but needs hardening for production use. Rating: 6/10 - Needs improvements in security and reliability before production deployment. |
| name: path.split("/").pop() || `${actionName}_${Date.now()}`, | ||
| mimetype: fileData.mimeType, | ||
| s3key: signedURL, |
There was a problem hiding this comment.
Inconsistent property naming between input fileData.mimeType and returned mimetype/s3key could cause integration issues. Properties should maintain consistent casing across the codebase.
📝 Committable Code Suggestion
‼️ Ensure you review the code suggestion before committing it to the branch. Make sure it replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| name: path.split("/").pop() || `${actionName}_${Date.now()}`, | |
| mimetype: fileData.mimeType, | |
| s3key: signedURL, | |
| name: path.split("/").pop() || `${actionName}_${Date.now()}`, | |
| mimeType: fileData.mimeType, | |
| s3Key: signedURL, |
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 76d7f70 in 1 minute and 16 seconds
More details
- Looked at
26lines of code in1files - Skipped
0files when reviewing. - Skipped posting
4drafted comments based on config settings.
1. js/src/sdk/utils/processor/fileUtils.ts:66
- Draft comment:
Renamed property from 'mimeType' to 'mimetype'. Ensure consistency across the module (e.g., with downloadFileFromS3) and with external contract consumers as this may break compatibility. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =40%<= threshold50%
The comment is asking the author to ensure consistency across the module and with external contract consumers, which is a form of asking the author to double-check their work. This violates the rule against asking the author to confirm or ensure things. However, it does point out a potential issue with breaking compatibility, which could be useful. Overall, the comment is borderline but leans towards being a violation.
2. js/src/sdk/utils/processor/fileUtils.ts:82
- Draft comment:
Renamed property from 's3Key' to 's3key'. Confirm clients expect the new key format to avoid breaking changes. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =40%<= threshold50%
The comment is asking the PR author to confirm that clients expect the new key format, which violates the rule against asking for confirmation of intention or behavior. However, it does point out a potential issue with renaming a property that could lead to breaking changes, which is a valid concern. The comment could be rephrased to suggest checking for potential breaking changes without asking for confirmation.
3. js/src/sdk/utils/processor/fileUtils.ts:66
- Draft comment:
Renamed return type keys from 'mimeType'/'s3Key' to 'mimetype'/'s3key'. Ensure all consumers are updated to match this change. - Reason this comment was not posted:
Marked as duplicate.
4. js/src/sdk/utils/processor/fileUtils.ts:82
- Draft comment:
The returned object now uses 'mimetype' and 's3key'. Confirm that using fileData.mimeType (camelCase) as the value remains consistent with other utilities. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_5D1MD55xSmE2j5d5
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| const data = response.data as unknown as { url: string; s3key: string }; | ||
| const signedURL = data!.url; | ||
| const s3key = data!.s3key; |
There was a problem hiding this comment.
Unnecessary non-null assertion (!) used with data.url and data.s3key after type assertion. Since we control the type, these assertions can be removed.
📝 Committable Code Suggestion
‼️ Ensure you review the code suggestion before committing it to the branch. Make sure it replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| const data = response.data as unknown as { url: string; s3key: string }; | |
| const signedURL = data!.url; | |
| const s3key = data!.s3key; | |
| const data = response.data as unknown as { url: string; s3key: string }; | |
| const signedURL = data.url; | |
| const s3key = data.s3key; |
There was a problem hiding this comment.
❌ Changes requested. Incremental review on 16614d2 in 1 minute and 20 seconds
More details
- Looked at
52lines of code in1files - Skipped
0files when reviewing. - Skipped posting
4drafted comments based on config settings.
1. js/src/sdk/utils/processor/fileUtils.ts:55
- Draft comment:
Returning signedURL on 403 may be inconsistent with the expected return value (s3key) causing downstream issues. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. js/src/sdk/utils/processor/fileUtils.ts:42
- Draft comment:
Consider removing non-null assertions on data properties if already using type assertions; it can hide potential null errors. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
3. js/src/sdk/utils/processor/fileUtils.ts:55
- Draft comment:
Inconsistent return: on 403 error the function returns 'signedURL' while normally it returns 's3key'. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. js/src/sdk/utils/processor/fileUtils.ts:77
- Draft comment:
Renaming the variable to 's3key' in getFileDataAfterUploadingToS3 improves clarity; verify that the API always provides a valid s3key. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
Workflow ID: wflow_bJ24ZeduzhHLFbkJ
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| }, | ||
| }); | ||
|
|
||
| const data = response.data as unknown as { url: string; s3key: string }; |
There was a problem hiding this comment.
Avoid relying on non-null assertion with an unchecked API response. Consider validating that response.data has both 'url' and 's3key'.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 07b6ed2 in 37 seconds
More details
- Looked at
16lines of code in1files - Skipped
0files when reviewing. - Skipped posting
4drafted comments based on config settings.
1. js/src/sdk/utils/processor/fileUtils.ts:45
- Draft comment:
Updated response mapping to use 'key' instead of 's3key' per updated API spec. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
This comment is purely informative, explaining a change that was made. It doesn't provide a suggestion, ask for confirmation, or point out a potential issue. It violates the rule against making purely informative comments.
2. js/src/sdk/utils/processor/fileUtils.ts:47
- Draft comment:
Variable renamed to 's3key' from data.key, ensure consistency with backend response. - Reason this comment was not posted:
Marked as duplicate.
3. js/src/sdk/utils/processor/fileUtils.ts:45
- Draft comment:
Updated type field to use 'key' from API. Ensure API docs align with this change. - Reason this comment was not posted:
Marked as duplicate.
4. js/src/sdk/utils/processor/fileUtils.ts:47
- Draft comment:
Assigning s3key from data!.key. Verify downstream code compatibility with this property name. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_GssWRSAkDwcZXtYk
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| @@ -104,7 +104,7 @@ export const generateMetadataFromAxiosError = ( | |||
| metadata?: Record<string, unknown>; | |||
| } | |||
| ): Record<string, unknown> => { | |||
| const requestId = axiosError.response?.headers["x-request-id"]; | |||
| const requestId = axiosError.request?.headers["x-request-id"]; | |||
There was a problem hiding this comment.
Changed from response to request headers which is incorrect - request headers won't contain the x-request-id. The x-request-id is set by the server in response headers.
📝 Committable Code Suggestion
‼️ Ensure you review the code suggestion before committing it to the branch. Make sure it replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| const requestId = axiosError.request?.headers["x-request-id"]; | |
| const requestId = axiosError.response?.headers["x-request-id"]; |
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 30ecf8f in 35 seconds
More details
- Looked at
13lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. js/src/sdk/utils/errors/src/formatter.ts:107
- Draft comment:
Review the change of using axiosError.request.headers instead of axiosError.response.headers for 'x-request-id'. Ensure that request.headers is always available and contains 'x-request-id', as axiosError.request can sometimes be an object without the expected header structure. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =20%<= threshold50%
The comment is asking the author to ensure that a specific change is correct and that the expected structure is present. It is not making a specific suggestion or pointing out a clear issue, but rather asking for confirmation of the author's intention. This violates the rule against asking the author to confirm their intention or ensure behavior is intended.
2. js/src/sdk/utils/errors/src/formatter.ts:107
- Draft comment:
Confirm retrieving 'x-request-id' from request.headers is intended. If the header is originally returned from the response, consider a fallback to response.headers. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_rNgRmXSZKnxHUy8m
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
Important
Enhance file processing with new processors and S3 support, update configurations, and replace
BackendClientwithAxiosBackendClient.FILE_INPUT_PROCESSOR,FILE_DOWNLOADABLE_PROCESSOR, andFILE_SCHEMA_PROCESSORinfile.tsfor handling file uploads and downloads.getFileDataAfterUploadingToS3anddownloadFileFromS3infileUtils.tsfor S3 interactions.getTestConfig.tsto includedrive.downloadable_file_id.test.config.local.json,test.config.prod.json, andtest.config.staging.jsonto includedownloadable_file_id.BackendClientwithAxiosBackendClientacross multiple files for consistency.Composioclass inindex.tsto useAxiosBackendClient.backendClient.spec.tsto testAxiosBackendClientinitialization and error handling.openapi-ts.config.jsto production URL.types.gen.ts.This description was created by
for 30ecf8f. It will automatically update as commits are pushed.