Skip to content

Part 3 - applying file URL or path improvement #17149

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 55 commits into from
Jun 25, 2025
Merged

Part 3 - applying file URL or path improvement #17149

merged 55 commits into from
Jun 25, 2025

Conversation

GTFalcao
Copy link
Collaborator

@GTFalcao GTFalcao commented Jun 17, 2025

Partially addresses #16977

Summary by CodeRabbit

  • New Features

    • Most file and document upload actions across supported integrations now accept both file URLs and local /tmp file paths, offering greater flexibility for file inputs.
    • File handling improvements now automatically extract and use file metadata (such as content type, size, and filename) for uploads.
  • Bug Fixes

    • Improved reliability and compatibility for file uploads by unifying file input handling and removing previous restrictions on file location.
  • Documentation

    • Updated property labels and descriptions throughout the UI to clarify support for both file URLs and local paths.
    • Enhanced descriptions for file input fields to include clear examples and guidance.
  • Refactor

    • Streamlined internal file handling logic for faster and more consistent uploads.
    • Deprecated or removed older properties and options that only supported local file paths or required manual configuration.
  • Chores

    • Updated dependencies to ensure compatibility and improved performance across all integrations.
    • Minor formatting improvements, such as adding newlines at the end of certain files.

@vunguyenhung
Copy link
Collaborator

Hello everyone, I have tested this PR and there're some test cases failed or needed improvement.

Please check the test report below for more information
https://vunguyenhung.notion.site/Part-3-applying-file-URL-or-path-improvement-217bf548bb5e8156bd12c17c8f0a2fd3

@GTFalcao
Copy link
Collaborator Author

GTFalcao commented Jun 20, 2025

About todoist-import-tasks - it seems the package used here had breaking changes over the years, I've locked the package version to avoid this.
About the zoho_catalyst components, they are .ts components so you need to run npm run build and then publish the output .mjs components:

npm run build
pd publish components\zoho_catalyst\dist\actions\detect-objects-in-image\detect-objects-in-image.mjs
pd publish components\zoho_catalyst\dist\actions\extract-text-from-image\extract-text-from-image.mjs
pd publish components\zoho_catalyst\dist\actions\perform-face-detection-and-analysis\perform-face-detection-and-analysis.mjs
pd publish components\zoho_catalyst\dist\actions\perform-face-detection-and-analysis\perform-face-detection-and-analysis.mjs

Also @vunguyenhung , there are some components in this PR that were not included in the test report:

  • ramp-upload-receipt
  • security_reporter-create-finding
  • security_reporter-update-finding
  • sftp-upload-file
  • zoho_bugtracker-create-bug
  • zoho_bugtracker-update-bug

@GTFalcao GTFalcao moved this from Changes Required to Ready for QA in Component (Source and Action) Backlog Jun 20, 2025
@vunguyenhung vunguyenhung moved this from Ready for QA to In QA in Component (Source and Action) Backlog Jun 20, 2025
@vunguyenhung vunguyenhung moved this from In QA to Changes Required in Component (Source and Action) Backlog Jun 20, 2025
@vunguyenhung
Copy link
Collaborator

Hello everyone, I have tested this PR and there're some test cases failed or needed improvement.

Please check the test report below for more information
https://vunguyenhung.notion.site/Part-3-applying-file-URL-or-path-improvement-217bf548bb5e8156bd12c17c8f0a2fd3

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
components/todoist/actions/export-tasks/export-tasks.mjs (1)

1-42: Consider adopting the standardized file handling approach.

This component still uses direct fs operations and tmp-promise, which appears inconsistent with the PR's objective of standardizing file handling using @pipedream/platform helpers like getFileStream and getFileStreamAndMetadata. Consider refactoring to align with the overall modernization effort.

For consistency with the PR's file handling standardization, consider updating this component to use @pipedream/platform helpers:

-import fs from "fs";
-import { file } from "tmp-promise";
+import { getFileStream } from "@pipedream/platform";
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 78c7203 and 14881d4.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (6)
  • components/todoist/actions/export-tasks/export-tasks.mjs (2 hunks)
  • components/todoist/actions/import-tasks/import-tasks.mjs (2 hunks)
  • components/todoist/package.json (2 hunks)
  • components/zoho_bugtracker/actions/create-bug/create-bug.mjs (2 hunks)
  • components/zoho_bugtracker/actions/update-bug/update-bug.mjs (3 hunks)
  • components/zoho_bugtracker/common/utils.mjs (0 hunks)
💤 Files with no reviewable changes (1)
  • components/zoho_bugtracker/common/utils.mjs
🚧 Files skipped from review as they are similar to previous changes (4)
  • components/todoist/actions/import-tasks/import-tasks.mjs
  • components/zoho_bugtracker/actions/update-bug/update-bug.mjs
  • components/zoho_bugtracker/actions/create-bug/create-bug.mjs
  • components/todoist/package.json
🔇 Additional comments (2)
components/todoist/actions/export-tasks/export-tasks.mjs (2)

10-10: Version bump is appropriate for the functional change.

The minor version increment from "0.0.3" to "0.1.0" is appropriate given the change from asynchronous to synchronous CSV conversion.


30-30: ```shell
#!/bin/bash

Verify json-2-csv dependency version and how it’s imported/used in the repo

1. Check if json-2-csv is listed in package.json and its version

grep -R ""json-2-csv"" -n package.json || echo "json-2-csv not found in package.json"

echo

2. Locate import or require statements for json-2-csv

rg "import .*json-2-csv" -n
rg "require('json-2-csv')" -n .

echo

3. Search for any references to the async API

rg "json2csvAsync" -n .


</details>

</blockquote></details>

</details>

<!-- This is an auto-generated comment by CodeRabbit for review status -->

@GTFalcao
Copy link
Collaborator Author

  • sftp_password_based_auth: we will have to update this in a follow-up PR, because it imports sftp as an npm package, similar to other situations we've had in the past - it cannot import the update which has not yet been published to npm.
  • todoist-import-tasks: I've updated both this and export-tasks to use the latest version of the package instead. I was able to use the action successfully afterwards.
  • zoho_bugtracker: the validation which had this error (which occurs in the live component, unrelated to this PR) confuses me, since the prop seems to expect a "document" but it accepts only image files. I've removed this entirely.
  • zoho_catalyst: oddly, I faced the same error with your account, but not with mine (I reconnected it now even though the account is very old, and it worked still). I've invited you to my account, hopefully that will work - must be a config or permission thing in the Zoho UI

image

image

@GTFalcao GTFalcao moved this from Changes Required to Ready for QA in Component (Source and Action) Backlog Jun 20, 2025
@vunguyenhung vunguyenhung moved this from Ready for QA to Changes Required in Component (Source and Action) Backlog Jun 20, 2025
@vunguyenhung
Copy link
Collaborator

Hello everyone, I have tested this PR and there're some test cases failed or needed improvement.

Please check the test report below for more information
https://vunguyenhung.notion.site/Part-3-applying-file-URL-or-path-improvement-217bf548bb5e8156bd12c17c8f0a2fd3

@GTFalcao
Copy link
Collaborator Author

I've created the follow-up issues as we've discussed, linked them to this PR, and prioritized them.

@GTFalcao
Copy link
Collaborator Author

/approve

@vunguyenhung vunguyenhung moved this from Ready for QA to In QA in Component (Source and Action) Backlog Jun 24, 2025
@vunguyenhung vunguyenhung moved this from In QA to Ready for Release in Component (Source and Action) Backlog Jun 24, 2025
@vunguyenhung
Copy link
Collaborator

Hi everyone, all test cases are passed! Ready for release!

Test report
https://vunguyenhung.notion.site/Part-3-applying-file-URL-or-path-improvement-217bf548bb5e8156bd12c17c8f0a2fd3

@GTFalcao GTFalcao merged commit 853f618 into master Jun 25, 2025
10 of 11 checks passed
@GTFalcao GTFalcao deleted the 16977-part-3 branch June 25, 2025 01:43
@github-project-automation github-project-automation bot moved this from Ready for Release to Done in Component (Source and Action) Backlog Jun 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants