Render correct error and enable buttons when pull failed#2964
Render correct error and enable buttons when pull failed#2964
Conversation
📊 Performance Test ResultsComparing 2555182 vs trunk app-size
site-editor
site-startup
Results are median values from multiple test runs. Legend: 🟢 Improvement (faster) | 🔴 Regression (slower) | ⚪ No change (<50ms diff) |
| return rejectWithValue( { | ||
| title: sprintf( __( 'Error pulling from %s' ), currentPullState.selectedSite.name ), | ||
| message: __( 'Failed to check backup file size. Please try again.' ), | ||
| message: error instanceof Error ? error.message : String( error ), |
There was a problem hiding this comment.
Do you we could use getErrorFromResponse here to make sure we are not exposing raw and unclear strings to users?
There was a problem hiding this comment.
Thanks Kat, I took a look at that function, and I see it checks for error inside error. Looks like it's some workaround to fix backend responses. So, getErrorFromResponse should be used directly to parse responses immediately. But in the upper level, like in this PR - it should not be used.
Moreover, getErrorFromResponse returns either error.error or generic string Studio was unable to connect to WordPress.com. Please try again.. It doesn't return simple error. It means that in the case described in this PR, getErrorFromResponse would return Studio was unable to connect to WordPress.com. Please try again., instead of the original no such file or directory, lstat '/var/folders/17/pw87dj654w53d10tbm0_tnhw0000gn/T/studio_backupBVYe8E/sql/wp_Links.sql'.
So we should go with the current state of changes in this PR.
There was a problem hiding this comment.
Alright, let's leave as is then 👍 Thanks for the explanation!
…tErrorAndEnableButtonsWhenPullFailed
katinthehatsite
left a comment
There was a problem hiding this comment.
The changes look good, thanks for this improvement 👍
|
There was a conflict with trunk on this PR so I resolved it and we can merge when the checks pass. |
| remoteSiteId | ||
| )( getState() ); | ||
|
|
||
| if ( signal.aborted && currentState?.status.key === 'cancelled' ) { |
There was a problem hiding this comment.
Why do we also need to check the pull state status here? In which scenario might the signal be aborted, but the pull state would have a different status?
There was a problem hiding this comment.
In which scenario might the signal be aborted, but the pull state would have a different status?
TBH, I don't remember exactly 😅 I looked at my testing steps to recall, and it's the "Downloading backup" step, but, AFAIR, when I was debugging - it was caught here as "importing".
There was a problem hiding this comment.
Why do we also need to check the pull state status here?
To avoid muting errors and print them to users. Before, we just said "Import failed," which didn't help users to investigate the actual issue and try to fix it themselves.
Additionally, this PR fixed a misleading error in other cases - we always printed an irrelevant error "Failed to check backup file size. Please try again."
Related issues
How AI was used in this PR
AI assisted me with the investigation and writing code, but the solution is mine.
Proposed Changes
This PR fixes two issues when pulling failed:
Testing Instructions
currentState?.status.key === 'cancelled'condition)Failed to check backup file size.):