fix file download in download menu #10408
Conversation
There was a problem hiding this comment.
PR Overview
This PR fixes the file download functionality in the download menu by replacing the hardware download process with an asynchronous file-saving operation.
- Renames the download click handler from onHwDownloadClick to onFileDownloadClick.
- Replaces the compile call with an asynchronous call to saveProjectToFileAsync.
- Updates the dropdown menu event to reference the new onFileDownloadClick method.
Reviewed Changes
| File | Description |
|---|---|
| webapp/src/editortoolbar.tsx | Updated download handler to use async file save for downloads |
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
| // Matching the tick in the call to compile() above for historical reasons | ||
| pxt.tickEvent("editortools.download", { collapsed: this.getCollapsedState() }, { interactiveConsent: true }); | ||
| pxt.tickEvent("editortools.downloadasfile", { collapsed: this.getCollapsedState() }, { interactiveConsent: true }); | ||
| (this.props.parent as ProjectView).compile(true); | ||
| await (this.props.parent as ProjectView).saveProjectToFileAsync(); |
There was a problem hiding this comment.
The asynchronous function call is not wrapped in error handling. Consider adding a try/catch block to handle potential failures when saving the project to a file.
| await (this.props.parent as ProjectView).saveProjectToFileAsync(); | |
| try { | |
| await (this.props.parent as ProjectView).saveProjectToFileAsync(); | |
| } catch (error) { | |
| console.error("Failed to save project to file:", error); | |
| // Optionally, display an error message to the user | |
| } |
Copilot is powered by AI, so mistakes are possible. Review output carefully before use.
There was a problem hiding this comment.
I would assume this is already happening in saveProjectToFileAsync but I guess worth double checking?
There was a problem hiding this comment.
saveProjectToFileAsync kinda has a lot going on in it, but from what I can tell, all of the different scenarios that it calls all error there, so I think we're safe to not add this try catch.
Fixes microsoft/pxt-arcade#5912
Edit:
Also fixes microsoft/pxt-arcade#6184