Skip to content

fix: uncaught error when renaming file failed#1563

Merged
dsanders11 merged 2 commits intoelectron:mainfrom
gaspardruan:fix-rename-file
Mar 19, 2024
Merged

fix: uncaught error when renaming file failed#1563
dsanders11 merged 2 commits intoelectron:mainfrom
gaspardruan:fix-rename-file

Conversation

@gaspardruan
Copy link
Contributor

Before

appState.editorMosaic.remove(editorId);
appState.editorMosaic.addNewFile(id, contents);
if (visible) appState.editorMosaic.show(id);

If the new file name already existed or was another main entry point file, addNewFile() would throw error. Then, remove() would be executed but addNewFile() not, which means renaming file failure would lead to the deletion of the file.

Now

try {
    appState.editorMosaic.addNewFile(id, contents);
    appState.editorMosaic.remove(editorId);

    if (visible) appState.editorMosaic.show(id);
} catch (err) {
    appState.showErrorDialog(err.message);
}

@gaspardruan gaspardruan requested review from a team and codebytere as code owners March 18, 2024 09:00
Copy link
Member

@codebytere codebytere left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this! We should also add a test to confirm that this fixes the issue.

You'll want to add the tests to this file, using a similar approach as the fails if trying to rename an editor to an invalid value test there.

@gaspardruan gaspardruan requested a review from codebytere March 18, 2024 13:06
Copy link
Member

@codebytere codebytere left a comment

Choose a reason for hiding this comment

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

thanks!!

@coveralls
Copy link

Coverage Status

coverage: 87.361% (+0.1%) from 87.245%
when pulling b7e597d on gaspardruan:fix-rename-file
into 23eb6a7 on electron:main.

Copy link
Member

@dsanders11 dsanders11 left a comment

Choose a reason for hiding this comment

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

Thanks @gaspardruan! Another quality PR. 🙂

@dsanders11 dsanders11 merged commit 76aaa80 into electron:main Mar 19, 2024
@gaspardruan gaspardruan deleted the fix-rename-file branch March 19, 2024 07:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants