Repository navigation
perf(web): resolve dashboard thumbnail URLs during the page render - #3531
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
|
hey @greptileai, please re-review the PR |
|
hey @greptileai, please re-review the PR |
| if (videoId && Option.isSome(url)) seeded[videoId] = url.value; | ||
| }); | ||
| return seeded; | ||
| }).pipe(Effect.provide(makeCurrentUserLayer(user)), runPromise); |
There was a problem hiding this comment.
Server-seeded storage URLs lose LAN hostname rewriting
Medium Severity
When another LAN device opens the dashboard with MinIO configured at http://localhost:9000, S3BucketAccess.getPublicSigningClient relies on HttpServerRequest to replace localhost with the request's private-network hostname. The existing RPC executes in the HTTP request context, but this helper supplies only CurrentUser through runPromise, whose runtime attaches password cookies but no HttpServerRequest. The seed therefore contains localhost URLs pointing at the viewer's own device. These count as successful seeds, suppress the RPC fallback, and produce broken thumbnails. Carry the incoming request context into signing, or omit seeding when the endpoint needs request-dependent rewriting.
|
hey @greptileai, please re-review the PR |
|
hey @greptileai, please re-review the PR |
|
hey @greptileai, please re-review the PR |
| user: Parameters<typeof makeCurrentUserLayer>[0], | ||
| allVideoIds: readonly Video.VideoId[], | ||
| ): SeededThumbnails | undefined { | ||
| const videoIds = allVideoIds.slice(0, MAX_SEEDED_THUMBNAILS); |
There was a problem hiding this comment.
Seed cap routes excess ids into an RPC batch that rejects over 50
Medium Severity
The fallback this cap hands excess ids to batches unchunked in apps/web/lib/Requests/ThumbnailRequest.ts, where dataLoader runs with only window: "10 millis" and no maxBatchSize (@effect/experimental@0.56.0 defaults it to Infinity), and the VideosGetThumbnails payload schema rejects more than 50 ids (a.length <= 50, packages/web-domain/src/Video.ts:383). The dashboard pages read limit from the query string with no upper bound (Number(searchParams.limit) || 15) and render a card per fetched row, so ?limit=101 renders 101 thumbnails: the first 50 seed, and every remaining id mounts its query in the same hydration commit, lands in one dataLoader window, and goes out as a single VideosGetThumbnails call with 51+ ids, which the payload schema rejects; the resolver's catchAll then fails every request in the batch, and TanStack's retries use identical delays so each retry re-forms the same oversized batch and fails again, leaving every non-seeded thumbnail stuck on the error gradient. At the previous head all page ids were seeded server-side one at a time and rendered, so this is a regression for large-limit pages. Seed misses among the first 50 join the same batch, so even ?limit=100 fails wholesale from a single miss (50 fallback ids + 1 miss = 51). Chunk the client batch to the payload limit — pass maxBatchSize: 50 to dataLoader in ThumbnailRequest.ts (or wrap the resolver in RequestResolver.batchN(50)) — so the documented fallback actually works.
Dashboard thumbnails used to wait for hydration, then a
VideosGetThumbnailsRPC, and only then the image request. The caps and space pages now start that same lookup during the server render and stream the URLs to the client, so the image request goes out as soon as the page hydrates.apps/web/lib/seeded-thumbnails.tsrunsVideos.getThumbnailURLfor the page's caps with the same policy check, storage lookup and concurrency of 10 as the RPC handler. It does not await the result. It returns{ videoIds, urls: Promise }, and the promise never rejects: an id that fails or has no thumbnail is left out of the map.caps/page.tsxstarts it as soon as the page's video rows are loaded, in parallel with the shared-spaces query.spaces/[spaceId]/page.tsxstarts it for both the space and the org-wide variants. Caps with an active upload are excluded, because their thumbnail query stays disabled until the upload finishes.VideoThumbnailreads the promise fromSeededThumbnailsProviderwithuse()inside its ownSuspense. A seeded URL becomes the query'sinitialDataand is treated as fresh for five minutes (staleTime), so mounting it does not trigger a second lookup. Later mounts re-sign it once it is older than that, asmaindoes on every mount. Signed URLs last an hour. Ids not in the map go through the RPC exactly as before, including retries and the error gradient.useSyncExternalStore,components/VideoThumbnail.tsx:121). Putting the<img>in the HTML made the images download alongside the page's JS. On a 10 Mbps profile that pushed hydration back by about 400 ms, while the image still could not show before hydration becauseCapCardkeeps it atopacity-0untilonLoadruns.PublicCapCard, the spaceVideoCard, folder pages) behave as before.Numbers
Production builds of
mainand this branch on a Mac mini. Both run against the same seeded MySQL and MinIO, with injected round-trip latency (4 ms to the database, 20 ms to S3). Headless Chromium at 1440x900, cold load, median of 7 runs, base and head alternating. "Throttled" is 4x CPU and 10 Mbps / 40 ms. "Thumbnails shown" means every thumbnail on the page is loaded and visible./dashboard/caps, no folders/dashboard/caps, no folders/dashboard/caps, 30 folders/dashboard/spaces/[id]/dashboard/caps, no folders/dashboard/caps, 30 folders/dashboard/spaces/[id]/dashboard/caps,/dashboard/spaces/[id]/dashboard/caps,/dashboard/spaces/[id]/dashboard/caps,/dashboard/spaces/[id]Client navigation, with the thumbnails settled after the click:
Cost
The lookups now run inside the page request, so they share the request's database pool with the dashboard layout. At concurrency 10, the per-video policy queries can take every connection in the default 10-connection pool while the layout still has queries waiting, which adds about 10 ms of TTFB unthrottled (15 runs each, freshly warmed servers). The HTML response also stays open until the lookups finish, which moves DOMContentLoaded. The space page starts its lookups as soon as its video rows load, so that tail stays short. Lower concurrency trades TTFB for a longer open response (concurrency 3: TTFB +1 to +3 ms, DOMContentLoaded +130 to +200 ms), so this PR keeps the RPC handler's concurrency of 10. With the layout query change in #3535 applied as well, TTFB on
/dashboard/capsgoes from 72 ms to 44 ms. A bulk video lookup for thumbnails would remove the contention entirely and is left for a follow-up.On "back to caps",
mainrefetches every cached thumbnail URL on remount and re-signs it, which forces a fresh download. Here a URL younger than five minutes is reused, and an older cached one is re-signed on mount as before. This was checked with a fake clock: no refetch one minute after leaving, and one refetch when returning to a page whose URLs are ten minutes old.Signed URL hosts are unchanged.
/api/erpcruns its handlers outside the HTTP request context, so onmainthe RPC does not apply the LAN rewrite for alocalhoststorage endpoint either. Loading the dashboard from another LAN address giveslocalhost:9011thumbnail URLs on both builds.With a deleted thumbnail object and a cap mid-upload on page 1, the settled page is pixel-identical to
main. Only those two ids go through the RPC; onmainall 15 do.

Confidence Score: 5/5
The PR appears safe to merge; no blocking issue was established.Summary
The PR starts dashboard thumbnail lookups during the server render and streams their URLs to the cards.
Reviews (6) · Last reviewed commit: "fix(web): seed at most 50 thumbnail URLs..." · Reviewed by Greptile