Skip to content

Commit

Permalink
Don't show error if user navigates while fetching members
Browse files Browse the repository at this point in the history
Don't show error messages when requests fail due to being canceled from the
client end while in flight. This can happen when navigating (eg. changing page)
while members are being fetched.
  • Loading branch information
robertknight committed Dec 6, 2024
1 parent 07f2e3b commit a8e4855
Show file tree
Hide file tree
Showing 2 changed files with 53 additions and 9 deletions.
22 changes: 16 additions & 6 deletions h/static/scripts/group-forms/components/EditGroupMembersForm.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,13 @@ import {
Pagination,
Select,
} from '@hypothesis/frontend-shared';
import { useContext, useEffect, useState } from 'preact/hooks';
import { useCallback, useContext, useEffect, useState } from 'preact/hooks';

import { Config } from '../config';
import type { APIConfig, Group } from '../config';
import type { GroupMember, GroupMembersResponse, Role } from '../utils/api';
import { callAPI } from '../utils/api';
import type { APIError } from '../utils/api';
import FormContainer from './forms/FormContainer';
import ErrorNotice from './ErrorNotice';
import GroupFormHeader from './GroupFormHeader';
Expand Down Expand Up @@ -180,9 +181,18 @@ export default function EditGroupMembersForm({
const [totalMembers, setTotalMembers] = useState<number | null>(null);
const totalPages =
totalMembers !== null ? Math.ceil(totalMembers / pageSize) : null;
const [errorMessage, setErrorMessage] = useState<string | null>(null);

const setError = useCallback((context: string, err: Error) => {
const apiErr = err as APIError;
if (apiErr.aborted) {
return;
}
const message = `${context}: ${err.message}`;
setErrorMessage(message);
}, []);

// Fetch group members when the form loads.
const [errorMessage, setErrorMessage] = useState<string | null>(null);
const [members, setMembers] = useState<MemberRow[] | null>(null);
useEffect(() => {
// istanbul ignore next
Expand All @@ -201,12 +211,12 @@ export default function EditGroupMembersForm({
setTotalMembers(total);
})
.catch(err => {
setErrorMessage(`Failed to fetch group members: ${err.message}`);
setError('Failed to fetch group members', err);
});
return () => {
abort.abort();
};
}, [config.api.readGroupMembers, currentUserid, pageIndex]);
}, [config.api.readGroupMembers, currentUserid, pageIndex, setError]);

const columns: TableColumn<MemberRow>[] = [
{
Expand Down Expand Up @@ -256,7 +266,7 @@ export default function EditGroupMembersForm({
);
} catch (err) {
updateMember(member.userid, { busy: false });
setErrorMessage(err.message);
setError('Failed to remove member', err);
}
};

Expand All @@ -274,7 +284,7 @@ export default function EditGroupMembersForm({
} catch (err) {
const prevRole = member.role;
updateMember(member.userid, { role: prevRole, busy: false });
setErrorMessage(err.message);
setError('Failed to change member role', err);
}
};

Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,13 @@
import { mount, waitFor, waitForElement } from '@hypothesis/frontend-testing';
import {
delay,
mount,
waitFor,
waitForElement,
} from '@hypothesis/frontend-testing';
import { Select } from '@hypothesis/frontend-shared';
import { act } from 'preact/test-utils';

import { APIError } from '../../utils/api';
import { Config } from '../../config';
import {
$imports,
Expand Down Expand Up @@ -201,6 +207,15 @@ describe('EditGroupMembersForm', () => {
);
};

/** Construct an APIError corresponding to an aborted request. */
const abortError = () => {
const abortError = new Error('Aborted');
abortError.name = 'AbortError';
return new APIError('Something went wrong', {
cause: abortError,
});
};

it('fetches and displays members', async () => {
const wrapper = createForm();
assert.calledWith(
Expand Down Expand Up @@ -293,6 +308,19 @@ describe('EditGroupMembersForm', () => {
);
});

// Don't show an error if fetching members is canceled due to a navigation
// (eg. page change) happening during the fetch.
it('does not display error if member fetch is aborted', async () => {
fakeCallAPI.withArgs('/api/groups/1234/members').rejects(abortError());
const wrapper = createForm();

await delay(0);

wrapper.update();
assert.equal(wrapper.find('ErrorNotice').prop('message'), null);
assert.deepEqual(getRenderedUsernames(wrapper), []);
});

it('handles member fetch being canceled', () => {
const wrapper = createForm();
assert.calledWith(fakeCallAPI, '/api/groups/1234/members');
Expand Down Expand Up @@ -371,7 +399,10 @@ describe('EditGroupMembersForm', () => {

await waitForError(wrapper);
const error = wrapper.find('ErrorNotice');
assert.equal(error.prop('message'), 'User not found');
assert.equal(
error.prop('message'),
'Failed to remove member: User not found',
);

// Controls should be re-enabled after saving fails.
assert.include(getRenderedUsernames(wrapper), '@bob');
Expand Down Expand Up @@ -470,7 +501,10 @@ describe('EditGroupMembersForm', () => {
// Wait for the role change to fail. An error should be displayed.
await waitForError(wrapper);
const error = wrapper.find('ErrorNotice');
assert.equal(error.prop('message'), 'Invalid role');
assert.equal(
error.prop('message'),
'Failed to change member role: Invalid role',
);

// The displayed role should revert to the original value.
wrapper.update();
Expand Down

0 comments on commit a8e4855

Please sign in to comment.