From 4ab315e9d7d3aeadb666e6b63c0ce2982441b814 Mon Sep 17 00:00:00 2001 From: Gregor Klevze Date: Sat, 29 Aug 2026 12:07:08 +0200 Subject: [PATCH] Defer artwork comments from initial render --- .../Controllers/Web/ArtworkPageController.php | 103 ++++-------------- resources/js/Pages/ArtworkPage.jsx | 40 ++----- .../js/components/artwork/ArtworkComments.jsx | 83 +++++++++++--- .../artwork/ArtworkComments.test.jsx | 77 +++++++++++++ .../ArtworkRecommendationsRails.test.jsx | 15 ++- .../js/components/viewer/viewer.test.jsx | 5 +- .../M18H1DeferredArtworkCommentsTest.php | 72 ++++++++++++ vitest.config.mjs | 10 ++ 8 files changed, 267 insertions(+), 138 deletions(-) create mode 100644 resources/js/components/artwork/ArtworkComments.test.jsx create mode 100644 tests/Feature/Artworks/M18H1DeferredArtworkCommentsTest.php create mode 100644 vitest.config.mjs diff --git a/app/Http/Controllers/Web/ArtworkPageController.php b/app/Http/Controllers/Web/ArtworkPageController.php index 5af3ef10..15680f30 100644 --- a/app/Http/Controllers/Web/ArtworkPageController.php +++ b/app/Http/Controllers/Web/ArtworkPageController.php @@ -1,5 +1,4 @@ where('id', $id) - ->public() - ->published() - ->firstOrFail(); + // ── Step 2: hydrate the already-validated model ──────────────────── + // The initial withTrashed lookup is intentionally retained so that + // missing, deleted, private, and unpublished artwork keep their + // distinct responses. Reusing that model avoids a second identical + // primary-key lookup on the public path. + $artwork = $raw; + $artwork->load(['user.profile', 'group.owner.profile', 'uploadedBy.profile', 'primaryAuthor.profile', 'contributors.user.profile', 'categories.contentType', 'categories.parent.contentType', 'tags', 'stats', 'awardStat']); $this->loadCategoryAncestors($artwork->categories); @@ -119,12 +117,15 @@ final class ArtworkPageController extends Controller ], 301); } - $thumbMd = ThumbnailPresenter::present($artwork, 'md'); - $thumbLg = ThumbnailPresenter::present($artwork, 'lg'); - $thumbXl = ThumbnailPresenter::present($artwork, 'xl'); - $thumbSq = ThumbnailPresenter::present($artwork, 'sq'); - $artworkData = (new ArtworkResource($artwork))->toArray($request); + $navigationData = $this->navigation->navigationFor($artwork); + // ArtworkResource is the canonical thumbnail projection. Reuse its + // exact values for page props and SEO instead of presenting each + // variant a second time in the controller. + $thumbMd = $artworkData['thumbs']['md'] ?? []; + $thumbLg = $artworkData['thumbs']['lg'] ?? []; + $thumbXl = $artworkData['thumbs']['xl'] ?? []; + $thumbSq = $artworkData['thumbs']['sq'] ?? []; $groupSummary = null; if ($artwork->group) { @@ -206,84 +207,22 @@ final class ArtworkPageController extends Controller ->values() ->all(); - $approvedComments = ArtworkComment::query() - ->with('user.profile') - ->where('artwork_id', $artwork->id) - ->where('is_approved', true) - ->orderBy('created_at') - ->limit(500) - ->get(); - - $commentsByParent = $approvedComments->groupBy( - static fn (ArtworkComment $comment): string => $comment->parent_id === null - ? 'root' - : (string) $comment->parent_id - ); - - // Recursive helper to format a comment and its nested replies. - $formatComment = null; - $formatComment = function (ArtworkComment $c) use (&$formatComment, $commentsByParent): array { - /** @var Collection $replies */ - $replies = $commentsByParent->get((string) $c->id, collect()); - $user = $c->user; - $userId = (int) ($c->user_id ?? 0); - $avatarHash = $user?->profile?->avatar_hash ?? null; - $canPublishLinks = (int) ($user?->level ?? 1) > 1 && strtolower((string) ($user?->rank ?? 'Newbie')) !== 'newbie'; - $rawContent = (string) ($c->raw_content ?? $c->content ?? ''); - $renderedContent = $c->rendered_content; - - if (! is_string($renderedContent) || trim($renderedContent) === '') { - $renderedContent = $rawContent !== '' - ? ContentSanitizer::render($rawContent) - : nl2br(e(strip_tags((string) ($c->content ?? '')))); - } - - return [ - 'id' => $c->id, - 'parent_id' => $c->parent_id, - 'content' => html_entity_decode((string) $c->content, ENT_QUOTES | ENT_HTML5, 'UTF-8'), - 'raw_content' => $c->raw_content ?? $c->content, - 'rendered_content' => ContentSanitizer::sanitizeRenderedHtml($renderedContent, $canPublishLinks), - 'created_at' => $c->created_at?->toIso8601String(), - 'time_ago' => $c->created_at ? Carbon::parse($c->created_at)->diffForHumans() : null, - 'user' => [ - 'id' => $userId, - 'name' => $user?->name, - 'username' => $user?->username, - 'display' => $user?->username ?? $user?->name ?? 'User', - 'profile_url' => $user?->username ? '/@' . $user->username : ($userId > 0 ? '/profile/' . $userId : null), - 'avatar_url' => $avatarHash !== null - ? AvatarUrl::forUser($userId, $avatarHash, 64) - : AvatarUrl::default(), - 'level' => (int) ($user?->level ?? 1), - 'rank' => (string) ($user?->rank ?? 'Newbie'), - ], - 'replies' => $replies->map($formatComment)->values()->all(), - ]; - }; - - $comments = $commentsByParent - ->get('root', collect()) - ->map($formatComment) - ->values() - ->all(); - $canReadSession = $request->hasSession() && ! $request->attributes->get('skinbase.session_skipped'); + $viewerId = ($canReadSession && $request->user() !== null) ? (int) $request->user()->id : null; - $userId = ($canReadSession && $request->user() !== null) ? (int) $request->user()->id : null; return Inertia::render('ArtworkPage', [ 'artwork' => $artworkData, + 'navigation' => $navigationData, 'presentMd' => $thumbMd, 'presentLg' => $thumbLg, 'presentXl' => $thumbXl, 'presentSq' => $thumbSq, 'related' => $related, 'canonicalUrl' => $canonical, - 'comments' => $comments, 'groupSummary' => $groupSummary, - 'isAuthenticated' => $userId !== null, - 'reactionTotals' => $this->artworkReactionTotals((int) $artwork->id, $userId), + 'isAuthenticated' => $viewerId !== null, + 'reactionTotals' => $this->artworkReactionTotals((int) $artwork->id, $viewerId), 'seo' => $seo, ])->rootView('artworks.show'); } diff --git a/resources/js/Pages/ArtworkPage.jsx b/resources/js/Pages/ArtworkPage.jsx index a6a9944d..9e3e5fa1 100644 --- a/resources/js/Pages/ArtworkPage.jsx +++ b/resources/js/Pages/ArtworkPage.jsx @@ -18,6 +18,7 @@ import ArtworkViewer from '../components/viewer/ArtworkViewer' import ReactionBar from '../components/comments/ReactionBar' import GroupSummaryPanel from '../components/groups/GroupSummaryPanel' import SeoHead from '../components/seo/SeoHead' +import { useDeferredSimilarAi } from '../lib/useDeferredSimilarAi' function publisherToGroupSummary(publisher) { if (!publisher || publisher.type !== 'group') return null @@ -42,7 +43,7 @@ function publisherToGroupSummary(publisher) { } } -function ArtworkPage({ artwork: initialArtwork, related: initialRelated, presentMd: initialMd, presentLg: initialLg, presentXl: initialXl, presentSq: initialSq, canonicalUrl: initialCanonical, isAuthenticated = false, comments: initialComments = [], groupSummary: initialGroupSummary = null, reactionTotals: initialReactionTotals = {}, seo = null }) { + function ArtworkPage({ artwork: initialArtwork, navigation: initialNavigation = null, related: initialRelated, presentMd: initialMd, presentLg: initialLg, presentXl: initialXl, presentSq: initialSq, canonicalUrl: initialCanonical, isAuthenticated = false, groupSummary: initialGroupSummary = null, reactionTotals: initialReactionTotals = {}, seo = null }) { const [viewerOpen, setViewerOpen] = useState(false) const [showMatureArtwork, setShowMatureArtwork] = useState(false) const openViewer = useCallback(() => setViewerOpen(true), []) @@ -66,12 +67,12 @@ function ArtworkPage({ artwork: initialArtwork, related: initialRelated, present const [presentXl, setPresentXl] = useState(initialXl) const [presentSq, setPresentSq] = useState(initialSq) const [related, setRelated] = useState(initialRelated) - const [comments, setComments] = useState(initialComments) const [canonicalUrl, setCanonicalUrl] = useState(initialCanonical) const [groupSummary, setGroupSummary] = useState(initialGroupSummary || publisherToGroupSummary(initialArtwork?.publisher)) const [selectedMediaId, setSelectedMediaId] = useState('cover') - const [similarRecommendations, setSimilarRecommendations] = useState([]) const [trendingRecommendations, setTrendingRecommendations] = useState([]) + const similarAiAnchorRef = useRef(null) + const { items: deferredSimilarRecommendations } = useDeferredSimilarAi(artwork?.id, similarAiAnchorRef) // Nav arrow state — populated by ArtworkNavigator once neighbors resolve const [navState, setNavState] = useState({ hasPrev: false, hasNext: false, navigatePrev: null, navigateNext: null }) @@ -89,32 +90,6 @@ function ArtworkPage({ artwork: initialArtwork, related: initialRelated, present .catch(() => setReactionTotals({})) }, [artwork?.id]) - useEffect(() => { - let isCancelled = false - - const loadSimilarRecommendations = async () => { - if (!artwork?.id) { - setSimilarRecommendations([]) - return - } - - try { - const response = await fetch(`/api/art/${artwork.id}/similar-ai`, { credentials: 'same-origin' }) - if (!response.ok) throw new Error('similar fetch failed') - const payload = await response.json() - if (!isCancelled) setSimilarRecommendations(payload?.data || []) - } catch { - if (!isCancelled) setSimilarRecommendations([]) - } - } - - loadSimilarRecommendations() - - return () => { - isCancelled = true - } - }, [artwork?.id]) - useEffect(() => { let isCancelled = false @@ -165,7 +140,6 @@ function ArtworkPage({ artwork: initialArtwork, related: initialRelated, present setCanonicalUrl(data.canonical_url ?? window.location.href) setGroupSummary(data.group_summary ?? publisherToGroupSummary(data.publisher)) setSelectedMediaId('cover') - setSimilarRecommendations([]) setTrendingRecommendations([]) setViewerOpen(false) // close viewer when navigating away setShowMatureArtwork(false) @@ -348,7 +322,6 @@ function ArtworkPage({ artwork: initialArtwork, related: initialRelated, present {/* Comments */} @@ -371,11 +344,11 @@ function ArtworkPage({ artwork: initialArtwork, related: initialRelated, present {/* ── Full-width recommendation rails ─────────────────────────── */} -
+
@@ -384,6 +357,7 @@ function ArtworkPage({ artwork: initialArtwork, related: initialRelated, present {/* Artwork navigator — prev/next arrows, keyboard, swipe, no page reload */} { - if (!artworkId) return + if (!artworkId || requestRef.current) return + const generation = generationRef.current + const controller = new AbortController() + requestRef.current = controller setLoading(true) + setError(false) try { - const { data } = await axios.get(`/api/artworks/${artworkId}/comments?page=${p}`) + const { data } = await axios.get(`/api/artworks/${artworkId}/comments?page=${p}`, { signal: controller.signal }) + if (generation !== generationRef.current) return if (p === 1) { setComments(data.data ?? []) } else { @@ -492,24 +499,62 @@ export default function ArtworkComments({ setLastPage(data.meta?.last_page ?? 1) setTotal(data.meta?.total ?? 0) } catch { - // keep existing + if (generation === generationRef.current && !controller.signal.aborted) setError(true) } finally { - setLoading(false) + if (requestRef.current === controller) { + requestRef.current = null + if (generation === generationRef.current) setLoading(false) + } } }, [artworkId], ) useEffect(() => { - if (initialized.current) return - initialized.current = true + generationRef.current += 1 + requestRef.current?.abort() + requestRef.current = null + setComments([]) + setLoading(false) + setError(false) + setPage(1) + setLastPage(1) + setTotal(0) - if (artworkId && initialComments.length === 0) { - loadComments(1) - } else { - setTotal(initialComments.length) + const section = sectionRef.current + if (!section || !artworkId) return undefined + + let fallbackTimer = null + let observer = null + let established = false + const trigger = () => { + observer?.disconnect() + if (!requestRef.current) loadComments(1) } - }, [artworkId, initialComments.length, loadComments]) + + if (typeof window !== 'undefined' && typeof window.IntersectionObserver === 'function') { + try { + observer = new window.IntersectionObserver((entries) => { + if (entries.some((entry) => entry.isIntersecting)) trigger() + }, { rootMargin: '1000px 0px' }) + observer.observe(section) + established = true + } catch { + observer?.disconnect() + } + } + + if (!established && typeof window !== 'undefined') { + fallbackTimer = window.setTimeout(trigger, 1200) + } + + return () => { + observer?.disconnect() + if (fallbackTimer !== null && typeof window !== 'undefined') window.clearTimeout(fallbackTimer) + requestRef.current?.abort() + requestRef.current = null + } + }, [artworkId, loadComments]) // New top-level comment posted const handlePosted = useCallback((newComment) => { @@ -538,7 +583,7 @@ export default function ArtworkComments({ }, []) return ( -
+
{/* Section header */}

@@ -554,6 +599,11 @@ export default function ArtworkComments({ {/* Comment list */} {loading && comments.length === 0 ? ( + ) : error && comments.length === 0 ? ( +
+

Comments could not be loaded.

+ +
) : comments.length === 0 ? (
@@ -611,4 +661,3 @@ export default function ArtworkComments({

) } - diff --git a/resources/js/components/artwork/ArtworkComments.test.jsx b/resources/js/components/artwork/ArtworkComments.test.jsx new file mode 100644 index 00000000..e98bb2e0 --- /dev/null +++ b/resources/js/components/artwork/ArtworkComments.test.jsx @@ -0,0 +1,77 @@ +import React from 'react' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { act, cleanup, render, screen, waitFor } from '@testing-library/react' +import axios from 'axios' +import ArtworkComments from './ArtworkComments' + +vi.mock('axios', () => ({ default: { get: vi.fn() } })) +vi.mock('../comments/CommentForm', () => ({ default: () =>
})) +vi.mock('../comments/ReactionBar', () => ({ default: () =>
})) +vi.mock('../xp/LevelBadge', () => ({ default: () => null })) +vi.mock('../../utils/emojiFlood', () => ({ isFlood: () => false })) + +describe('ArtworkComments deferred loading', () => { + let observerCallback + + beforeEach(() => { + axios.get.mockResolvedValue({ + data: { + data: [{ id: 1, content: 'Loaded comment', replies: [], user: {}, reactions: {} }], + meta: { current_page: 1, last_page: 1, total: 1 }, + }, + }) + window.IntersectionObserver = vi.fn((callback) => { + observerCallback = callback + return { observe: vi.fn(), disconnect: vi.fn() } + }) + }) + + afterEach(() => { + cleanup() + vi.restoreAllMocks() + delete window.IntersectionObserver + }) + + it('does not fetch until the comments section approaches the viewport', async () => { + render() + + await new Promise((resolve) => setTimeout(resolve, 20)) + expect(axios.get).not.toHaveBeenCalled() + + await act(async () => observerCallback([{ isIntersecting: true }])) + + await waitFor(() => expect(axios.get).toHaveBeenCalledTimes(1)) + expect(axios.get).toHaveBeenCalledWith( + '/api/artworks/9357/comments?page=1', + expect.objectContaining({ signal: expect.any(AbortSignal) }), + ) + expect(screen.getByText('Loaded comment')).not.toBeNull() + }) + + it('uses the delayed compatibility fallback when IntersectionObserver is unavailable', async () => { + delete window.IntersectionObserver + vi.useFakeTimers() + render() + + expect(axios.get).not.toHaveBeenCalled() + await act(async () => vi.advanceTimersByTime(1200)) + + expect(axios.get).toHaveBeenCalledTimes(1) + vi.useRealTimers() + }) + + it('resets on artwork changes and ignores stale responses', async () => { + let resolveRequest + axios.get.mockImplementation(() => new Promise((resolve) => { resolveRequest = resolve })) + const { rerender } = render() + await act(async () => observerCallback([{ isIntersecting: true }])) + await waitFor(() => expect(axios.get).toHaveBeenCalledTimes(1)) + + rerender() + await act(async () => { + resolveRequest({ data: { data: [{ id: 1, content: 'stale', replies: [], user: {}, reactions: {} }], meta: {} } }) + await Promise.resolve() + }) + expect(screen.queryByText('stale')).toBeNull() + }) +}) diff --git a/resources/js/components/artwork/ArtworkRecommendationsRails.test.jsx b/resources/js/components/artwork/ArtworkRecommendationsRails.test.jsx index 7573a47e..f0ce5a13 100644 --- a/resources/js/components/artwork/ArtworkRecommendationsRails.test.jsx +++ b/resources/js/components/artwork/ArtworkRecommendationsRails.test.jsx @@ -42,7 +42,7 @@ describe('ArtworkRecommendationsRails', () => { vi.restoreAllMocks() }) - it('loads recommendation rails after mount', async () => { + it('loads normal trending rails after mount without owning Similar-AI loading', async () => { render( { categories: [{ id: 5, name: 'Sci-Fi' }], }} related={[]} + trendingData={[{ + id: 11, + title: 'Star map drift', + urls: { direct: '/art/11/star-map-drift' }, + author: { name: 'Pilot' }, + thumbnail_url: '/thumbs/11.webp', + }]} />, ) @@ -58,7 +65,7 @@ describe('ArtworkRecommendationsRails', () => { expect(screen.getByText('Trending in Sci-Fi')).not.toBeNull() }) - expect(global.fetch).toHaveBeenCalledWith('/api/art/69827/similar-ai', { credentials: 'same-origin' }) - expect(global.fetch).toHaveBeenCalledWith('/api/rank/category/5?type=trending', { credentials: 'same-origin' }) + expect(global.fetch).not.toHaveBeenCalledWith('/api/art/69827/similar-ai', { credentials: 'same-origin' }) + expect(global.fetch).not.toHaveBeenCalled() }) -}) \ No newline at end of file +}) diff --git a/resources/js/components/viewer/viewer.test.jsx b/resources/js/components/viewer/viewer.test.jsx index 0a385ef2..265544ad 100644 --- a/resources/js/components/viewer/viewer.test.jsx +++ b/resources/js/components/viewer/viewer.test.jsx @@ -51,7 +51,7 @@ describe('Context navigation — useNavContext', () => { }) it('resolves prev/next IDs from the same-user API', async () => { - const apiData = { prev_id: 100, next_id: 300, prev_url: '/art/100', next_url: '/art/300' } + const apiData = { prev_id: 100, next_id: 300, prev_url: '/art/100', next_url: '/art/300', prev_slug: 'prev', next_slug: 'next' } mockFetch(apiData) const { useNavContext } = await import('../../lib/useNavContext') @@ -99,7 +99,7 @@ describe('Fallback — API navigation when no sessionStorage context', () => { it('calls /api/artworks/navigation/{id} when sessionStorage is empty', async () => { mockSessionStorage(null) - const apiData = { prev_id: 50, next_id: 150, prev_url: '/art/50', next_url: '/art/150' } + const apiData = { prev_id: 50, next_id: 150, prev_url: '/art/50', next_url: '/art/150', prev_slug: 'prev', next_slug: 'next' } mockFetch(apiData) const { useNavContext } = await import('../../lib/useNavContext') @@ -143,6 +143,7 @@ describe('Fallback — API navigation when no sessionStorage context', () => { await waitFor(() => expect(screen.getByTestId('result')).not.toBeNull()) expect(screen.getByTestId('result').textContent).toBe('null|null') }) + }) // ─── 3. Keyboard Test ───────────────────────────────────────────────────────── diff --git a/tests/Feature/Artworks/M18H1DeferredArtworkCommentsTest.php b/tests/Feature/Artworks/M18H1DeferredArtworkCommentsTest.php new file mode 100644 index 00000000..df74397f --- /dev/null +++ b/tests/Feature/Artworks/M18H1DeferredArtworkCommentsTest.php @@ -0,0 +1,72 @@ +create([ + 'slug' => 'm18h1-deferred-comments', + ]); + ArtworkComment::factory()->count(2)->create(['artwork_id' => $artwork->id]); + + DB::enableQueryLog(); + DB::flushQueryLog(); + + $response = $this->get(route('art.show', ['id' => $artwork->id, 'slug' => $artwork->slug]))->assertOk(); + $queries = collect(DB::getQueryLog()); + DB::disableQueryLog(); + + expect($response->getContent())->not->toContain('"comments"') + ->and($queries->filter(fn (array $query): bool => str_contains(strtolower($query['query']), 'artwork_comments'))->count())->toBe(0); +}); + +it('keeps the existing complete comments API contract', function (): void { + $artwork = Artwork::factory()->create(); + $comment = ArtworkComment::factory()->create(['artwork_id' => $artwork->id]); + + $this->getJson("/api/artworks/{$artwork->id}/comments") + ->assertOk() + ->assertJsonStructure([ + 'data' => [[ + 'id', 'parent_id', 'raw_content', 'rendered_content', + 'created_at', 'time_ago', 'user', 'reactions', 'replies', + ]], + 'meta' => ['current_page', 'last_page', 'total', 'per_page'], + ]) + ->assertJsonPath('data.0.id', $comment->id); +}); + +it('measures the initial artwork detail SQL after deferring comments', function (): void { + $artwork = Artwork::factory()->create([ + 'slug' => 'm18h1-sql-measurement', + ]); + ArtworkComment::factory()->count(2)->create(['artwork_id' => $artwork->id]); + + DB::flushQueryLog(); + DB::enableQueryLog(); + + $this->get(route('art.show', ['id' => $artwork->id, 'slug' => $artwork->slug])) + ->assertOk(); + + $queries = DB::getQueryLog(); + DB::disableQueryLog(); + + $normalized = collect($queries)->map(fn (array $query): string => strtolower(preg_replace('/\s+/', ' ', $query['query']))); + $selects = $normalized->filter(fn (string $query): bool => str_starts_with(ltrim($query), 'select ')); + $writes = $normalized->reject(fn (string $query): bool => str_starts_with(ltrim($query), 'select ')); + $commentQueries = $normalized->filter(fn (string $query): bool => str_contains($query, 'artwork_comments')); + + fwrite(STDERR, sprintf( + "M18H1_SQL initial_guest_total=%d initial_guest_select=%d initial_guest_write=%d initial_guest_comment_queries=%d\n", + count($queries), + $selects->count(), + $writes->count(), + $commentQueries->count(), + )); + + expect($commentQueries)->toHaveCount(0); +}); diff --git a/vitest.config.mjs b/vitest.config.mjs new file mode 100644 index 00000000..c4877eb6 --- /dev/null +++ b/vitest.config.mjs @@ -0,0 +1,10 @@ +import { defineConfig } from 'vitest/config' + +export default defineConfig({ + test: { + environment: 'jsdom', + globals: true, + setupFiles: ['resources/js/test/setupTests.js'], + include: ['resources/js/**/*.test.{js,jsx}'], + }, +})