Skip to content

Conversation

knowsuchagency
Copy link
Contributor

@knowsuchagency knowsuchagency commented Aug 22, 2025

Summary

  • Fixed parseS3Object function to properly handle S3 URIs with empty storage (s3:///)
  • Empty storage strings are now converted to undefined instead of being passed as empty strings
  • This prevents "Invalid path" errors in the backend when loading CSV previews and other S3 operations

Problem

When returning S3 objects from Python scripts using the format {"s3": path}, the system would auto-detect it as an S3 asset using the s3:/// prefix. However, this caused the parseS3Object function to return an empty string for storage, which led to backend validation errors.

Solution

Updated the parseS3Object function to check if the storage value is an empty string and convert it to undefined. This ensures the backend APIs receive the correct parameters and can handle S3 objects without explicit storage/bucket names.

Test Plan

Tested the parseS3Object function with various inputs:

  • {"s3": "path/to/file.csv"} → works correctly
  • {"s3": "path/to/file.csv", "storage": "mybucket"} → works correctly
  • "s3:///path/to/file.csv" → now correctly returns undefined for storage
  • "s3://mybucket/path/to/file.csv" → works correctly

Resolves #6442


Important

Fixes parseS3Object in utils.ts to handle S3 URIs with empty storage by converting empty storage strings to undefined, preventing backend errors.

  • Behavior:
    • Fixes parseS3Object in utils.ts to handle S3 URIs with empty storage by converting empty storage strings to undefined.
    • Prevents "Invalid path" errors in backend for CSV previews and other S3 operations.
  • Test Plan:
    • Tested parseS3Object with various inputs to ensure correct handling of empty storage in S3 URIs.

This description was created by Ellipsis for 6c218dc. You can customize this summary. It will automatically update as commits are pushed.

When S3 objects are returned with the s3:/// prefix (indicating no storage/bucket),
the parseS3Object function now correctly returns undefined for storage instead of
an empty string. This prevents "Invalid path" errors in the backend when loading
CSV previews and other S3 operations.

Fixes windmill-labs#6442
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

Important

Looks good to me! 👍

Reviewed everything up to 6c218dc in 48 seconds. Click for details.
  • Reviewed 18 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 draft comments. View those below.
  • Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. frontend/src/lib/utils.ts:1508
  • Draft comment:
    Good change: explicitly handling empty storage correctly for S3 URIs (e.g. s3:///file.csv). This ensures empty storage strings are converted to undefined, which resolves the backend error.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50% None
2. frontend/src/lib/utils.ts:1512
  • Draft comment:
    Consider using optional chaining (e.g. storage?.length > 0) for clarity over storage && storage.length > 0.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50% None

Workflow ID: wflow_t5SdKxHnV0M1wECG

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

@knowsuchagency knowsuchagency marked this pull request as draft August 22, 2025 11:34
@diegoimbert
Copy link
Contributor

diegoimbert commented Aug 25, 2025

I don't understand how the fix changes anything,
match?.[1] || undefined will return undefined if match?.[1] is falsy (and '' is falsy)

Also parseS3Object seems to only be used when opening an S3FilePicker so I don't see how it relates to backend validation

Note : this is why i used or (||) instead of nullish coallision (??)

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.

bug: S3 object output display error - 'Encountered object with invalid path' in Rich S3 Output component
2 participants