Skip to content

Render correct error and enable buttons when pull failed #2964

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.

Sign up for GitHub

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

Merged
nightnei merged 3 commits into from
Apr 9, 2026
Merged

Render correct error and enable buttons when pull failed #2964

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1343dc3
Render correct error and enable buttons when pull failed
nightnei Apr 2, 2026
c72caff
Lint
nightnei Apr 6, 2026
2555182
Merge branch 'trunk' of github.com:Automattic/studio into printCorrec…
Apr 9, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion apps/studio/src/ipc-utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import { PreviewCommandLoggerAction } from '@studio/common/logger-actions';
import { ImportExportEventData } from 'src/lib/import-export/handle-events';
import { getMainWindow } from 'src/main-window';
import type { StoredAuthToken } from '@studio/common/lib/shared-config';
import type { UserData } from 'src/storage/storage-types';

type SnapshotEventData = {
action: PreviewCommandLoggerAction;
Expand Down
9 changes: 7 additions & 2 deletions apps/studio/src/stores/sync/sync-operations-slice.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -868,14 +868,19 @@ const pollPullBackupThunk = createTypedAsyncThunk(
);
}
} catch ( error ) {
if ( signal.aborted ) {
const currentState = syncOperationsSelectors.selectPullState(
selectedSiteId,
remoteSiteId
)( getState() );

if ( signal.aborted && currentState?.status.key === 'cancelled' ) {
Copy link
Copy Markdown
Contributor

@fredrikekelund fredrikekelund Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor Author

@nightnei nightnei Apr 9, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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".

Copy link
Copy Markdown
Contributor Author

@nightnei nightnei Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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."

return;
}

Sentry.captureException( error );
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 ),
Copy link
Copy Markdown
Contributor

@katinthehatsite katinthehatsite Apr 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you we could use getErrorFromResponse here to make sure we are not exposing raw and unclear strings to users?

Copy link
Copy Markdown
Contributor Author

@nightnei nightnei Apr 7, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

@katinthehatsite katinthehatsite Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, let's leave as is then 👍 Thanks for the explanation!

} );
}
}
Expand Down
Loading