fix(explore): Chart save modal displays error instead of failing silently #21920
Conversation
|
/testenv up |
|
@kgabryje Ephemeral environment spinning up at http://34.220.201.232:8080. Credentials are |
| @@ -150,9 +151,6 @@ class SaveModal extends React.Component<SaveModalProps, SaveModalState> { | |||
| async saveOrOverwrite(gotodash: boolean) { | |||
| this.setState({ alert: null, isLoading: true }); | |||
| this.props.actions.removeSaveModalAlert(); | |||
| // eslint-disable-next-line @typescript-eslint/no-unused-vars | |||
|
|
|||
| let promise = Promise.resolve(); | |||
There was a problem hiding this comment.
Changing promises to awaits looks great, thanks! Two related notes:
- Since all the
catchblocks are the same, should we just wrap the entire function body in onetryblock? - Previously, since these promises had no
catchblock, errors would go unhandled and therefore show up in the browser console. Should we preserve that behavior by re-throwing or not catching errors?
If you think yes for both of those, we could just do something like:
async saveOrOverwrite(goodish:boolean) {
try {
// If anything throws here, modal will stay open, finally block will execute,
// and error will remain unhandled because there's no catch block
this.props.onHide();
} finally {
this.setState({ isLoading: false });
}
}
|
Hey @kgabryje I think I am noticing a weird behaviour when saving the chart as a new one. After saving the chart shows the empty state. Superset.mp4 |
kgabryje
force-pushed
the
fix/save-modal-error
branch
from
134c7ef to
64a82e7
Compare
|
@geido @codyml Thank you for your feedback and testing! I fixed the issue with chart disappearing after saving. The problem was a race condition between re-hydrating explore after saving and triggering query. Because of some strange flows in the code, delaying closing the save modal was causing the query to be triggered before Explore was rehydrated. The result was that the query was updating the previous chart (previous slice_id), instead of the freshly created one. I fixed this issue by moving the logic responsible for detecting saving from URL (we were reading Screen.Recording.2022-10-26.at.10.45.32.mov |
Codecov Report
@@ Coverage Diff @@
## master #21920 +/- ##
=======================================
Coverage 66.95% 66.96%
=======================================
Files 1807 1807
Lines 69196 69208 +12
Branches 7402 7406 +4
=======================================
+ Hits 46331 46343 +12
- Misses 20954 20958 +4
+ Partials 1911 1907 -4
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
Amazing job fixing the chart refresh error!! Code looks great; two things:
- I think the loading indicator is still left-justified
- I noticed that the overwrite/saveas toggle is now sticky on the same chart, so if you open a chart that you don't have permission to overwrite and save it as a new chart, the next time you go to save it'll still be on Save As, whereas previously I think it would always default to Overwrite if you had permissions. Do we want to maintain that previous behavior?
Oops, I didn't notice your first comment. Fixed 🙂
Well spotted! 🙏 Fixed |
|
/testenv up |
|
@kgabryje Ephemeral environment spinning up at http://54.190.235.8:8080. Credentials are |
|
Ephemeral environment shutdown and build artifacts deleted. |
mistercrunch
added
🏷️ bot
SUMMARY
When user tried to save a chart and a request executed by SaveModal chart failed, the modal would just close and there would be no indication that an error occurred. That might lead the user to think that their chart was successfully saved even though it wasn't.
This PR fixes that behaviour by not closing the modal and displaying an alert with error message within it.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before:
Screen.Recording.2022-10-24.at.13.21.55.mov
After:
Screen.Recording.2022-10-24.at.12.29.24.mov
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION
CC @codyml