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.

Sign up for GitHub

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

Jump to bottom

Simplify media FetchState and fix fetching errors #5323

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

Simplify media FetchState and fix fetching errors #5323

wants to merge 6 commits into from

Conversation

obulat
Copy link
Contributor

@obulat obulat commented Jan 8, 2025 •
edited
Loading

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
@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 8, 2025 •
edited
Loading

Latest k6 run output1

     ✓ status was 200

     checks.........................: 100.00% ✓ 410      ✗ 0   
     data_received..................: 98 MB   408 kB/s
     data_sent......................: 54 kB   222 B/s
     http_req_blocked...............: avg=34.47µs  min=2.14µs   med=4.65µs   max=265.6µs  p(90)=154.67µs p(95)=171.14µs
     http_req_connecting............: avg=21.47µs  min=0s       med=0s       max=195.81µs p(90)=103.42µs p(95)=118.73µs
     http_req_duration..............: avg=165.47ms min=17.8ms   med=118.19ms max=1.04s    p(90)=350.97ms p(95)=443.53ms
       { expected_response:true }...: avg=165.47ms min=17.8ms   med=118.19ms max=1.04s    p(90)=350.97ms p(95)=443.53ms
   ✓ http_req_failed................: 0.00%   ✓ 0        ✗ 410 
     http_req_receiving.............: avg=174.86µs min=59.25µs  med=148.67µs max=649.91µs p(90)=284.37µs p(95)=341.37µs
     http_req_sending...............: avg=24.89µs  min=9.96µs   med=23.55µs  max=65.48µs  p(90)=34.91µs  p(95)=43.35µs 
     http_req_tls_handshaking.......: avg=0s       min=0s       med=0s       max=0s       p(90)=0s       p(95)=0s      
     http_req_waiting...............: avg=165.27ms min=17.69ms  med=118.04ms max=1.04s    p(90)=350.67ms p(95)=443.3ms 
     http_reqs......................: 410     1.701362/s
     iteration_duration.............: avg=899.31ms min=424.51ms med=956.62ms max=1.79s    p(90)=1.18s    p(95)=1.51s   
     iterations.....................: 76      0.315375/s
     vus............................: 3       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 3 times, most recently 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 3 times, most recently from 4507ed1 to fe145ad Compare January 10, 2025 05:09
@github-actions GitHub Actions
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
obulat
obulat commented Jan 10, 2025
View reviewed changes
frontend/.storybook/main.ts Outdated Show resolved Hide resolved
obulat
obulat commented Jan 10, 2025
View reviewed changes
frontend/src/components/VCollectionHeader/meta/VCollectionHeader.stories.ts
@@ -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
@obulat obulat force-pushed the obulat/refactor-fetchstate branch from eed90eb to 2cc2ad8 Compare January 10, 2025 18:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@krysal krysal Awaiting requested review from krysal krysal is a code owner automatically assigned from WordPress/openverse-frontend

@dhruvkb dhruvkb Awaiting requested review from dhruvkb dhruvkb is a code owner automatically assigned from WordPress/openverse-frontend

At least 1 approving review is required to merge this pull request.

Assignees

@obulat obulat

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
Openverse PRs
Status: 👀 Needs Review
Milestone
No milestone
2 participants
@obulat @openverse-bot