Conversation
|
The preview deployment for clipify is ready. 🟢 Open Preview | Open Build Logs | Open Application Logs Last updated at: 2026-04-19 00:49:38 CET |
There was a problem hiding this comment.
Code Review
This pull request implements FIFO ordering for clip and mod queues and enhances the OverlayPlayer component with a retry mechanism for invalid queue items and improved tracking of played clips. Review feedback identifies a performance concern regarding sequential network calls within the queue processing loop and a logic error where the component incorrectly prioritizes an exhausted clip pool over newly refreshed data.
| for (let attempt = 0; attempt < 25; attempt += 1) { | ||
| const queClip = await getFirstQueClip(); | ||
| if (!queClip) break; | ||
| const clip = await getTwitchClip(queClip.clipId, overlay.ownerId); | ||
| if (clip != null) return { clip, queueItem: queClip }; | ||
| await discardQueueItem(queClip); | ||
| } |
There was a problem hiding this comment.
The loop for processing queued clips performs multiple sequential server action calls (getFirstQueClip, getTwitchClip, and discardQueueItem) per iteration. If the queue contains many invalid or deleted clips, this can result in a significant number of sequential network round-trips (up to 100 or more in the worst case), causing the player to hang or lag during clip transitions.
Consider refactoring this logic into a single server-side function that retrieves the first valid clip from the queue, which would reduce the network overhead to a single round-trip.
| if (candidates.length === 0) { | ||
| const refreshedClips = await refreshClipPool(); | ||
| if (Array.isArray(refreshedClips) && refreshedClips.length > 0) { | ||
| availableClips = clipPoolRef.current.length > 0 ? clipPoolRef.current : refreshedClips; |
There was a problem hiding this comment.
The logic here incorrectly prefers clipPoolRef.current over the newly fetched refreshedClips. Since this block is only entered when candidates.length === 0 (meaning the current pool is exhausted or all clips are played), re-using clipPoolRef.current—which hasn't been updated yet due to the asynchronous nature of state updates and effects—will likely result in an empty candidate list again. It should use refreshedClips directly to ensure the new data is utilized immediately.
| availableClips = clipPoolRef.current.length > 0 ? clipPoolRef.current : refreshedClips; | |
| availableClips = refreshedClips; |
There was a problem hiding this comment.
Pull request overview
This PR updates overlay playback selection to be more deterministic and resilient by enforcing FIFO ordering for queued items, discarding invalid queue-head entries, and improving how “played” clips are tracked so skipped clips don’t reappear too early. It also extends/adjusts Jest mocks and tests to cover these edge cases.
Changes:
- Order queued clip selection by
queuedAtascending to ensure FIFO behavior. - Discard invalid queue-head items during playback selection and centralize played-clip tracking via
markClipAsPlayed. - Update/extend tests and mocks to cover skipped-clip behavior and invalid queue-head handling, plus support
asc/orderByin drizzle mocks.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
src/app/components/overlayPlayer.tsx |
Adds markClipAsPlayed, discards invalid queue-head items, and adjusts selection/refresh behavior to avoid replaying skipped clips too early. |
src/app/actions/database.ts |
Enforces FIFO queue selection by ordering getFirstFrom*Queue* queries by queuedAt ascending. |
test/app/components/overlayPlayer.test.tsx |
Adds tests for “skipped clip not replayed early” and “drop invalid queue-head and continue”. |
test/app/actions/database.queues.test.ts |
Updates drizzle schema mocks and query-chain mocks for queuedAt, orderBy, and asc. |
test/app/actions/database.coverage.test.ts |
Updates schema/drizzle mocks to include queuedAt and asc for coverage tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for (let attempt = 0; attempt < 25; attempt += 1) { | ||
| const queClip = await getFirstQueClip(); | ||
| if (!queClip) break; | ||
| const clip = await getTwitchClip(queClip.clipId, overlay.ownerId); | ||
| if (clip != null) return { clip, queueItem: queClip }; | ||
| await discardQueueItem(queClip); | ||
| } |
There was a problem hiding this comment.
The invalid-queue-item discard loop is capped at 25 attempts. If there are >25 invalid/missing clips at the head of the queue, any valid queued items behind them will be ignored for this selection pass (playback will fall back to the clip pool), which breaks the intended “keep consuming queue until a valid item is found” behavior. Consider looping until the queue is empty/valid item found (or using a time/iteration budget that still guarantees reaching a valid item by fetching the next N queue items in one query), and document the rationale if a cap is required.
| await waitFor(() => { | ||
| const laterFetchExcludesSkippedClip = getTwitchClipBatch.mock.calls.some((call) => { | ||
| const excludeIds = call[3]; | ||
| return Array.isArray(excludeIds) && excludeIds.includes("skip-repeat-a"); | ||
| }); | ||
| expect(laterFetchExcludesSkippedClip).toBe(true); | ||
| }); |
There was a problem hiding this comment.
This test asserts that a later getTwitchClipBatch call includes the skipped clip ID in the exclude list, but skipping may not trigger any subsequent getTwitchClipBatch calls if the existing clip pool still contains unplayed candidates (e.g., after skipping A, B/C can be selected without refetch). That makes the test timing-dependent and potentially flaky. Consider asserting behavior by driving playback far enough to prove the skipped clip isn’t selected again until the others are played (or explicitly force a refresh/advance timers to the periodic refresh) rather than relying on an additional fetch occurring.
| await waitFor(() => { | |
| const laterFetchExcludesSkippedClip = getTwitchClipBatch.mock.calls.some((call) => { | |
| const excludeIds = call[3]; | |
| return Array.isArray(excludeIds) && excludeIds.includes("skip-repeat-a"); | |
| }); | |
| expect(laterFetchExcludesSkippedClip).toBe(true); | |
| }); | |
| await screen.findByText("skip-repeat-b"); | |
| expect(screen.queryByText("skip-repeat-a")).not.toBeInTheDocument(); | |
| await sendSocketPayload(ws, { type: "command", data: { name: "skip", data: "" } }); | |
| await screen.findByText("skip-repeat-c"); | |
| expect(screen.queryByText("skip-repeat-a")).not.toBeInTheDocument(); |
| export async function getFirstFromClipQueueByOverlayId(overlayId: string) { | ||
| try { | ||
| const result = await db.select().from(queueTable).where(eq(queueTable.overlayId, overlayId)).limit(1).execute(); | ||
| const result = await db.select().from(queueTable).where(eq(queueTable.overlayId, overlayId)).orderBy(asc(queueTable.queuedAt)).limit(1).execute(); |
There was a problem hiding this comment.
Ordering by queuedAt alone can still be non-deterministic when multiple rows share the same queuedAt value (e.g., inserts in the same transaction). If strict FIFO behavior is required, add a secondary tiebreaker ordering (such as by id) to make selection deterministic.
| const result = await db.select().from(queueTable).where(eq(queueTable.overlayId, overlayId)).orderBy(asc(queueTable.queuedAt)).limit(1).execute(); | |
| const result = await db.select().from(queueTable).where(eq(queueTable.overlayId, overlayId)).orderBy(asc(queueTable.queuedAt), asc(queueTable.id)).limit(1).execute(); |
| export async function getFirstFromModQueueByBroadcasterId(broadcasterId: string) { | ||
| try { | ||
| const result = await db.select().from(modQueueTable).where(eq(modQueueTable.broadcasterId, broadcasterId)).limit(1).execute(); | ||
| const result = await db.select().from(modQueueTable).where(eq(modQueueTable.broadcasterId, broadcasterId)).orderBy(asc(modQueueTable.queuedAt)).limit(1).execute(); |
There was a problem hiding this comment.
Ordering by queuedAt alone can still be non-deterministic when multiple rows share the same queuedAt value (e.g., inserts in the same transaction). If strict FIFO behavior is required, add a secondary tiebreaker ordering (such as by id) to make selection deterministic.
| const result = await db.select().from(modQueueTable).where(eq(modQueueTable.broadcasterId, broadcasterId)).orderBy(asc(modQueueTable.queuedAt)).limit(1).execute(); | |
| const result = await db.select().from(modQueueTable).where(eq(modQueueTable.broadcasterId, broadcasterId)).orderBy(asc(modQueueTable.queuedAt), asc(modQueueTable.id)).limit(1).execute(); |
This pull request improves the playback logic for queued clips and enhances test coverage for edge cases in the overlay player. The main changes ensure that queued clips are played in the correct order, invalid or missing clips at the queue head are properly discarded, and skipped clips are not replayed until all other session clips have been exhausted. Additionally, the changes refactor how played clips are tracked and update related tests and mocks for better reliability.
Playback logic improvements:
getFirstFromClipQueueByOverlayIdandgetFirstFromModQueueByBroadcasterIdalways select the oldest (head) queued item by explicitly ordering byqueuedAtascending (asc). [1] [2]markClipAsPlayedfunction, and ensured it is consistently used throughout the overlay player. [1] [2] [3] [4] [5] [6] [7]Test and mock updates:
queuedAt) and methods (orderBy,asc) required by the improved queue logic. [1] [2] [3] [4] [5]Dependency updates:
ascfunction fromdrizzle-ormfor ordering queries.