Open
Conversation
Contributor
WalkthroughThis pull request implements a media error handling feature where posts with failed media are hidden from the feed. PostMedia emits mediaError events, which PostComponent re-emits with the post ID, and the posts page tracks these errors to filter out affected posts from rendering. Changes
Sequence Diagram(s)sequenceDiagram
participant PM as PostMedia
participant PC as PostComponent
participant Page as Posts Page
participant UI as Feed Render
PM->>PM: Media load fails
PM->>PC: emit mediaError
PC->>PC: onMediaError(postId)
PC->>Page: emit mediaError with postId
Page->>Page: onPostMediaError(postId)<br/>Add postId to hiddenPostMediaErrorKeys
Page->>Page: Filter posts<br/>Exclude hidden error keys
Page->>UI: Render filtered posts<br/>(problematic post removed)
Note over UI: "No results" shown if all posts filtered
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
✨ Simplify code
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pages/posts/[domain].vue (1)
589-605:⚠️ Potential issue | 🟡 Minor
isFirstPostcan become incorrect after hidden-post filtering.When a page’s original first post is filtered out at Line 589, no remaining post in that page gets
isFirstPost = true(Line 603 uses the pre-filter index). That can suppress the page indicator for that page.💡 Suggested fix
- return data.value.pages.flatMap((page) => { - // - - return page.data.flatMap((post, index) => { - if (hiddenPostMediaErrorKeys.value.includes(getPostMediaErrorKey(post.id))) { - return [] - } - - // - - return { - ...post, - - // Custom meta data - // TODO: Remove when API returns domain - domain: selectedBooru.value.domain, - - current_page: page.meta.current_page, - isFirstPost: index === 0 - } - }) - }) + return data.value.pages.flatMap((page) => { + const visiblePosts = page.data.filter((post) => { + return !hiddenPostMediaErrorKeys.value.includes(getPostMediaErrorKey(post.id)) + }) + + return visiblePosts.map((post, index) => ({ + ...post, + + // Custom meta data + // TODO: Remove when API returns domain + domain: selectedBooru.value.domain, + + current_page: page.meta.current_page, + isFirstPost: index === 0 + })) + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pages/posts/`[domain].vue around lines 589 - 605, The bug is that isFirstPost uses the original index (index) before filtering hidden posts, so if the first post is filtered out no post gets isFirstPost=true; to fix, first filter out hidden posts (using hiddenPostMediaErrorKeys.value.includes(getPostMediaErrorKey(post.id))) into a visible posts array for that page and then map that visible array to add domain, current_page and set isFirstPost based on the new mapped index (isFirstPost: mappedIndex === 0); update any code using the original page.posts/index to use this filtered-and-mapped array instead so the page indicator reflects the first visible post.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/pages/posts.test.ts`:
- Around line 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.
---
Outside diff comments:
In `@pages/posts/`[domain].vue:
- Around line 589-605: The bug is that isFirstPost uses the original index
(index) before filtering hidden posts, so if the first post is filtered out no
post gets isFirstPost=true; to fix, first filter out hidden posts (using
hiddenPostMediaErrorKeys.value.includes(getPostMediaErrorKey(post.id))) into a
visible posts array for that page and then map that visible array to add domain,
current_page and set isFirstPost based on the new mapped index (isFirstPost:
mappedIndex === 0); update any code using the original page.posts/index to use
this filtered-and-mapped array instead so the page indicator reflects the first
visible post.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 37167562-5ee2-4594-8571-40b12aae74af
📒 Files selected for processing (4)
components/pages/posts/post/PostComponent.vuecomponents/pages/posts/post/PostMedia.vuepages/posts/[domain].vuetest/pages/posts.test.ts
Comment on lines
151
to
164
| 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) | ||
| }) |
Contributor
There was a problem hiding this comment.
🧩 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()(orrequestfinishedif 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(...)) orlocator.waitFor(...), because those synchronize on DOM state rather than network events. (playwright.dev)
Citations:
- 1: https://playwright.dev/docs/api/class-page
- 2: https://playwright.dev/docs/api/class-page
- 3: https://playwright.dev/docs/actionability
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
PostMediathroughPostComponentso the posts page can filter failed items per search contextValidation
pnpm build(fails due to pre-existingBufferexport issue inassets/js/nuxt-image/imgproxy.provider.tsunder the current environment)pnpm test test/pages/posts.test.ts -t \"hides post when media failed to load in feed\"(fails because@nuxt/test-utilsis not installed in this repo environment)Summary by CodeRabbit
Release Notes