Skip to content

Commit

Permalink
Fix fetching search results and handle fetching errors (#2618)
Browse files Browse the repository at this point in the history
* Fix search results fetching and error handling

* Move to useFetch composable

Improve comments and code readability

* Add unit tests

* Remove unused tapes

* Correct NO_RESULT handling

* Add tapes

* Update unit test
  • Loading branch information
obulat authored Jul 28, 2023
1 parent c31c6a2 commit a102809
Show file tree
Hide file tree
Showing 24 changed files with 458 additions and 481 deletions.
21 changes: 12 additions & 9 deletions frontend/src/components/VSearchGrid.vue
Original file line number Diff line number Diff line change
@@ -1,11 +1,5 @@
<template>
<section
v-if="
!fetchState.hasStarted ||
fetchState.isFetching ||
(!fetchState.isFetching && resultsCount)
"
>
<section v-if="!showError">
<header v-if="query.q && supported" class="my-0 md:mb-8 md:mt-4">
<VSearchResultsTitle :size="isAllView ? 'large' : 'default'">
{{ searchTerm }}
Expand Down Expand Up @@ -41,9 +35,9 @@ import { computed, defineComponent, PropType } from "vue"
import {
ALL_MEDIA,
IMAGE,
SearchType,
isSupportedMediaType,
isAdditionalSearchType,
isSupportedMediaType,
SearchType,
} from "~/constants/media"
import { NO_RESULT } from "~/constants/errors"
import { defineEvent } from "~/types/emits"
Expand Down Expand Up @@ -130,13 +124,22 @@ export default defineComponent({
const searchTerm = computed(() => props.query.q || "")
const showError = computed(() => {
return (
props.fetchState.hasStarted &&
!props.fetchState.isFetching &&
props.resultsCount === 0
)
})
return {
hasNoResults,
externalSourcesType,
isAllView,
NO_RESULT,
externalSources,
searchTerm,
showError,
}
},
})
Expand Down
43 changes: 34 additions & 9 deletions frontend/src/middleware/search.ts
Original file line number Diff line number Diff line change
@@ -1,11 +1,20 @@
import { useSearchStore } from "~/stores/search"
import { useMediaStore } from "~/stores/media"
import { NO_RESULT } from "~/constants/errors"

import type { Middleware } from "@nuxt/types"

export const searchMiddleware: Middleware = ({ redirect, route, $pinia }) => {
export const searchMiddleware: Middleware = async ({
redirect,
route,
$pinia,
error: nuxtError,
}) => {
const {
query: { q },
query: { q: rawQ },
} = route
const q = Array.isArray(rawQ) ? rawQ[0] : rawQ

/**
* This middleware redirects any search without a query to the homepage.
* This is meant to block direct access to /search and all sub-routes.
Expand All @@ -19,13 +28,29 @@ export const searchMiddleware: Middleware = ({ redirect, route, $pinia }) => {
* Note that the search by creator is not displayed in the UI.
*/
if (!q) return redirect("/")
/**
* We need to make sure that query `q` exists before checking if it matches
* the store searchTerm.
*/

const searchStore = useSearchStore($pinia)
const querySearchTerm = Array.isArray(q) ? q[0] : q
if (querySearchTerm !== searchStore.searchTerm) {
searchStore.setSearchTerm(querySearchTerm)

await searchStore.initProviderFilters()

searchStore.setSearchStateFromUrl({
path: route.path,
urlQuery: route.query,
})

// Fetch results before rendering the page on the server.
if (process.server) {
const mediaStore = useMediaStore($pinia)
const results = await mediaStore.fetchMedia()

const fetchingError = mediaStore.fetchState.fetchingError
// NO_RESULTS is handled client-side, for other errors show server error page
if (
!results &&
fetchingError &&
!fetchingError?.message?.includes(NO_RESULT)
) {
nuxtError(fetchingError)
}
}
}
79 changes: 48 additions & 31 deletions frontend/src/pages/search.vue
Original file line number Diff line number Diff line change
Expand Up @@ -35,11 +35,18 @@
import { isShallowEqualObjects } from "@wordpress/is-shallow-equal"
import { computed, inject, watch } from "vue"
import { storeToRefs } from "pinia"
import { defineComponent, useMeta, useRoute } from "@nuxtjs/composition-api"
import {
defineComponent,
useContext,
useFetch,
useMeta,
useRoute,
} from "@nuxtjs/composition-api"
import { searchMiddleware } from "~/middleware/search"
import { useMediaStore } from "~/stores/media"
import { useSearchStore } from "~/stores/search"
import { NO_RESULT } from "~/constants/errors"
import { skipToContentTargetId } from "~/constants/window"
import { IsSidebarVisibleKey, ShowScrollButtonKey } from "~/types/provides"
Expand All @@ -54,6 +61,7 @@ export default defineComponent({
},
layout: "search-layout",
middleware: searchMiddleware,
fetchOnServer: false,
setup() {
const showScrollButton = inject(ShowScrollButtonKey)
const isSidebarVisible = inject(IsSidebarVisibleKey)
Expand All @@ -79,39 +87,70 @@ export default defineComponent({
const { resultCount, fetchState, resultItems } = storeToRefs(mediaStore)
const needsFetching = computed(() =>
Boolean(
supported.value && !resultCount.value && searchTerm.value.trim() !== ""
)
Boolean(supported.value && !resultCount.value && searchTerm.value !== "")
)
useMeta({
title: `${searchTerm.value} | Openverse`,
meta: [{ hid: "robots", name: "robots", content: "all" }],
})
const { error: nuxtError } = useContext()
const fetchMedia = async (
payload: { shouldPersistMedia?: boolean } = {}
) => {
return mediaStore.fetchMedia(payload)
/**
* If the fetch has already started in the middleware,
* and there is an error or no results were found, don't re-fetch.
*/
const shouldNotRefetch =
mediaStore.fetchState.fetchingError?.message &&
mediaStore.fetchState.hasStarted
if (shouldNotRefetch) return
const results = await mediaStore.fetchMedia(payload)
if (!results) {
const error = mediaStore.fetchState.fetchingError
/**
* NO_RESULT error is handled by the VErrorSection component in the child pages.
* For all other errors, show the Nuxt error page.
*/
if (error?.statusCode && !error?.message.includes(NO_RESULT))
return nuxtError(mediaStore.fetchState.fetchingError)
}
}
/**
* Search middleware runs when the path changes. This watcher
* is necessary to handle the query changes.
*
* It updates the search store state from the URL query,
* fetches media, and scrolls to top if necessary.
*/
watch(route, async (newRoute, oldRoute) => {
/**
* Updates the search type only if the route's path changes.
* Scrolls `main-page` to top if the path changes.
*/
if (
newRoute.path !== oldRoute.path ||
!isShallowEqualObjects(newRoute.query, oldRoute.query)
) {
const { query: urlQuery, path } = newRoute
searchStore.setSearchStateFromUrl({ urlQuery, path })
/**
* By default, Nuxt only scrolls to top when the path changes.
* This is a workaround to scroll to top when the query changes.
*/
document.getElementById("main-page")?.scroll(0, 0)
await fetchMedia()
}
})
useFetch(async () => {
if (needsFetching.value) {
await fetchMedia()
}
})
return {
showScrollButton,
searchTerm,
Expand All @@ -126,28 +165,6 @@ export default defineComponent({
isSidebarVisible,
skipToContentTargetId,
fetchMedia,
}
},
/**
* asyncData blocks the rendering of the page, so we only
* update the state from the route here, and do not fetch media.
*/
async asyncData({ route, $pinia }) {
const searchStore = useSearchStore($pinia)
await searchStore.initProviderFilters()
searchStore.setSearchStateFromUrl({
path: route.path,
urlQuery: route.query,
})
},
/**
* Fetch media, if necessary, in a non-blocking way.
*/
async fetch() {
if (this.needsFetching) {
await this.fetchMedia()
}
},
head: {},
Expand Down
Loading

0 comments on commit a102809

Please sign in to comment.