Defer highlight reel generation until requested#65
Conversation
|
@Liamzhangg is attempting to deploy a commit to the samuelz12's projects Team on Vercel. A member of the Team first needs to authorize it. |
There was a problem hiding this comment.
Pull request overview
This PR updates the analyze workspace flow so the transcript/video UI loads and remains usable before highlight reels are generated, deferring highlight generation until the user explicitly requests it while moving summary generation into a non-blocking background task.
Changes:
- Gate highlight reel generation behind a “Generate highlight reels” CTA (including cached-reel reveal) and keep highlight errors localized to the highlights UI.
- Refactor the YouTube player to support direct transcript seeking via an imperative ref handle, scoped iframe container IDs, and safer command queuing.
- Add layout hardening (defensive right-column min height) and introduce targeted tests covering the new flows.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| components/youtube-player.tsx | Adds player helpers, scoped element IDs, command queueing, and an imperative seekTo handle for transcript-driven seeking. |
| components/video-progress-bar.tsx | Extracts density normalization into a helper used by tests. |
| components/topic-card.tsx | Updates React import style to align with other components/tests. |
| components/highlights-panel.tsx | Adds “generate highlights” CTA state, localized error/loading UI, and hides Play All controls until topics exist. |
| components/tests/youtube-player.timeline.test.ts | Adds unit coverage for new YouTube player helpers/queueing behavior and density normalization. |
| components/tests/highlights-panel.generate.test.tsx | Verifies generate-CTA, elapsed-time display, and Play All visibility rules. |
| app/analyze/tests/summary-background-flow.test.ts | Ensures summary failures stay local (no full-page throw) for background generation paths. |
| app/analyze/tests/right-column-layout.test.ts | Verifies right-column min-height/resize behavior and player remount + direct seek attempt behavior. |
| app/analyze/tests/cached-highlights-flow.test.ts | Verifies cached highlights are stored but hidden until user generates, and cached reveal ordering/behavior. |
| app/analyze/[videoId]/page.tsx | Defers highlights generation behind a button, runs takeaways generation in background, adds cached highlight payload ref, and adjusts layout/seek behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| let lastUpdateTime = 0; | ||
| timeUpdateIntervalRef.current = setInterval(() => { | ||
| if (!shouldPollPlayerTime(playerRef.current, true)) return; |
There was a problem hiding this comment.
startTimeUpdateInterval calls shouldPollPlayerTime(playerRef.current, true), which bypasses the readiness guard that shouldPollPlayerTime is meant to enforce. This makes the helper misleading and risks polling before the iframe is actually ready if this code is refactored/reused.
Use playerReadyRef.current (or the playerReady state) instead of the hard-coded true so polling is consistently gated on readiness.
| if (!shouldPollPlayerTime(playerRef.current, true)) return; | |
| if (!shouldPollPlayerTime(playerRef.current, playerReadyRef.current)) return; |
| }, [selectedLanguage, onRequestTranslation]); | ||
|
|
||
| const hasTopics = topics.length > 0; | ||
| const showGenerateState = !hasTopics && onGenerateHighlights; |
There was a problem hiding this comment.
showGenerateState is currently computed as !hasTopics && onGenerateHighlights, which makes its type false | (() => void) (truthy function) rather than an explicit boolean. This is easy to misread and can lead to accidental non-boolean usage later.
Prefer const showGenerateState = !hasTopics && Boolean(onGenerateHighlights); (or !!onGenerateHighlights) to keep the intent and type clear.
| const showGenerateState = !hasTopics && onGenerateHighlights; | |
| const showGenerateState = !hasTopics && Boolean(onGenerateHighlights); |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ee45ae838f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (youtubePlayerRef.current?.seekTo(time)) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Short-circuiting requestSeek when youtubePlayerRef.current.seekTo(time) succeeds leaves any previously queued playbackCommand intact, so a stale command can still fire ~50ms later in YouTubePlayer and override the user's newer seek. This is user-visible when a prior command (for example PLAY_TOPIC) has already scheduled execution and the user clicks another timestamp immediately afterward; before this change, updating playbackCommand to SEEK would replace and cancel the older command.
Useful? React with 👍 / 👎.
Summary
Generate highlight reelsbutton.Details
originis set for more reliable local seeking.Validation
node --import tsx --test components/__tests__/youtube-player.timeline.test.ts components/__tests__/highlights-panel.generate.test.tsx app/analyze/__tests__/right-column-layout.test.ts app/analyze/__tests__/summary-background-flow.test.tsnpm run lintnpm run build