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(schema): File download COMPASS-8704 #6740

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Conversation

paula-stacho
Copy link
Contributor

@paula-stacho paula-stacho commented Feb 21, 2025

Description

Doing the file download with blob + createObjectUrl solution. Tested in electron and chrome. Including e2e

Checklist

Motivation and Context

  • Bugfix
  • New feature
  • Dependency update
  • Misc

Open Questions

Dependents

Types of changes

  • Backport Needed
  • Patch (non-breaking change which fixes an issue)
  • Minor (non-breaking change which adds functionality)
  • Major (fix or feature that would cause existing functionality to change)

@paula-stacho paula-stacho changed the title wip feat(schema): File download COMPASS-8704 Feb 21, 2025
@github-actions github-actions bot added the feat label Feb 21, 2025
@paula-stacho paula-stacho added the no release notes Fix or feature not for release notes label Feb 21, 2025
@gribnoysup
Copy link
Collaborator

gribnoysup commented Feb 24, 2025

but I've run into some security issues with link.clicking programmatically.

I'm curious what was the case when it happened. Generally browsers allow for this if the click is triggered by another user action, so if this programmatic link clicking is triggered by a user button click for example, browsers should not prevent that

(I tried naively replacing the href with a button that creates a link and clicks it on click event and it seems to work both in local sandbox and when using browser extension to run this code in cloud)

@paula-stacho paula-stacho removed the wip label Feb 24, 2025
@paula-stacho
Copy link
Contributor Author

paula-stacho commented Feb 24, 2025

but I've run into some security issues with link.clicking programmatically.

I'm curious what was the case when it happened. Generally browsers allow for this if the click is triggered by another user action, so if this programmatic link clicking is triggered by a user button click for example, browsers should not prevent that

(I tried naively replacing the href with a button that creates a link and clicks it on click event and it seems to work both in local sandbox and when using browser extension to run this code in cloud)

Aaaah 🤦 So now I spent some more time on it and I think the issue was appending the temporary element to document.body. Appending it inside the modal appears to work (at least in electron). I don't hate the current solution though, so I'm thinking to keep it. It seems more semantic html-ish.

@gribnoysup
Copy link
Collaborator

So now I spent some more time on it and I think the issue was appending the temporary element to document.body

If you're doing this from inside the modal, the modal logic is probably conflicting with the click (either the focus trap or a pointer-events directive somewhere I'd assume). FWIW you don't need to add it in the DOM at all to click, you can just call the method on the element. Semantics are good, but I'd consider to think about what other things this affects: keeping blob in state in addition to the schema basically means we're keeping schema in memory two times, then you create the object URL and it makes it three times, I'm not sure how much it's worth it, especially as this is not a "real" URL anyway

@paula-stacho
Copy link
Contributor Author

So now I spent some more time on it and I think the issue was appending the temporary element to document.body

If you're doing this from inside the modal, the modal logic is probably conflicting with the click (either the focus trap or a pointer-events directive somewhere I'd assume). FWIW you don't need to add it in the DOM at all to click, you can just call the method on the element. Semantics are good, but I'd consider to think about what other things this affects: keeping blob in state in addition to the schema basically means we're keeping schema in memory two times, then you create the object URL and it makes it three times, I'm not sure how much it's worth it, especially as this is not a "real" URL anyway

Okay, reverting to the temporary link solution, doing the cleanup wasn't too bad but you're right that the objects can get big and we shouldn't need to store two (the url itself is small so I'm not counting that)

@paula-stacho paula-stacho marked this pull request as ready for review February 24, 2025 15:04
@Anemy Anemy added feature flagged PRs labeled with this label will not be included in the release notes of the next release and removed no release notes Fix or feature not for release notes labels Feb 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat feature flagged PRs labeled with this label will not be included in the release notes of the next release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants