From fe145adb65a160128aea7342f2e64a2e0ce41679 Mon Sep 17 00:00:00 2001 From: Olga Bulat Date: Thu, 9 Jan 2025 13:43:31 +0300 Subject: [PATCH] Fix loading labels, results, no result on later pages --- frontend/nuxt.config.ts | 1 + .../components/VContentLink/VContentLink.vue | 22 +----- frontend/src/components/VLoadMore.vue | 10 +-- .../VSearchResultsGrid/VSearchResults.vue | 18 ++++- .../meta/CustomButtonComponents.stories.ts | 13 +++- frontend/src/composables/use-collection.ts | 5 +- .../src/composables/use-i18n-utilities.ts | 15 +++- frontend/src/composables/use-search.ts | 7 +- frontend/src/middleware/search.ts | 6 +- frontend/src/pages/search.vue | 31 ++++---- frontend/src/stores/media/index.ts | 75 ++++++++++++++----- .../composables/use-i18n-utilities.spec.js | 11 +++ .../unit/specs/stores/media-store.spec.ts | 37 +++++---- 13 files changed, 159 insertions(+), 92 deletions(-) diff --git a/frontend/nuxt.config.ts b/frontend/nuxt.config.ts index bf28acf2d57..d45ccb5b7a6 100644 --- a/frontend/nuxt.config.ts +++ b/frontend/nuxt.config.ts @@ -12,6 +12,7 @@ export default defineNuxtConfig({ port: 8443, host: "0.0.0.0", }, + devtools: { enabled: true }, imports: { autoImport: false, }, diff --git a/frontend/src/components/VContentLink/VContentLink.vue b/frontend/src/components/VContentLink/VContentLink.vue index 89cb9c117f2..9b7c6d1d6d0 100644 --- a/frontend/src/components/VContentLink/VContentLink.vue +++ b/frontend/src/components/VContentLink/VContentLink.vue @@ -1,43 +1,27 @@ @@ -62,9 +74,9 @@ const resultCounts = computed(() => mediaStore.resultCountsPerMediaType) diff --git a/frontend/src/components/meta/CustomButtonComponents.stories.ts b/frontend/src/components/meta/CustomButtonComponents.stories.ts index fde4f8534c2..83989d5f986 100644 --- a/frontend/src/components/meta/CustomButtonComponents.stories.ts +++ b/frontend/src/components/meta/CustomButtonComponents.stories.ts @@ -1,5 +1,6 @@ import { h } from "vue" +import { AUDIO, IMAGE } from "#shared/constants/media" import { useMediaStore } from "~/stores/media" import VLoadMore from "~/components/VLoadMore.vue" @@ -11,8 +12,16 @@ const Template: Story = { components: { VLoadMore }, setup() { const mediaStore = useMediaStore() - mediaStore.results.image.page = 1 - mediaStore.results.image.pageCount = 12 + mediaStore.results[AUDIO] = { + ...mediaStore.results[AUDIO], + page: 1, + pageCount: 12, + } + mediaStore.results[IMAGE] = { + ...mediaStore.results[IMAGE], + page: 1, + pageCount: 12, + } return () => h("div", { class: "flex p-4", id: "wrapper" }, [ h(VLoadMore, { diff --git a/frontend/src/composables/use-collection.ts b/frontend/src/composables/use-collection.ts index 208dcdf00da..686df8e3285 100644 --- a/frontend/src/composables/use-collection.ts +++ b/frontend/src/composables/use-collection.ts @@ -38,9 +38,8 @@ export const useCollection = ({ shouldPersistMedia: false, } ) => { - media.value = (await mediaStore.fetchMedia({ - shouldPersistMedia, - })) as ResultType[] + const results = await mediaStore.fetchMedia({ shouldPersistMedia }) + media.value = results.items as ResultType[] creatorUrl.value = media.value.length > 0 ? media.value[0].creator_url : undefined return media.value diff --git a/frontend/src/composables/use-i18n-utilities.ts b/frontend/src/composables/use-i18n-utilities.ts index 3930e1c7599..8605d0fe57c 100644 --- a/frontend/src/composables/use-i18n-utilities.ts +++ b/frontend/src/composables/use-i18n-utilities.ts @@ -1,4 +1,5 @@ import { useNuxtApp } from "#imports" +import type { ComputedRef } from "vue" import { ALL_MEDIA, AUDIO, IMAGE } from "#shared/constants/media" import type { @@ -88,7 +89,7 @@ export function getCountKey(resultsCount: number): keyof KeyCollection { /** * Returns the localized text for the number of search results. */ -export function useI18nResultsCount() { +export function useI18nResultsCount(showLoading?: ComputedRef) { const { t } = useNuxtApp().$i18n const getLocaleFormattedNumber = useGetLocaleFormattedNumber() @@ -98,6 +99,9 @@ export function useI18nResultsCount() { resultsCount: number, searchType: SupportedSearchType ) => { + if (showLoading?.value) { + return "header.loading" + } const countKey = getCountKey(resultsCount) return searchResultKeys[searchType][countKey] } @@ -111,6 +115,9 @@ export function useI18nResultsCount() { query: string, mediaType: SupportedMediaType ) => { + if (showLoading?.value) { + return getLoading() + } return t(getI18nKey(resultsCount, mediaType), { count: resultsCount, localeCount: getLocaleFormattedNumber(resultsCount), @@ -124,6 +131,9 @@ export function useI18nResultsCount() { collectionType: Collection, params: Record | undefined = undefined ) => { + if (showLoading?.value) { + return getLoading() + } const key = collectionKeys[collectionType][mediaType][getCountKey(resultCount)] return t(key, { @@ -139,6 +149,9 @@ export function useI18nResultsCount() { * E.g. "No results", "132 results", "Top 240 results". */ const getI18nCount = (resultsCount: number) => { + if (showLoading?.value) { + return getLoading() + } return t(getI18nKey(resultsCount, ALL_MEDIA), { count: resultsCount, localeCount: getLocaleFormattedNumber(resultsCount), diff --git a/frontend/src/composables/use-search.ts b/frontend/src/composables/use-search.ts index 7d6a53740b6..4aa06464e4e 100644 --- a/frontend/src/composables/use-search.ts +++ b/frontend/src/composables/use-search.ts @@ -73,10 +73,10 @@ export const useSearch = ( return navigateTo(searchPath) } - const isFetching = computed(() => mediaStore.isFetching) const resultsCount = computed(() => mediaStore.resultCount) + const showLoading = computed(() => mediaStore.showLoading) - const { getI18nCount, getLoading } = useI18nResultsCount() + const { getI18nCount } = useI18nResultsCount(showLoading) /** * Additional text at the end of the search bar. * Shows the loading state or result count. @@ -85,9 +85,6 @@ export const useSearch = ( if (searchStore.searchTerm === "") { return "" } - if (isFetching.value) { - return getLoading() - } return getI18nCount(resultsCount.value) }) diff --git a/frontend/src/middleware/search.ts b/frontend/src/middleware/search.ts index 9a14df112c6..1db2621cb82 100644 --- a/frontend/src/middleware/search.ts +++ b/frontend/src/middleware/search.ts @@ -50,7 +50,11 @@ export const searchMiddleware = defineNuxtRouteMiddleware(async (to) => { const results = await mediaStore.fetchMedia() const fetchingError = mediaStore.fetchState.error - if (!results.length && fetchingError && !handledClientSide(fetchingError)) { + if ( + !results.items.length && + fetchingError && + !handledClientSide(fetchingError) + ) { showError(createError(fetchingError)) } } diff --git a/frontend/src/pages/search.vue b/frontend/src/pages/search.vue index a80ab4a0989..9b52218a8ad 100644 --- a/frontend/src/pages/search.vue +++ b/frontend/src/pages/search.vue @@ -14,7 +14,6 @@ import { computed, ref, watch } from "vue" import { watchDebounced } from "@vueuse/core" import { storeToRefs } from "pinia" -import { ALL_MEDIA } from "#shared/constants/media" import { skipToContentTargetId } from "#shared/constants/window" import { areQueriesEqual } from "#shared/utils/search-query-transform" import { handledClientSide, isRetriable } from "#shared/utils/errors" @@ -62,15 +61,7 @@ useHead(() => ({ })) const searchResults = ref( - isSearchTypeSupported(searchType.value) - ? ({ - type: searchType.value, - items: - searchType.value === ALL_MEDIA - ? mediaStore.allMedia - : mediaStore.resultItems[searchType.value], - } as Results) - : null + isSearchTypeSupported(searchType.value) ? mediaStore.searchResults : null ) const fetchMedia = async (payload: { shouldPersistMedia?: boolean } = {}) => { @@ -82,7 +73,9 @@ const fetchMedia = async (payload: { shouldPersistMedia?: boolean } = {}) => { * and there is an error status that will not change if retried, don't re-fetch. */ const shouldNotRefetch = - fetchingError.value !== null && !isRetriable(fetchingError.value) + mediaStore.fetchState.status !== "idle" && + fetchingError.value !== null && + !isRetriable(fetchingError.value) if (shouldNotRefetch) { return } @@ -91,9 +84,9 @@ const fetchMedia = async (payload: { shouldPersistMedia?: boolean } = {}) => { } const media = await mediaStore.fetchMedia(payload) - searchResults.value = { type: searchType.value, items: media } as Results - if (fetchingError.value === null || handledClientSide(fetchingError.value)) { + if (!fetchingError.value || handledClientSide(fetchingError.value)) { + searchResults.value = media return media } return fetchingError.value @@ -135,14 +128,20 @@ await useAsyncData( */ document.getElementById("main-page")?.scroll(0, 0) const res = await fetchMedia() - if (!res || (res && "requestKind" in res)) { - return showError(res ?? createError("No results found")) + if (!res) { + return showError( + createError( + fetchingError.value ?? "Fetch media did not return anything" + ) + ) + } + if ("requestKind" in res) { + return showError(res) } return res }, { server: false, - lazy: true, watch: [shouldFetchSensitiveResults, routeQuery, routePath], } ) diff --git a/frontend/src/stores/media/index.ts b/frontend/src/stores/media/index.ts index 54c5654f507..e1c298c217d 100644 --- a/frontend/src/stores/media/index.ts +++ b/frontend/src/stores/media/index.ts @@ -20,6 +20,7 @@ import type { Media, } from "#shared/types/media" import type { FetchingError, FetchState } from "#shared/types/fetch-state" +import type { Results } from "#shared/types/result" import { warn } from "~/utils/console" import { isSearchTypeSupported, useSearchStore } from "~/stores/search" import { useRelatedMediaStore } from "~/stores/media/related-media" @@ -60,7 +61,9 @@ const areMorePagesAvailable = ({ }: { page: number pageCount: number -}) => page < pageCount +}) => { + return page < pageCount +} export const useMediaStore = defineStore("media", { state: (): MediaState => ({ @@ -138,10 +141,24 @@ export const useMediaStore = defineStore("media", { /** * Search fetching state for selected search type. For 'All content', aggregates - * the values for supported media types. + * the values for supported media types: + * - show fetching state if any of the media types is fetching, even if one of the + * media types has an error. + * - show idle state if all media types are idle. + * - show error state if any of the media types has an error. + * - show success state if all media types are successful, and idle - otherwise. */ fetchState(): FetchState { if (this._searchType === ALL_MEDIA) { + const statuses = supportedMediaTypes.map( + (type) => this.mediaFetchState[type].status + ) + if (statuses.includes("fetching")) { + return { status: "fetching", error: null } + } + if (statuses.every((s) => s === "idle")) { + return { status: "idle", error: null } + } /** * Returns a combined error for all media types. * @@ -180,16 +197,10 @@ export const useMediaStore = defineStore("media", { if (error) { return { status: "error", error } } - const status = supportedMediaTypes.some( - (type) => this.mediaFetchState[type].status === "fetching" - ) - ? "fetching" - : supportedMediaTypes.some( - (type) => this.mediaFetchState[type].status === "success" - ) - ? "success" - : "idle" - return { status, error: null } + return { + status: statuses.includes("idle") ? "idle" : "success", + error: null, + } } else if (isSearchTypeSupported(this._searchType)) { return this.mediaFetchState[this._searchType] } else { @@ -201,6 +212,13 @@ export const useMediaStore = defineStore("media", { return this.fetchState.status === "fetching" }, + showLoading(): boolean { + return ( + this.fetchState.status === "idle" || + (this.fetchState.status === "fetching" && this.currentPage < 2) + ) + }, + /** * Returns a mixed bag of search results across media types. * @@ -292,6 +310,21 @@ export const useMediaStore = defineStore("media", { }) }, + /** + * Returns the search results array for the current search type. + */ + searchResults(): Results { + const searchType = this._searchType + + if (searchType === ALL_MEDIA) { + return { type: ALL_MEDIA, items: [...this.allMedia] } as const + } else if (searchType === IMAGE) { + return { type: IMAGE, items: this.resultItems[IMAGE] } as const + } else { + return { type: AUDIO, items: this.resultItems[AUDIO] } as const + } + }, + /** * Used to display the load more button in the UI. * For all the media types that can be fetched for the current search type @@ -299,8 +332,10 @@ export const useMediaStore = defineStore("media", { * that the first page of the results has been fetched and that the API has more pages. */ canLoadMore(): boolean { - return this._fetchableMediaTypes.every( - (type) => this.results[type].pageCount !== 0 + const types = this._fetchableMediaTypes + return ( + types.length > 0 && + types.every((type) => this.results[type].pageCount !== 0) ) }, }, @@ -309,6 +344,7 @@ export const useMediaStore = defineStore("media", { _startFetching(mediaType: SupportedMediaType) { this.mediaFetchState[mediaType] = { status: "fetching", error: null } }, + /** * Called when the request is finished, regardless of whether it was successful or not. * @param mediaType - The media type for which the request was made. @@ -410,8 +446,9 @@ export const useMediaStore = defineStore("media", { * If the search query changed, fetch state is reset, otherwise only the media types for which * fetchState.isFinished is not true are fetched. */ - async fetchMedia(payload: { shouldPersistMedia?: boolean } = {}) { - const mediaType = this._searchType + async fetchMedia( + payload: { shouldPersistMedia?: boolean } = {} + ): Promise { const shouldPersistMedia = Boolean(payload.shouldPersistMedia) await Promise.allSettled( @@ -420,9 +457,7 @@ export const useMediaStore = defineStore("media", { ) ) - return mediaType === ALL_MEDIA - ? this.allMedia - : this.resultItems[mediaType] + return this.searchResults }, clearMedia() { @@ -482,7 +517,7 @@ export const useMediaStore = defineStore("media", { * When there are no results for a query, the API returns a 200 response. * In such cases, we show the "No results" client error page. */ - if (!mediaCount) { + if (!mediaCount && page < 2) { page = 1 errorData = { message: `No results found for ${queryParams.q}`, diff --git a/frontend/test/unit/specs/composables/use-i18n-utilities.spec.js b/frontend/test/unit/specs/composables/use-i18n-utilities.spec.js index 5f396bad87e..31c3552585d 100644 --- a/frontend/test/unit/specs/composables/use-i18n-utilities.spec.js +++ b/frontend/test/unit/specs/composables/use-i18n-utilities.spec.js @@ -1,3 +1,5 @@ +import { computed } from "vue" + import { useI18nResultsCount } from "~/composables/use-i18n-utilities" describe("i18nResultsCount", () => { @@ -16,4 +18,13 @@ describe("i18nResultsCount", () => { expect(result).toEqual(expectedResult) } ) + + it("Shows loading message", () => { + const showLoading = computed(() => true) + const { getI18nCount } = useI18nResultsCount(showLoading) + + const result = getI18nCount(240) + + expect(result).toEqual("Loading...") + }) }) diff --git a/frontend/test/unit/specs/stores/media-store.spec.ts b/frontend/test/unit/specs/stores/media-store.spec.ts index 516b6ca97cc..7acb9675487 100644 --- a/frontend/test/unit/specs/stores/media-store.spec.ts +++ b/frontend/test/unit/specs/stores/media-store.spec.ts @@ -244,22 +244,29 @@ describe("media store", () => { }) it.each` - searchType | audioError | fetchState - ${ALL_MEDIA} | ${{ code: NO_RESULT }} | ${{ error: null, status: "fetching" }} - ${ALL_MEDIA} | ${{ statusCode: 429 }} | ${{ error: { requestKind: "search", statusCode: 429, searchType: ALL_MEDIA }, status: "error" }} + fetchingMediaType | error | fetchState + ${AUDIO} | ${{ code: NO_RESULT }} | ${{ error: null, status: "fetching" }} + ${IMAGE} | ${null} | ${{ error: null, status: "fetching" }} `( - "fetchState for $searchType returns $fetchState", - ({ searchType, audioError, fetchState }) => { + "fetchState for ALL_MEDIA returns `fetching` even if at least one media type is fetching", + ({ fetchingMediaType, error, fetchState }) => { const mediaStore = useMediaStore() const searchStore = useSearchStore() - searchStore.searchType = searchType - const audioFetchError = { - requestKind: "search", - searchType: AUDIO, - ...audioError, - } - mediaStore.updateFetchState(IMAGE, "start") - mediaStore.updateFetchState(AUDIO, "end", audioFetchError) + searchStore.searchType = ALL_MEDIA + const fetchError = error + ? { + requestKind: "search", + searchType: AUDIO, + ...error, + } + : null + supportedMediaTypes.forEach((mediaType) => { + if (mediaType === fetchingMediaType) { + mediaStore.updateFetchState(mediaType, "start") + } else { + mediaStore.updateFetchState(mediaType, "end", fetchError) + } + }) expect(mediaStore.fetchState).toEqual(fetchState) } @@ -507,7 +514,7 @@ describe("media store", () => { const mediaStore = useMediaStore() const media = await mediaStore.fetchMedia() - expect(media.length).toEqual(6) + expect(media.items.length).toEqual(6) expect(mocks.createApiClient).toHaveBeenCalledWith({ accessToken: undefined, @@ -534,7 +541,7 @@ describe("media store", () => { const mediaStore = useMediaStore() const media = await mediaStore.fetchMedia() - expect(media.length).toEqual(4) + expect(media.items.length).toEqual(4) expect(searchMock).toHaveBeenCalledTimes(1) expect(searchMock).toHaveBeenCalledWith("image", { q: "cat" }) expect(mediaStore.currentPage).toEqual(1)