Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
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
8 changes: 6 additions & 2 deletions src/components/CitationExporter/CitationExporter.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ const Exporter = (props: ICitationExporterProps): ReactElement => {
...divProps
} = props;

const { data, state, dispatch } = useCitationExporter({
const { data, state, dispatch, isLoading } = useCitationExporter({
format: initialFormat,
authorcutoff,
keyformat,
Expand All @@ -98,7 +98,6 @@ const Exporter = (props: ICitationExporterProps): ReactElement => {
sort,
});
const ctx = state.context;
const isLoading = state.matches('fetching');
const router = useRouter();

const { isValidFormat } = useExportFormats();
Expand All @@ -120,6 +119,9 @@ const Exporter = (props: ICitationExporterProps): ReactElement => {
},
);
}
// one-way format->route sync; adding state/router/singleMode would revert
// user-initiated format changes (see machine sync note)
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [state.value, router.query, ctx.params.format]);

// Attempt to parse the url to grab the format, then update it, otherwise allow the server to handle the path
Expand All @@ -140,6 +142,8 @@ const Exporter = (props: ICitationExporterProps): ReactElement => {
return true;
});
return () => router.beforePopState(() => true);
// isValidFormat is stable; re-register only on dispatch/router change
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [dispatch, router]);

const handleOnSubmit: ChangeEventHandler<HTMLFormElement> = (e) => {
Expand Down
15 changes: 14 additions & 1 deletion src/components/CitationExporter/useCitationExporter.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { act } from '@testing-library/react';
import { describe, expect, test, vi } from 'vitest';

import { ExportApiFormatKey } from '@/api/export/types';
import { renderHook } from '@/test-utils';
import { renderHook, waitFor } from '@/test-utils';

import { useCitationExporter } from './useCitationExporter';

Expand Down Expand Up @@ -47,3 +47,16 @@ describe('useCitationExporter — prop ↔ machine sync', () => {
expect(result.current.state.context.params.format).toBe(ExportApiFormatKey.ris);
});
});

describe('useCitationExporter — initial loading', () => {
test('reports isLoading during the on-mount prefetch when nothing is cached', async () => {
const { result } = renderHook(() => useCitationExporter(baseProps));

// with no cached export, the initial prefetch is in flight: the consumer
// should see a loading state (not the idle "press submit" placeholder)
expect(result.current.isLoading).toBe(true);

// once the prefetch settles, loading clears
await waitFor(() => expect(result.current.isLoading).toBe(false));
});
});
45 changes: 29 additions & 16 deletions src/components/CitationExporter/useCitationExporter.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { useMachine } from '@xstate/react/fsm';
import { useEffect, useMemo } from 'react';
import { useEffect, useMemo, useState } from 'react';
import { useQueryClient } from '@tanstack/react-query';
import { generateMachine, ICitationExporterState } from './CitationExporter.machine';
import { purifyString } from '@/utils/common/formatters';
Expand Down Expand Up @@ -53,6 +53,12 @@ export const useCitationExporter = ({
const [state, dispatch] = useMachine(machine);
const queryClient = useQueryClient();

// True while the on-mount prefetch below is in flight. The machine stays in
// `idle` during that prefetch (a guarded SUBMIT cannot transition on mount),
// so without this flag the result area would show its static "press submit"
// placeholder for the entire initial fetch instead of a loading indicator.
const [isInitialLoading, setIsInitialLoading] = useState(false);

// clean params before submitting to API
const params: IExportApiParams = {
...state.context.params,
Expand All @@ -63,20 +69,24 @@ export const useCitationExporter = ({
// subsequent renders — doing so would re-trigger the initial load on every
// format/record change, bypassing the state machine's normal flow.
useEffect(() => {
(async () => {
const queryKey = exportCitationKeys.primary(params);
const cached = queryClient.getQueryData<IExportApiParams>(queryKey);

if (!cached) {
// no cached value, prefetch and submit it here since we know the params
await queryClient.prefetchQuery({
queryKey,
queryFn: fetchExportCitation,
meta: { params },
});
dispatch('SUBMIT');
}
})();
let cancelled = false;
const queryKey = exportCitationKeys.primary(params);
const cached = queryClient.getQueryData(queryKey);

if (!cached) {
// no cached value: surface a loading state, prefetch, then submit
setIsInitialLoading(true);
void queryClient.prefetchQuery({ queryKey, queryFn: fetchExportCitation, meta: { params } }).finally(() => {
if (!cancelled) {
setIsInitialLoading(false);
dispatch('SUBMIT');
}
});
}

return () => {
cancelled = true;
};
}, []); // eslint-disable-line react-hooks/exhaustive-deps

// trigger updates to machine state if incoming props change
Expand Down Expand Up @@ -141,5 +151,8 @@ export const useCitationExporter = ({
}
}, [state.value, result.data, dispatch]);

return { ...result, state, dispatch };
// `isLoading` reflects both the machine's fetching state and the on-mount
// prefetch, so the result area shows a loading indicator throughout the
// initial load rather than the static "press submit" placeholder.
return { ...result, state, dispatch, isLoading: state.matches('fetching') || isInitialLoading };
};
19 changes: 17 additions & 2 deletions src/components/ResultList/ListActions.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,8 @@ export const ListActions = (props: IListActionsProps): ReactElement => {
});
setPath(null);
}
// runs on vault result/path change; router/toast/clearSelected are stable
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [data, error, path]);

const handleExploreOption = (value: string | string[]) => {
Expand Down Expand Up @@ -165,6 +167,8 @@ export const ListActions = (props: IListActionsProps): ReactElement => {
}
};

// keyed on exploreAll/router; handleOperationsLink is re-created each render
// eslint-disable-next-line react-hooks/exhaustive-deps
const handleOpsLink = useCallback((name: Operator) => () => handleOperationsLink(name), [exploreAll, router]);

const colors = useColorModeColors();
Expand Down Expand Up @@ -441,7 +445,11 @@ const ExportMenu = (props: MenuGroupProps & { exploreAll: boolean; defaultExport
const defaultExportFormatValue = getFormatOptionByLabel(defaultExportFormat).value;

useEffect(() => {
if (data) {
// Only use a vault qid in 'selected' mode. A disabled vault query keeps its
// cached data, so in 'all' mode `data` may still hold the qid from a prior
// subselection — navigating with it would re-export just that subset. The
// route[0] guard ensures we only navigate in response to an export click.
if (!exploreAll && data && isNonEmptyString(route[0])) {
setSelected([]);

// when vault query is done, transition to the export page passing only qid
Expand All @@ -450,9 +458,14 @@ const ExportMenu = (props: MenuGroupProps & { exploreAll: boolean; defaultExport
{ pathname: route[1], query: { ...router.query, qid: data.qid } },
);
}
// Runs only on `data`/`route` change, using the latest render's
// `exploreAll`. It's intentionally not a dep: adding it would re-run on a
// mode toggle and could navigate with a stale qid (route[0] persists from a
// prior click). The `!exploreAll` guard keeps the qid to 'selected' mode.
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [data, route]);

// on route change
// on route change (export click)
useEffect(() => {
if (!exploreAll) {
// if using a selection, update the state value (triggers a vault request)
Expand All @@ -463,6 +476,8 @@ const ExportMenu = (props: MenuGroupProps & { exploreAll: boolean; defaultExport
// if explore all, then just use the current query, and do not trigger vault (redirect immediately)
void router.push({ pathname: route[0], query: router.query }, { pathname: route[1], query: router.query });
}
// runs only on export click (route change); router/store are stable
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [route]);

const handleExportItemClick = curryN(2, (format: string) => {
Expand Down
98 changes: 84 additions & 14 deletions src/components/ResultList/__tests__/ListActions.test.tsx
Original file line number Diff line number Diff line change
@@ -1,19 +1,24 @@
import { render } from '@/test-utils';
import { describe, expect, test, vi } from 'vitest';
import { render, waitFor } from '@/test-utils';
import { beforeEach, describe, expect, test, vi } from 'vitest';
import { ListActions } from '../ListActions';

const mocks = vi.hoisted(() => ({
useRouter: vi.fn(() => ({
query: { q: 'test query' },
asPath: '/search?q=test%20query',
push: vi.fn(),
events: { on: vi.fn(), off: vi.fn() },
})),
useSession: vi.fn(() => ({
isAuthenticated: false,
logout: vi.fn(),
})),
}));
const mocks = vi.hoisted(() => {
const push = vi.fn();
return {
push,
useRouter: vi.fn(() => ({
query: { q: 'test query' },
asPath: '/search?q=test%20query',
pathname: '/search',
push,
events: { on: vi.fn(), off: vi.fn() },
})),
useSession: vi.fn(() => ({
isAuthenticated: false,
logout: vi.fn(),
})),
};
});

vi.mock('next/router', () => ({ useRouter: mocks.useRouter }));
vi.mock('@/lib/useSession', () => ({ useSession: mocks.useSession }));
Expand Down Expand Up @@ -59,3 +64,68 @@ describe('ListActions notification bell button', () => {
});
});
});

describe('ListActions export selection handling', () => {
const defaultProps = {
onSortChange: vi.fn(),
onOpenAddToLibrary: vi.fn(),
isLoading: false,
};

const selectedStore = {
docs: {
doc: '',
current: ['2020aaa..1', '2020bbb..2'],
selected: ['2020aaa..1', '2020bbb..2'],
isAllSelected: true,
isSomeSelected: true,
},
};

beforeEach(() => {
mocks.push.mockClear();
mocks.useSession.mockReturnValue({ isAuthenticated: false, logout: vi.fn() });
});

test('exporting a subselection navigates with a qid and preserves the selection', async () => {
const { user, getByRole, getByText, getByTestId } = render(<ListActions {...defaultProps} />, {
initialStore: selectedStore,
});

// a subselection is active, so the control is in "selected" mode
expect(getByTestId('listactions-selected')).toHaveTextContent('2 Selected');

await user.click(getByRole('button', { name: /Bulk Actions/ }));
await user.click(getByText('in BibTeX'));

// the vault request resolves and we navigate with a qid for the subset
await waitFor(() => expect(mocks.push).toHaveBeenCalled());
expect(mocks.push.mock.calls[0][0].query).toHaveProperty('qid');

// the selection is preserved (not cleared) so the user can act on it again
expect(getByTestId('listactions-selected')).toHaveTextContent('2 Selected');
});

test('switching to "All" after a subselection export omits the stale qid', async () => {
const { user, getByRole, getByText } = render(<ListActions {...defaultProps} />, {
initialStore: selectedStore,
});

// export the current subselection -> navigates with a qid
await user.click(getByRole('button', { name: /Bulk Actions/ }));
await user.click(getByText('in BibTeX'));
await waitFor(() => expect(mocks.push).toHaveBeenCalled());
expect(mocks.push.mock.calls[0][0].query).toHaveProperty('qid');

// switch to "All" (selection still present) and export again. Only the Bulk
// Actions menu is open, so the single visible "All" radio is unambiguous.
mocks.push.mockClear();
await user.click(getByRole('button', { name: /Bulk Actions/ }));
await user.click(getByRole('menuitemradio', { name: 'All' }));
await user.click(getByText('in BibTeX'));

// the "all" export must use the plain query, not the previous subset qid
await waitFor(() => expect(mocks.push).toHaveBeenCalled());
expect(mocks.push.mock.calls[0][0].query).not.toHaveProperty('qid');
});
});
28 changes: 12 additions & 16 deletions src/pages/search/exportcitation/[format].tsx
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import { useSettings } from '@/lib/useSettings';
import { logger } from '@/logger';
import { SimpleLink } from '@/components/SimpleLink';
import { CitationExporter } from '@/components/CitationExporter';
import { ExportContainer } from '@/components/CitationExporter/components/ExportContainer';
import { JournalFormatMap } from '@/components/Settings';
import { parseQueryFromUrl } from '@/utils/common/search';
import { unwrapStringValue } from '@/utils/common/formatters';
Expand Down Expand Up @@ -61,14 +62,9 @@ const ExportCitationPage: NextPage<IExportCitationPageProps> = (props) => {
const router = useRouter();
const { data, fetchNextPage, hasNextPage, error } = useSearchInfinite(query);

// TODO: add more error handling here
if (!data) {
return null;
}

const res = last(data?.pages).response;
const records = res.docs.map((d) => d.bibcode);
const numFound = res.numFound;
const res = data ? last(data.pages).response : null;
const records = res ? res.docs.map((d) => d.bibcode) : [];
const numFound = res?.numFound ?? 0;

const handleNextPage = () => {
void fetchNextPage();
Expand Down Expand Up @@ -101,7 +97,14 @@ const ExportCitationPage: NextPage<IExportCitationPageProps> = (props) => {
<AlertIcon />
{error.message}
</Alert>
) : isClient ? (
) : !data || !isClient ? (
// Render a loading state until the data is ready AND we are on the
// client. Gating the interactive vs. static exporter on isClient
// otherwise renders the simplified static version on the first
// (server) render, then visibly swaps to the full version after
// hydration.
<ExportContainer header={<>Loading records&hellip;</>} isLoading />
) : (
<CitationExporter
initialFormat={format}
keyformat={keyformat}
Expand All @@ -115,13 +118,6 @@ const ExportCitationPage: NextPage<IExportCitationPageProps> = (props) => {
page={data.pages.length - 1}
sort={query.sort}
/>
) : (
<CitationExporter.Static
initialFormat={format}
records={records}
totalRecords={numFound}
sort={query.sort}
/>
)}
</Box>
</Flex>
Expand Down
Loading