Skip to content
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.

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

"Request failed" error when message is Deleted with "Show Next Message..." option is enabled #1220

Closed
SergeyMosin opened this issue Jul 22, 2023 · 4 comments
Labels
bug Something isn't working enhancement New feature or request

Comments

@SergeyMosin
Copy link

Describe the bug
When the "Show Next Message..." option is enabled and a message is deleted the "Request Failed" screen is briefly shown. This error mostly happens when connection between Snappymail and the Mail server is slow. This is somewhat an edge case to have slow connection server side but it does help with flushing out bugs

To Reproduce
Steps to reproduce the behavior:

  1. Slow connection (ex: install SnappyMail locally and connect to a remote IMAP/SMTP server)
  2. Enable "Show next message when (re)move current message" option
  3. Delete a message
  4. 💥 "Request failed" error pops up and the next message is not loaded

Possible Solution
The problem seems to be two fold:

  1. 'AbortError' is not properly handled
  2. Double requests are sent which causes the 'AbortError'

Abort handling error:

  1. controler.abort is called here:
    if (controller) {
    clearTimeout(controller.timeoutId);
    bClearOnly || controller.abort(sReason || 'AbortError');
    }
  2. And is caught as a type string not type error here:
    }).catch(err => {
    clearTimeout(controller.timeoutId);
    err.aborted = signal.aborted;
    err.reason = signal.reason;
    return Promise.reject(err);
    });
  3. But because 'err' is type string line 63 throws 'not an object error`
    One possible way to fix this is:
...
	}).catch(err => {
		clearTimeout(controller.timeoutId);
		// controller.abort(sReason || 'AbortError'); sends the err as string
		// and also sets signal.reason = sReason || 'AbortError'
		if (typeof err === 'string') {
			const _msg=err
			err = new Error(_msg);
			// there is err.name == 'AbortError' check in the request() function
			// maybe the check should be err.aborted === true instead
			err.name = _msg;
		}
		err.aborted = signal.aborted;
		err.reason = signal.reason;
		return Promise.reject(err);
	});
...

This way the error will be properly propagated to here:

.catch(err => {
console.error({fetchError:err});
fCallback && fCallback(
'TimeoutError' == err.reason ? 3 : (err.name == 'AbortError' ? 2 : 1),
err
);
});

and the fCallback will have the proper iError code, which will skip the 'Request failed` screen here:
if (iError) {
if (Notifications.RequestAborted !== iError && !popup) {
MessageUserStore.message(null);
MessageUserStore.error(getNotification(iError));
}
} else {

Double Requests
On slower connections there are duplicate requests under certain conditions that actually cause the AbortError.
The first request is properly initiated from here:

// Select next email https://github.com/the-djmaze/snappymail/issues/968
if (currentMessage && 1 == messages.length && SettingsUserStore.showNextMessage()) {
let next = MessagelistUserStore.indexOf(currentMessage) + 1;
if (0 < next && (next = MessagelistUserStore()[next])) {
currentMessage = null;
fireEvent('mailbox.message.show', {
folder: next.folder,
uid: next.uid
});
}
}

The second request can be initiated by the remove() call from here sometime:

setTimeout(() => messages.forEach(item => MessagelistUserStore.remove(item)), 350);

because on slow connection it will pass this.autoSelect() check here:
this.listChecked.subscribe(items => {
if (items.length) {
koSelectedItem() ? koSelectedItem(null) : koSelectedItem.valueHasMutated?.();
} else if (this.autoSelect()) {
koSelectedItem(koFocusedItem());
}
});

It is possible to fix the this.autoSelect() check like so

MessagelistUserStore.canAutoSelect = () =>
	!/is:unseen/.test(MessagelistUserStore.mainSearch())
	&& !disableAutoSelect()
	&& SettingsUserStore.usePreviewPane()
	// When the 'Show Next Message...' option is enable, the next message is "selected"
	// in a different request. This will prevent duplicate requests and 'AbortError'
	// under some conditions on slow connections
	&& !SettingsUserStore.showNextMessage();

here:

MessagelistUserStore.canAutoSelect = () =>
!/is:unseen/.test(MessagelistUserStore.mainSearch())
&& !disableAutoSelect()
&& SettingsUserStore.usePreviewPane();

@the-djmaze I can submit a PR as well.

Thanks for all the work you put into this app btw

@the-djmaze
Copy link
Owner

The handling of controller.abort() was interpreted incorrectly because i misread the documentation.
https://developer.mozilla.org/en-US/docs/Web/API/AbortController/abort#parameters

If not specified, the reason is set to "AbortError" DOMException.

I thought it was a string, but it is a DOMException object.
So i've changed the code to always use DOMException.

@the-djmaze the-djmaze added bug Something isn't working enhancement New feature or request labels Aug 21, 2023
@SergeyMosin
Copy link
Author

Thanks for fixing this.

BTW, the Double Requests possible fix in essagelistUserStore.canAutoSelect will not work. It breaks keyboard navigation.

the-djmaze pushed a commit that referenced this issue Aug 21, 2023
@the-djmaze
Copy link
Owner

On slower connections there are duplicate requests under certain conditions that actually cause the AbortError.

The main reason why this happens is disableAutoSelect.
It is set to true on MessagelistUserStore.reload and goes back to false after 500 milliseconds.

After 'MessageMove' or 'MessageDelete' it does:

MessagelistUserStore.reload(!MessagelistUserStore.length)

But there are some caveats due to uncheck/removal/page changes that cause multiple autoSelect and MessageList requests.

So now i made big changes to handle them

@the-djmaze the-djmaze added the needs feedback Further information is requested label Nov 28, 2023
@the-djmaze
Copy link
Owner

Solved, or do you still see this issue?

@the-djmaze the-djmaze removed the needs feedback Further information is requested label Nov 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants