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

Simplify media FetchState and fix fetching errors #5323

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

obulat
Copy link
Contributor

@obulat obulat commented Jan 8, 2025

Fixes

Fixes #5322
Fixes #5325

Description

This PR replaces the flags used in FetchState with single status property, and an additional error property. To prevent invalid states where status is not error, but there's error, the type uses TypeScript discriminated union.

Some other changes:

  • getters for the derived states added or updated in the media store to decide:
    • whether to show the load more button
    • whether to show the result count or "Loading" in the results labels
  • fetchMedia returns the Results type that is a discriminated union (so, the type property determines the type of the items in the items list)
  • moved the canLoadMore and i18n label computation out of the component to the parent components. This simplifies the component rendering in the Storybook.

Testing instructions

Run the app using ov just frontend/run dev
Search for "bamberg" and try the steps in #5325 - the errors from the issue should be fixed.
Fetching the media should work properly, both in the client and in SSR.
When the first page of results is loading on All media content, the content links should not say "No results", but should show "Loading..." label instead.


I tried creating a PR completely using the GitHub copilot, but I had to add a lot of changes on the first version: Copilot Workspace session.

@obulat obulat requested a review from a team as a code owner January 8, 2025 10:22
@obulat obulat requested review from krysal and dhruvkb January 8, 2025 10:22
@obulat obulat marked this pull request as draft January 8, 2025 10:22
@openverse-bot openverse-bot added 🧱 stack: frontend Related to the Nuxt frontend 🟨 priority: medium Not blocking but should be addressed soon 🧰 goal: internal improvement Improvement that benefits maintainers, not users 💻 aspect: code Concerns the software code in the repository labels Jan 8, 2025
@obulat obulat force-pushed the obulat/refactor-fetchstate branch 2 times, most recently from b31b1f9 to af9a134 Compare January 8, 2025 18:37
Copy link

github-actions bot commented Jan 8, 2025

Latest k6 run output1

     ✓ status was 200

     checks.........................: 100.00% ✓ 406      ✗ 0   
     data_received..................: 98 MB   405 kB/s
     data_sent......................: 53 kB   220 B/s
     http_req_blocked...............: avg=73.84µs  min=2.08µs   med=4.76µs   max=1.24ms   p(90)=135.75µs p(95)=656.18µs
     http_req_connecting............: avg=56.33µs  min=0s       med=0s       max=1.19ms   p(90)=93.07µs  p(95)=199.79µs
     http_req_duration..............: avg=160.92ms min=18.64ms  med=99.12ms  max=1.05s    p(90)=369.7ms  p(95)=475.9ms 
       { expected_response:true }...: avg=160.92ms min=18.64ms  med=99.12ms  max=1.05s    p(90)=369.7ms  p(95)=475.9ms 
   ✓ http_req_failed................: 0.00%   ✓ 0        ✗ 406 
     http_req_receiving.............: avg=169.55µs min=59.17µs  med=145.24µs max=579.04µs p(90)=277.94µs p(95)=351.22µs
     http_req_sending...............: avg=28.21µs  min=8.14µs   med=22.8µs   max=129.75µs p(90)=42.16µs  p(95)=85.32µs 
     http_req_tls_handshaking.......: avg=0s       min=0s       med=0s       max=0s       p(90)=0s       p(95)=0s      
     http_req_waiting...............: avg=160.72ms min=18.5ms   med=99ms     max=1.05s    p(90)=369.47ms p(95)=475.69ms
     http_reqs......................: 406     1.686162/s
     iteration_duration.............: avg=866.76ms min=283.53ms med=903.78ms max=1.67s    p(90)=1.2s     p(95)=1.45s   
     iterations.....................: 76      0.315636/s
     vus............................: 1       min=0      max=6 
     vus_max........................: 60      min=60     max=60

Footnotes

  1. This comment will automatically update with new output each time k6 runs for this PR

@obulat obulat force-pushed the obulat/refactor-fetchstate branch 2 times, most recently from fda6fbe to 467d6e1 Compare January 9, 2025 05:11
@obulat obulat force-pushed the obulat/refactor-fetchstate branch from 467d6e1 to aa359a0 Compare January 9, 2025 07:35
@obulat obulat changed the title Refactor FetchState to use discriminated union Simplify media FetchState and fix fetching errors Jan 9, 2025
@obulat obulat force-pushed the obulat/refactor-fetchstate branch 2 times, most recently from 3887eae to 4507ed1 Compare January 9, 2025 17:43
@obulat obulat force-pushed the obulat/refactor-fetchstate branch from 4507ed1 to fe145ad Compare January 10, 2025 05:09
Copy link

Full-stack documentation: https://docs.openverse.org/_preview/5323

Please note that GitHub pages takes a little time to deploy newly pushed code, if the links above don't work or you see old versions, wait 5 minutes and try again.

You can check the GitHub pages deployment action list to see the current status of the deployments.

@openverse-bot openverse-bot added the 🧱 stack: documentation Related to Sphinx documentation label Jan 10, 2025
@obulat obulat force-pushed the obulat/refactor-fetchstate branch from 24ceaf2 to 011fb6e Compare January 10, 2025 08:18
@obulat obulat self-assigned this Jan 10, 2025
@@ -1,7 +1,7 @@
import type { StorybookConfig } from "@storybook-vue/nuxt"

const config: StorybookConfig = {
stories: ["../src/**/*.mdx", "../src/**/*.stories.@(js|jsx|mjs|ts|tsx)"],
stories: ["../src/**/*.stories.@(js|jsx|mjs|ts|tsx)"],
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't have any .mdx files, so Storybook was always printing warnings about this pattern.

@@ -95,6 +95,10 @@ export const AllCollections: Omit<Story, "args"> = {
const mediaStore = useMediaStore()
mediaStore.$patch({
results: { image: { count: 240 } },
mediaFetchState: {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The image count label is blank when the status is "idle" (the default value before fetching the media for the collection starts)

@obulat obulat force-pushed the obulat/refactor-fetchstate branch from f6004ce to eed90eb Compare January 10, 2025 12:32
@obulat obulat marked this pull request as ready for review January 10, 2025 13:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💻 aspect: code Concerns the software code in the repository 🧰 goal: internal improvement Improvement that benefits maintainers, not users 🟨 priority: medium Not blocking but should be addressed soon 🧱 stack: documentation Related to Sphinx documentation 🧱 stack: frontend Related to the Nuxt frontend
Projects
Status: 👀 Needs Review
2 participants