Skip to content
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

feat: update file utilities for windows processing #1483

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

sjd9021
Copy link
Contributor

@sjd9021 sjd9021 commented Mar 24, 2025

Fix Windows path handling in file utils
This PR addresses an issue where filename extraction fails on Windows systems due to backslash () path separators.
Changes:
Add path normalization before basename extraction
Handle URLs and file paths differently for proper cross-platform support
Maintain backward compatibility with existing functionality
This fix ensures consistent filename extraction across all operating systems, particularly benefiting Windows users.

Copy link

vercel bot commented Mar 24, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
composio ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 25, 2025 8:23am

Copy link
Contributor

LGTM 👍

Copy link

github-actions bot commented Mar 24, 2025

This comment was generated by github-actions[bot]!

JS SDK Coverage Report

📊 Coverage report for JS SDK can be found at the following URL:
https://pub-92e668239ab84bfd80ee07d61e9d2f40.r2.dev/coverage-14054847336/coverage/index.html

📁 Test report folder can be found at the following URL:
https://pub-92e668239ab84bfd80ee07d61e9d2f40.r2.dev/html-report-14054847336/html-report/report.html

return {
name: path.split("/").pop() || `${actionName}_${Date.now()}`,
name: pathModule.basename(path) || `${actionName}_${Date.now()}`,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Consider adding a comment explaining the fallback filename pattern. Something like:

// Fallback to timestamp-based name if basename extraction fails
name: pathModule.basename(path) || `${actionName}_${Date.now()}`,

This would help future maintainers understand the purpose of the fallback.

@shreysingla-bot
Copy link
Collaborator

Code Review Summary

Overall Assessment: ✅ Good to merge with minor suggestions

The changes improve the codebase by:

  • Using proper path handling with Node.js path module
  • Making the code more cross-platform compatible
  • Maintaining existing functionality while improving robustness

Suggestions:

  1. Consider renaming pathModule to path for consistency with Node.js conventions
  2. Add a comment explaining the fallback filename generation logic

Code Quality: 8/10

  • ✅ Improves cross-platform compatibility
  • ✅ Uses proper Node.js APIs
  • ✅ Maintains error handling
  • 📝 Could use minor documentation improvements

The changes are well-thought-out and improve the code's reliability. The suggestions are minor and don't impact functionality.

Copy link
Contributor

LGTM 👍

Copy link
Contributor

LGTM 👍

Copy link
Contributor

LGTM 👍

Copy link
Contributor

LGTM 👍

Copy link
Contributor

LGTM 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants