Use puppeteer-core for browser based fetching to avoid API breakage - #29
Open
DominicTWHV wants to merge 4 commits into
Open
Use puppeteer-core for browser based fetching to avoid API breakage#29DominicTWHV wants to merge 4 commits into
DominicTWHV wants to merge 4 commits into
Conversation
WalkthroughPuppeteer 기반 브라우저 자동화를 도입해 YouTube 재생 페이지의 DOM을 직접 파싱하여 자막과 메타데이터를 추출하도록 전환하고, 브라우저 생명주기(shutdownFetcher)와 신호 기반 서버 종료(handleSignal)를 통합했다. Changes
Sequence DiagramsequenceDiagram
actor Client
participant Server
participant Browser as "Puppeteer Browser"
participant Page as "YouTube Page"
participant DOM as "DOM Parser"
Client->>Server: getSubtitles(videoID, lang)
activate Server
Server->>Browser: getBrowser()
activate Browser
Browser-->>Server: Browser Instance
deactivate Browser
Server->>Page: newPage() / goto(watch URL)
activate Page
Page-->>Server: page loaded
Server->>DOM: parse captionTracks & metadata from HTML
activate DOM
DOM-->>Server: captionTracks, metadata
deactivate DOM
Server->>Server: selectLanguage(requested, available)
Server->>Page: open transcript panel / scrollTranscriptPanel()
Page-->>Server: all segments loaded
Server->>DOM: extractTranscriptSegments()
DOM-->>Server: TranscriptSegment[]
Server->>Server: toTranscriptLines(segments)
Server-->>Client: SubtitleResult
deactivate Page
deactivate Server
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 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 Tip CodeRabbit can suggest fixes for GitHub Check annotations.Configure the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/index.ts`:
- Around line 346-350: The stop() method currently awaits shutdownFetcher()
before this.server.close(), so if shutdownFetcher() rejects the server.close()
call is skipped; change stop() to ensure both cleanup tasks run independently by
either awaiting shutdownFetcher() inside a try/catch and then always calling
await this.server.close() in a finally block, or run both with
Promise.allSettled([shutdownFetcher(), this.server.close()]) and handle any
rejections/logging; update references in stop(), shutdownFetcher(), and
this.server.close() accordingly so server shutdown always executes even if
fetcher cleanup fails.
In `@src/youtube-fetcher.ts`:
- Around line 136-153: The regex used in parseCaptionTracks to extract
"captionTracks" is too lazy and can break on nested arrays (e.g.,
track.name.runs), causing JSON.parse to fail; update parseCaptionTracks to
extract the full captionTracks array robustly by locating the "captionTracks"
key and then iterating characters to find the matching closing bracket (balance
'[' and ']' and nested objects) or by parsing the surrounding JSON blob (e.g.,
player response) into an object and reading its captionTracks, then continue
using uniqueLanguageCodes and the existing mapping (languageCode, name fallback,
isAutoGenerated) so behavior remains the same.
- Around line 100-106: 현재 fetcher 모듈의 process signal 핸들러(closeBrowser 호출)를 제거하고,
모든 프로세스 신호 처리(SIGHUP/SIGTERM 포함)를 src/index.ts의 TranscriptServer로 통합하세요:
src/youtube-fetcher.ts에서 process.once('exit', ... )와 process.once('SIGTERM', ...
) 블록을 삭제하고 대신 TranscriptServer의 기존 SIGINT 처리 로직(현재 line 238)을 확장하여 SIGTERM을 수신하면
shutdownFetcher()를 호출하고 필요한 비동기 정리(closeBrowser 내부 동작 포함)를 await 하여 정상 종료하도록
처리하도록 수정하세요; 즉, fetcher의 closeBrowser()를 직접 호출하지 말고 공개된 shutdownFetcher()를 사용하고,
TranscriptServer에서 Promise를 올바로 기다리도록 구현하세요.
- Around line 78-82: The code always passes '--no-sandbox' and
'--disable-setuid-sandbox' to puppeteer.launch (in the browserPromise
assignment), which disables Chromium sandboxing by default; change this to
conditionally include these args only when an environment flag is set (e.g.,
process.env.DISABLE_BROWSER_SANDBOX or NO_SANDBOX) so typical local/secure
environments keep sandboxing; update the browserPromise creation (the
puppeteer.launch call that uses resolveChromeExecutablePath()) to build the args
array based on that env flag and document the env var usage in a comment.
- Around line 76-84: The current getBrowser() stores a rejected promise in
browserPromise when puppeteer.launch() fails, causing permanent failures; update
getBrowser(), closeBrowser(), and shutdownFetcher() to clear/reset
browserPromise to null on any launch/close rejection so subsequent calls can
retry: wrap the puppeteer.launch() call (and any browser.close() usage) to catch
errors and in the catch handler set browserPromise = null (and rethrow or return
a controlled error), ensuring that getBrowser() only caches a successful browser
instance and that rejected promises are not left cached.
- Around line 44-52: CHROME_CANDIDATES currently only covers Linux and misses
macOS, Windows and PATH lookups; update the list used by CHROME_CANDIDATES and
the resolveChromeExecutablePath function to include macOS default
"/Applications/Google Chrome.app/Contents/MacOS/Google Chrome", Windows defaults
like "C:\\Program Files\\Google\\Chrome\\Application\\chrome.exe" and
"%LOCALAPPDATA%\\Google\\Chrome\\Application\\chrome.exe" (expand %LOCALAPPDATA%
at runtime), and also search the PATH by attempting to resolve "google-chrome",
"chrome", and "chromium" via process.env.PATH (or a cross-platform which/where
lookup). Ensure existing env vars (PUPPETEER_EXECUTABLE_PATH, CHROME_PATH,
GOOGLE_CHROME_BIN) remain first in order and keep filtering out non-existent
entries before returning the resolved executable from
resolveChromeExecutablePath.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6009eea2-f9df-4159-926b-bf7174aefcec
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
package.jsonsrc/index.tssrc/youtube-fetcher.ts
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/youtube-fetcher.ts (2)
429-439: 선택자 타임아웃 시 사용자 친화적인 에러 메시지 고려
waitForSelector타임아웃 발생 시 Puppeteer의 기본 에러 메시지는 사용자가 이해하기 어려울 수 있습니다. 예를 들어, transcript 버튼이 없는 영상(자막 미지원)의 경우 더 명확한 에러 메시지가 도움이 될 수 있습니다.♻️ 선택적 개선 예시
- await page.waitForSelector('ytd-video-description-transcript-section-renderer button', { timeout: 10000 }); + await page.waitForSelector('ytd-video-description-transcript-section-renderer button', { timeout: 10000 }) + .catch(() => { + throw new Error('Transcript button not found. This video may not have captions available.'); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/youtube-fetcher.ts` around lines 429 - 439, Wrap the transcript-related waits (the calls to page.waitForSelector for 'ytd-video-description-transcript-section-renderer button' and 'ytd-engagement-panel-section-list-renderer[target-id="PAmodern_transcript_view"]' and the page.waitForFunction checking '.ytwTranscriptSegmentViewModelHost') in try/catch blocks; on timeout/throw, catch the error and rethrow or return a clearer, user-friendly error (e.g., "Transcript not available for this video" or "Transcript UI not found") that references the specific selector or step that failed, so consumers of fetchTranscript (or whichever function contains these calls) get a descriptive message instead of the raw Puppeteer timeout. Ensure the error preserves the original error for debugging (attach as cause or include original message).
334-337:availableLanguages가 비어있을 때의 동작 검토 필요
availableLanguages.length === 0일 때requestedLanguage를 그대로 반환하면, 실제로 사용 가능한 언어가 없는 영상에서도 transcript 가져오기를 시도하게 됩니다. 이 경우fetchTranscriptSegments가 빈 결과를 반환하고 Line 521-523에서 에러가 발생하므로 동작은 정상이지만, 에러 메시지가 명확하지 않을 수 있습니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/youtube-fetcher.ts` around lines 334 - 337, The function selectLanguage currently returns the requestedLanguage when availableLanguages is empty, which causes fetchTranscriptSegments to be called on an unavailable language and yields an opaque error later; change selectLanguage to detect availableLanguages.length === 0 and instead throw or return a sentinel (e.g., empty string/null) with a clear message so callers can handle it; update the caller of selectLanguage (the code that calls fetchTranscriptSegments) to check for that sentinel or catch the thrown Error and surface a clear error message like "no caption tracks available for this video" before attempting fetchTranscriptSegments.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/youtube-fetcher.ts`:
- Around line 429-439: Wrap the transcript-related waits (the calls to
page.waitForSelector for 'ytd-video-description-transcript-section-renderer
button' and
'ytd-engagement-panel-section-list-renderer[target-id="PAmodern_transcript_view"]'
and the page.waitForFunction checking '.ytwTranscriptSegmentViewModelHost') in
try/catch blocks; on timeout/throw, catch the error and rethrow or return a
clearer, user-friendly error (e.g., "Transcript not available for this video" or
"Transcript UI not found") that references the specific selector or step that
failed, so consumers of fetchTranscript (or whichever function contains these
calls) get a descriptive message instead of the raw Puppeteer timeout. Ensure
the error preserves the original error for debugging (attach as cause or include
original message).
- Around line 334-337: The function selectLanguage currently returns the
requestedLanguage when availableLanguages is empty, which causes
fetchTranscriptSegments to be called on an unavailable language and yields an
opaque error later; change selectLanguage to detect availableLanguages.length
=== 0 and instead throw or return a sentinel (e.g., empty string/null) with a
clear message so callers can handle it; update the caller of selectLanguage (the
code that calls fetchTranscriptSegments) to check for that sentinel or catch the
thrown Error and surface a clear error message like "no caption tracks available
for this video" before attempting fetchTranscriptSegments.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 062cae07-47bb-4c0e-a92a-e55886b6b0a4
📒 Files selected for processing (3)
README.mdsrc/index.tssrc/youtube-fetcher.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/youtube-fetcher.ts (3)
382-418: 매직 넘버를 명명된 상수로 추출하면 가독성이 향상됩니다.
20,2,250,16같은 값들의 의미가 명확하지 않습니다. 명명된 상수로 추출하면 코드의 의도를 더 명확하게 전달할 수 있습니다.개선 제안
+const SCROLL_MAX_ATTEMPTS = 20; +const SCROLL_STABLE_READS_REQUIRED = 2; +const SCROLL_POLL_INTERVAL_MS = 250; +const SCROLL_HEIGHT_THRESHOLD = 16; + async function scrollTranscriptPanel(page: import('puppeteer-core').Page): Promise<void> { let previousCount = -1; let stableReads = 0; - for (let attempt = 0; attempt < 20 && stableReads < 2; attempt++) { + for (let attempt = 0; attempt < SCROLL_MAX_ATTEMPTS && stableReads < SCROLL_STABLE_READS_REQUIRED; attempt++) { // ... polling logic ... const scrollers = [panel, ...Array.from(panel.querySelectorAll('*'))].filter((element) => { const htmlElement = element as HTMLElement; - return htmlElement.scrollHeight > htmlElement.clientHeight + 16; + return htmlElement.scrollHeight > htmlElement.clientHeight + ${SCROLL_HEIGHT_THRESHOLD}; }) as HTMLElement[]; // ... - await new Promise((resolve) => setTimeout(resolve, 250)); + await new Promise((resolve) => setTimeout(resolve, SCROLL_POLL_INTERVAL_MS)); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/youtube-fetcher.ts` around lines 382 - 418, The loop in scrollTranscriptPanel uses magic numbers (20, 2, 250, 16) which reduces readability; extract them to named constants (e.g., MAX_SCROLL_ATTEMPTS = 20, REQUIRED_STABLE_READS = 2, SCROLL_SLEEP_MS = 250, SCROLL_HEIGHT_THRESHOLD_PX = 16) declared near the top of the file or immediately above the scrollTranscriptPanel function, then replace the hardcoded literals in the for-loop condition, stableReads check, setTimeout delay, and scrollHeight > clientHeight + threshold comparison so the intent is clear and maintainable.
420-498: DOM 셀렉터를 상수로 추출하면 유지보수가 용이해집니다.YouTube의 내부 클래스명(
.ytwTranscriptSegmentViewModelHost,.ytwTranscriptSegmentViewModelTimestamp등)은 언제든 변경될 수 있습니다. 이러한 셀렉터들을 파일 상단에 상수로 정의하면, YouTube UI가 변경되었을 때 수정 지점을 쉽게 찾을 수 있습니다.개선 제안
const REQUEST_TIMEOUT = 30000; const WATCH_USER_AGENT = 'Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/122.0.0.0 Safari/537.36'; const CHROME_COMMAND_CANDIDATES = ['google-chrome', 'google-chrome-stable', 'chromium-browser', 'chromium', 'chrome', 'chrome.exe']; + +// YouTube DOM selectors - may need updates if YouTube changes their UI +const SELECTORS = { + transcriptButton: 'ytd-video-description-transcript-section-renderer button', + transcriptPanel: 'ytd-engagement-panel-section-list-renderer[target-id="PAmodern_transcript_view"]', + transcriptSegment: '.ytwTranscriptSegmentViewModelHost', + timestamp: '.ytwTranscriptSegmentViewModelTimestamp', + segmentText: '.yt-core-attributed-string' +} as const;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/youtube-fetcher.ts` around lines 420 - 498, The fetchTranscriptSegments function hardcodes fragile DOM selectors (e.g., 'ytd-video-description-transcript-section-renderer button', 'ytd-engagement-panel-section-list-renderer[target-id="PAmodern_transcript_view"]', '.ytwTranscriptSegmentViewModelHost', '.ytwTranscriptSegmentViewModelTimestamp', '.yt-core-attributed-string'); extract these into well-named constants at the top of the file (e.g., TRANSCRIPT_BUTTON_SELECTOR, TRANSCRIPT_PANEL_SELECTOR, TRANSCRIPT_SEGMENT_SELECTOR, TIMESTAMP_SELECTOR, ATTRIBUTED_STRING_SELECTOR) and replace every inline literal in fetchTranscriptSegments (including page.waitForSelector, page.evaluate, page.$$eval, waitForFunction) with those constants so future UI class changes are easy to update in one place.
500-510: 마지막 세그먼트의dur이 항상 0입니다.
nextStart가 현재 세그먼트의start와 동일하게 설정되어 마지막 세그먼트의 지속 시간이 0이 됩니다. 이는 기술적으로 정확하지 않으며, API 소비자에게 혼란을 줄 수 있습니다. 평균 지속 시간을 기반으로 추정값을 제공하거나 최소 기본값(예: 5초)을 사용하는 것을 고려해 보세요.개선 제안
function toTranscriptLines(segments: TranscriptSegment[]): TranscriptLine[] { + const DEFAULT_LAST_SEGMENT_DURATION = 5; // seconds + return segments.map((segment, index) => { const start = parseTimestampToSeconds(segment.timestamp); - const nextStart = index < segments.length - 1 ? parseTimestampToSeconds(segments[index + 1].timestamp) : start; + const nextStart = index < segments.length - 1 + ? parseTimestampToSeconds(segments[index + 1].timestamp) + : start + DEFAULT_LAST_SEGMENT_DURATION; return { text: segment.text, start, dur: Math.max(nextStart - start, 0) }; }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/youtube-fetcher.ts` around lines 500 - 510, toTranscriptLines sets the last segment's dur to 0 because nextStart equals start; change to compute durations for all non-last segments as now, then for the final segment estimate its duration as Math.max(averageDurationOfPreviousSegments, MIN_DUR) (choose MIN_DUR = 5 seconds) and ensure all dur values are non-negative; update the toTranscriptLines function to calculate averageDuration from earlier computed durations (or fallback to MIN_DUR if none) and assign that to the last TranscriptLine so API consumers get a reasonable duration instead of 0.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/youtube-fetcher.ts`:
- Around line 544-548: The catch block's error comparison doesn't match the
message thrown by selectLanguage (selectLanguage throws 'No caption tracks
available for this video in any language.'), so update the condition in the
catch (error) handler to either compare against the exact string used by
selectLanguage or use a more robust check (e.g., error.message.startsWith('No
caption tracks available for this video')) so the special-case
createDescriptiveError call for missing captions will actually run; reference
the selectLanguage function and the catch block that currently checks for 'No
caption tracks available for this video.' when making the change.
---
Nitpick comments:
In `@src/youtube-fetcher.ts`:
- Around line 382-418: The loop in scrollTranscriptPanel uses magic numbers (20,
2, 250, 16) which reduces readability; extract them to named constants (e.g.,
MAX_SCROLL_ATTEMPTS = 20, REQUIRED_STABLE_READS = 2, SCROLL_SLEEP_MS = 250,
SCROLL_HEIGHT_THRESHOLD_PX = 16) declared near the top of the file or
immediately above the scrollTranscriptPanel function, then replace the hardcoded
literals in the for-loop condition, stableReads check, setTimeout delay, and
scrollHeight > clientHeight + threshold comparison so the intent is clear and
maintainable.
- Around line 420-498: The fetchTranscriptSegments function hardcodes fragile
DOM selectors (e.g., 'ytd-video-description-transcript-section-renderer button',
'ytd-engagement-panel-section-list-renderer[target-id="PAmodern_transcript_view"]',
'.ytwTranscriptSegmentViewModelHost', '.ytwTranscriptSegmentViewModelTimestamp',
'.yt-core-attributed-string'); extract these into well-named constants at the
top of the file (e.g., TRANSCRIPT_BUTTON_SELECTOR, TRANSCRIPT_PANEL_SELECTOR,
TRANSCRIPT_SEGMENT_SELECTOR, TIMESTAMP_SELECTOR, ATTRIBUTED_STRING_SELECTOR) and
replace every inline literal in fetchTranscriptSegments (including
page.waitForSelector, page.evaluate, page.$$eval, waitForFunction) with those
constants so future UI class changes are easy to update in one place.
- Around line 500-510: toTranscriptLines sets the last segment's dur to 0
because nextStart equals start; change to compute durations for all non-last
segments as now, then for the final segment estimate its duration as
Math.max(averageDurationOfPreviousSegments, MIN_DUR) (choose MIN_DUR = 5
seconds) and ensure all dur values are non-negative; update the
toTranscriptLines function to calculate averageDuration from earlier computed
durations (or fallback to MIN_DUR if none) and assign that to the last
TranscriptLine so API consumers get a reasonable duration instead of 0.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e2e84c67-d4c2-4494-8664-699c7e77db6f
📒 Files selected for processing (1)
src/youtube-fetcher.ts
Comment on lines
+544
to
+548
| } catch (error) { | ||
| if (error instanceof Error && error.message === 'No caption tracks available for this video.') { | ||
| throw createDescriptiveError('No caption tracks available for this video.', error); | ||
| } | ||
| throw error; |
There was a problem hiding this comment.
에러 메시지 불일치로 인해 특수 처리가 동작하지 않습니다.
selectLanguage 함수(Line 341)는 'No caption tracks available for this video in any language.'를 throw하지만, 여기서는 'No caption tracks available for this video.'와 비교하고 있습니다. "in any language" 부분이 다르므로 이 조건은 절대 참이 되지 않습니다.
수정 제안
} catch (error) {
- if (error instanceof Error && error.message === 'No caption tracks available for this video.') {
- throw createDescriptiveError('No caption tracks available for this video.', error);
- }
+ if (error instanceof Error && error.message === 'No caption tracks available for this video in any language.') {
+ throw createDescriptiveError('No caption tracks available for this video in any language.', error);
+ }
throw error;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/youtube-fetcher.ts` around lines 544 - 548, The catch block's error
comparison doesn't match the message thrown by selectLanguage (selectLanguage
throws 'No caption tracks available for this video in any language.'), so update
the condition in the catch (error) handler to either compare against the exact
string used by selectLanguage or use a more robust check (e.g.,
error.message.startsWith('No caption tracks available for this video')) so the
special-case createDescriptiveError call for missing captions will actually run;
reference the selectLanguage function and the catch block that currently checks
for 'No caption tracks available for this video.' when making the change.
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.
Using headless Chrome to avoid YouTube private API breakages.
Tested to function properly.
Requires Chrome or Chromium installed locally. Set
CHROME_PATHorPUPPETEER_EXECUTABLE_PATHif it is not available at a standard system path.Summary by CodeRabbit
개선 사항
유지보수