Skip to content

fix: hide broken media posts in feed #99

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

Open
AlejandroAkbal wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: hide broken media posts in feed #99

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
6 changes: 6 additions & 0 deletions components/pages/posts/post/PostComponent.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,13 @@
addTag: [tag: string]
setTag: [tag: string]
openTagInNewTab: [tag: string]
mediaError: [postId: number]
}>()

function onMediaError() {
emit('mediaError', props.post.id)
}

const currentUrl = useRequestURL()

const { postFullSizeImages } = useUserSettings()
Expand Down Expand Up @@ -108,6 +113,7 @@
:mediaSrcWidth="mediaFile.width"
:mediaType="post.media_type"
:postIndex="props.postIndex"
@mediaError="onMediaError"
/>

<figcaption>
Expand Down
5 changes: 5 additions & 0 deletions components/pages/posts/post/PostMedia.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,10 @@

const props = defineProps<PostMediaProps>()

const emit = defineEmits<{
mediaError: []
}>()

const isMainHost = computed(() => import.meta.dev || requestUrl.hostname === project.urls.production.hostname)

const mediaElement = ref<HTMLElement | null>(null)
Expand Down Expand Up @@ -337,6 +341,7 @@
}

error.value = new Error('Error loading media')
emit('mediaError')
}

function manuallyReloadMedia() {
Expand Down
63 changes: 50 additions & 13 deletions pages/posts/[domain].vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,23 +34,27 @@
onMounted(() => {
const hasLoadedAds = ref(false)

watch([hasInteracted, isPremium], ([hasInteracted, isPremium]) => {
if (hasLoadedAds.value) {
return
}
watch(
[hasInteracted, isPremium],
([hasInteracted, isPremium]) => {
if (hasLoadedAds.value) {
return
}

if (!hasInteracted) {
return
}
if (!hasInteracted) {
return
}

if (isPremium) {
return
}
if (isPremium) {
return
}

hasLoadedAds.value = true
hasLoadedAds.value = true

useAdvertisements()
}, { immediate: true })
useAdvertisements()
},
{ immediate: true }
)
})

/**
Expand Down Expand Up @@ -375,6 +379,34 @@
window.location.reload()
}

const hiddenPostMediaErrorKeys = ref<string[]>([])

const hiddenPostMediaErrorScopeKey = computed(() => {
return JSON.stringify({
domain: selectedBooru.value.domain,
tags: selectedTags.value.map((tag) => tag.name),
filters: selectedFilters.value
})
})

function getPostMediaErrorKey(postId: number) {
return `${selectedBooru.value.domain}-${postId}`
}

function onPostMediaError(postId: number) {
const postMediaErrorKey = getPostMediaErrorKey(postId)

if (hiddenPostMediaErrorKeys.value.includes(postMediaErrorKey)) {
return
}

hiddenPostMediaErrorKeys.value.push(postMediaErrorKey)
}

watch(hiddenPostMediaErrorScopeKey, () => {
hiddenPostMediaErrorKeys.value = []
})

/**
* Data fetching
*/
Expand Down Expand Up @@ -554,6 +586,10 @@
//

return page.data.flatMap((post, index) => {
if (hiddenPostMediaErrorKeys.value.includes(getPostMediaErrorKey(post.id))) {
return []
}

//

return {
Expand Down Expand Up @@ -1059,6 +1095,7 @@
:postIndex="virtualRow.index"
:selectedTags="selectedTags"
@addTag="onPostAddTag"
@mediaError="onPostMediaError"
@openTagInNewTab="onPostOpenTagInNewTab"
@setTag="onPostSetTag"
/>
Expand Down
25 changes: 9 additions & 16 deletions test/pages/posts.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
import {describe, expect, it} from 'vitest'
import {createPage, setup, url} from '@nuxt/test-utils'
import { describe, expect, it } from 'vitest'
import { createPage, setup, url } from '@nuxt/test-utils'
import {
mockPostsPage0,
mockPostsPage1,
mockPostsPageWithOfflineMedia,
mockPostsPageWithoutResults
} from './posts.mock-data'
import {defaultSetupConfig} from '../helper'
import { defaultSetupConfig } from '../helper'

describe('/', async () => {
await setup(defaultSetupConfig)
Expand Down Expand Up @@ -127,7 +127,7 @@ describe('/', async () => {
throw new Error('Not implemented')
})

it('renders warning when media failed to load', async () => {
it('hides post when media failed to load in feed', async () => {
// Arrange
const page = await createPage()

Expand All @@ -151,23 +151,16 @@ describe('/', async () => {
await Promise.all([
page.goto(url('/posts/safebooru.org')),
page.waitForResponse('**/posts?baseEndpoint=*'),

await page.waitForRequest('**/example.local/**')
page.waitForRequest('**/example.local/**')
])

// Assert

// Expect 1 post
expect(
//
await page.getByTestId('posts-list').locator('li').count()
).toBe(1)
// Expect post to be hidden after media fails
expect(await page.getByRole('heading', { name: /no results/i }).isVisible()).toBe(true)

// Expect post to have warning
expect(
//
await page.getByTestId('posts-list').locator('li').first().textContent()
).toContain('Error loading media')
// Expect no media error placeholder to remain in feed
expect(await page.getByText('Error loading media').count()).toBe(0)
})
Comment on lines 151 to 164
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Mar 14, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🌐 Web query:

In Playwright, does page.waitForRequest()guarantee that the response/error handling and resulting DOM updates have completed, or shouldpage.waitForResponse()/locator waitFor() be used for post-request UI assertions?

💡 Result:

page.waitForRequest() only waits until the request is issued/matched and returns the Request object—it does not imply the response arrived, failed, your app processed it, or that any DOM/UI updates finished. (playwright.dev)

For post-request assertions:

  • If you need to ensure the backend call completed (and optionally assert status, etc.), wait on page.waitForResponse() (or requestfinished if you specifically care about the body download). (playwright.dev)
  • If your goal is “UI is updated,” prefer waiting on the UI condition itself using Playwright’s auto-retrying assertions (e.g., await expect(locator).toHaveText(...)) or locator.waitFor(...), because those synchronize on DOM state rather than network events. (playwright.dev)

Citations:

Replace page.waitForRequest() with explicit waits for response and UI state.

page.waitForRequest() only confirms the request was issued—not that the response completed or DOM updated. This makes assertions flaky. Use page.waitForResponse() to wait for the request to complete, then explicitly wait for the UI condition:

Suggested fix 
       await Promise.all([
         page.goto(url('/posts/safebooru.org')),
         page.waitForResponse('**/posts?baseEndpoint=*'),
-        page.waitForRequest('**/example.local/**')
+        page.waitForResponse('**/example.local/**')
       ])
+
+      await page.getByRole('heading', { name: /no results/i }).waitFor()
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@test/pages/posts.test.ts` around lines 151 - 164, The test uses
page.waitForRequest() which only confirms a request started; replace that call
with a page.waitForResponse(...) for the same URL pattern and then add explicit
UI waits before assertions: wait for the posts network response (use
page.waitForResponse('**/example.local/**') or equivalent), then wait for the
heading to appear via page.getByRole('heading', { name: /no results/i
}).waitFor() or a page.waitForSelector targeting the same element, and finally
wait for the absence of the "Error loading media" text (e.g.,
page.getByText('Error loading media').waitFor({ state: 'detached' }) or an
equivalent explicit check) before making the expect(...) assertions; replace the
original page.waitForRequest(...) usage with these response+UI waits to make the
assertions deterministic.

})

Expand Down