Fix: Display error message when PDF export fails #4836
Conversation
|
❌ Some Jest tests failed. Please check the logs and fix the issues before merging. Failed Tests: |
There was a problem hiding this comment.
Pull request overview
This PR addresses a TODO comment by implementing user-facing error messages for PDF export failures in the Music Blocks application. Previously, when Lilypond to PDF conversion failed, errors were only logged to the console. Now, users receive visible feedback via the error message system.
Key Changes:
- Replaced TODO comment with
this.activity.errorMsg()call to display PDF generation errors to users - Improved code formatting consistency with multi-line string concatenations and standardized arrow function syntax
- Fixed JSDoc comment indentation for better readability
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
js/SaveInterface.js
Outdated
| // eslint-disable-next-line no-console | ||
| console.debug("Error: " + dataurl); | ||
| //TODO: Error message box | ||
| this.activity.errorMsg(_("Error generating PDF") + ": " + dataurl); |
There was a problem hiding this comment.
The error message exposes technical details (the raw dataurl error message) to end users, which may be confusing or unhelpful. Consider providing a more user-friendly error message that doesn't include the technical details, or at least truncate/sanitize the dataurl value before displaying it. For example, the dataurl could be a lengthy data URI or contain technical error information that isn't meaningful to users.
| this.activity.errorMsg(_("Error generating PDF") + ": " + dataurl); | |
| this.activity.errorMsg(_("Error generating PDF")); |
js/SaveInterface.js
Outdated
| // eslint-disable-next-line no-console | ||
| console.debug("Error: " + dataurl); | ||
| //TODO: Error message box | ||
| this.activity.errorMsg(_("Error generating PDF") + ": " + dataurl); |
There was a problem hiding this comment.
The test for the afterSaveLilypondPDF error case (line 702-718 in SaveInterface.test.js) should be updated to verify that activity.errorMsg is called with the expected error message. Currently, the test only checks that console.debug is called, but the new implementation also displays an error message to the user via errorMsg.
|
✅ All Jest tests passed! This PR is ready to merge. |
|
@walterbender Hi! Just a gentle ping 😊 |
|
Maybe you can sort out the conflicts... |
WillyEverGreen
force-pushed
the
fix-pdf-error-msg
branch
from
fb95906 to
1b56c10
Compare
|
❌ Some Jest tests failed. Please check the logs and fix the issues before merging. Failed Tests: |
Replace TODO comment with call to this.activity.errorMsg() so PDF export failures are surfaced to the user instead of only being logged to console.
WillyEverGreen
force-pushed
the
fix-pdf-error-msg
branch
from
1b56c10 to
04781a9
Compare
|
❌ Some Jest tests failed. Please check the logs and fix the issues before merging. Failed Tests: |
|
@walterbender I’ve rebased the branch onto the latest master and addressed the conflicts you mentioned. |
2 participants
Description
This PR addresses a TODO in
SaveInterface.jswhere PDF export errors were previously only logged to the console.Changes
this.activity.errorMsg()so PDF export failures are surfaced to the user.ly2pdfconversion fails, a visible error message is displayed instead of a silent failure.Testing