Conversation
…#152) - Remove detail parameter from system MessageBox to prevent window overflow - Display only essential update information with version number - Direct users to Settings > About page for full release notes - Remove unused formatReleaseNotes method and ReleaseNoteInfo interface Fixes issue where long release notes caused system dialog to overflow screen height, hiding the Install button.
Remove all keyboard shortcuts for subtitle display modes: - subtitle_mode_none (Cmd/Ctrl+1) - subtitle_mode_original (Cmd/Ctrl+2) - subtitle_mode_translated (Cmd/Ctrl+3) - subtitle_mode_bilingual (Cmd/Ctrl+4) Changes: - Remove shortcut configurations from DEFAULT_SHORTCUTS - Remove shortcut handlers from usePlayerShortcuts hook - Remove i18n label references - Clean up unused imports and fix formatting Users can still change subtitle display modes through the UI interface.
…ncy logging (#156) - Reduce Logger export history from 10k to 1k entries and add automatic cleanup - Implement lightweight serialization to replace deep serialization - Add sampling strategy for high-frequency logs (time_update, MediaClock events) - Disable export history completely in production environment - Optimize MediaClock logging with 5-10% sampling for duplicates and debug messages - Reduce PlayerOrchestrator trace buffer from 200 to 50 entries - Remove high-frequency onTimeUpdate silly logs - Implement stricter production log levels (WARN+ for renderer, ERROR+ for main process) This should significantly reduce memory pressure from logger-related objects as identified in Chrome DevTools memory analysis.
…e state sync (#153) * fix(subtitle): resolve overlay pause/seek update delays with immediate state sync - Add immediate store updates in PlayerOrchestrator during seek/jumpToCue - Implement UserSeeking state in PlayerSettingsSaver to prevent conflicts - Ensure subtitle overlay UI responds instantly to user interactions - Maintain data consistency during user-initiated time changes * Update src/renderer/src/pages/player/engine/PlayerOrchestrator.ts Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * Update src/renderer/src/services/PlayerSettingsSaver.ts Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * fix(player): resolve seeking event mismatch causing duplicate subtitle jumps - Add missing mediaClock.startSeeking() call in PlayerOrchestrator.onSeeking() - Fix SeekEventCoordinator state inconsistency between start/end seeking events - Eliminate 'Ignoring seeked without active seeking' warnings in console - Ensure proper event sequence for both paused and playing states - All 66 player engine tests pass with TypeScript and lint validation Resolves issue where users needed to press subtitle buttons twice in paused state. * refactor(player): Use destroyOnHidden instead of destoryOnClose. * fix(player): prevent SubtitleSyncStrategy from overriding user subtitle jumps - Lock subtitle state machine for 2 seconds after user subtitle jump - Prevent SubtitleSyncStrategy from immediately overriding user selection - Add automatic unlock mechanism to restore normal subtitle sync behavior - Ensure 'goToNextSubtitle' respects user intent in paused state - Fix seeking event coordination in PlayerOrchestrator.onSeeking() Resolves the core issue where clicking to jump to a subtitle would be immediately overridden by the subtitle sync strategy, requiring users to click twice to achieve their intended jump. * fix(subtitle): eliminate subtitle content flickering during jumps - Add activeCueIndex to PlayerState and StateUpdater interface - Sync PlayerOrchestrator's activeCueIndex to store on updates - Modify useSubtitleEngine to prioritize store's activeCueIndex over time-based calculation - Ensure SubtitleContent component uses authoritative subtitle index - Fix initial activeCueIndex synchronization on StateUpdater connection Resolves flickering where subtitle content would briefly show wrong subtitle before displaying the correct one during user-initiated subtitle jumps. * fix(subtitle): eliminate subtitle overlay flickering during jumps - Add smart tolerance mechanism to useSubtitleOverlay shouldShow calculation - Display overlay when currentTime is within 2 seconds of subtitle start time - Handles user jump delays while preserving smooth normal playback behavior - Fixes flickering issue where overlay would briefly hide during subtitle navigation Closes final flickering issue in subtitle navigation system. * fix(subtitle): resolve overlay flickering through index priority and data stabilization - Add stable subtitle data memoization in useSubtitleOverlay to prevent unnecessary re-renders - Prioritize currentIndex over time-based checks in shouldShow calculation - Reorder PlayerOrchestrator lock sequence to ensure SubtitleLockFSM is active during updateContext - Implement granular dependency tracking for subtitle content changes --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…anagement (#155) * feat(ffmpeg): implement dynamic FFmpeg download system with runtime management - Remove static FFmpeg bundling from build configuration - Add FFmpegDownloadService for cross-platform binary management - Implement automatic platform detection (Windows/macOS/Linux, x64/ARM64) - Add IPC channels for download progress monitoring and control - Integrate download service with existing FFmpegService architecture - Update build scripts to remove prebuild FFmpeg requirements - Add comprehensive test coverage for download functionality Changes: - electron-builder.yml: Remove extraResources FFmpeg bundling - package.json: Remove prebuild FFmpeg download from release scripts - FFmpegDownloadService.ts: New service with download/extract/management capabilities - FFmpegService.ts: Enhanced with download service integration and fallback logic - IpcChannel.ts: Add 9 new channels for download operations - ipc.ts: Register download service handlers for renderer communication - preload/index.ts: Expose download APIs to renderer process - useVideoFileSelect.ts: Updated to work with dynamic FFmpeg detection This implementation enables on-demand FFmpeg installation, reducing app bundle size by ~200MB while maintaining cross-platform compatibility and user experience. The system gracefully falls back to bundled → downloaded → system FFmpeg. * feat(settings): implement FFmpeg settings UI with download management - Add new FFmpegSettings component with status indicator and download controls - Remove deprecated FFmpeg build plugin from electron.vite.config.ts - Enhance IndicatorLight component with proper CSS-in-JS animation syntax - Add comprehensive i18n support for FFmpeg management (en-us, zh-cn) - Remove box-shadow from ant-btn components for cleaner UI appearance - Integrate FFmpeg settings into main SettingsPage navigation Changes: - FFmpegSettings.tsx: Complete UI implementation with download progress, path validation, and status management - electron.vite.config.ts: Remove build-time FFmpeg download plugin (shift to runtime approach) - IndicatorLight.tsx: Fix styled-components animation with proper css helper - i18n locales: Add 61 new translation keys for FFmpeg settings UI - ant.scss: Remove button shadows for consistent design system - SettingsPage.tsx: Add FFmpeg settings tab integration This implements the frontend interface for the dynamic FFmpeg download system, providing users with a comprehensive management UI for FFmpeg installation, status monitoring, and path configuration. * feat(ffmpeg): implement FFmpeg guidance dialog for enhanced user experience Add comprehensive FFmpeg download guidance system that transforms technical errors into user-friendly guidance with seamless navigation to settings and auto-download. **Components Added:** - FFmpegDownloadPrompt: Full-featured guidance dialog with benefits, effort info, and actions - Comprehensive internationalization support (zh-CN, en-US) **Hook Enhancements:** - useVideoFileSelect: Extended with FFmpeg prompt state management - Replaced technical error throwing with guided dialog display - Enhanced error detection for FFmpeg missing scenarios **Integration Updates:** - HomePage: State management for prompt visibility and component integration - EmptyState/VideoAddButton: Bidirectional state communication with parent - HeaderNavbar: Props forwarding for prompt handler **Features:** - Benefits explanation (compatibility, performance, reliability) - Installation effort communication - Auto-navigation to settings with download trigger - Seamless integration with existing video file selection workflow - Graceful error handling with user-centric messaging **Technical Details:** - Styled-components with theme variables and CSS custom properties - Modal-based UI with responsive design and accessibility - State management across component hierarchy - URL parameter-based auto-download triggering - Comprehensive TypeScript interfaces Transforms "视频处理组件未安装" technical errors into guided user experience that educates users about FFmpeg benefits and provides immediate resolution path. * test: fix FFmpegService mock for dynamic download system - Add getDownloadService method to FFmpegService mock - Include all FFmpegDownloadService interface methods in mock - Fix 43 failing test cases caused by missing mock method - All 554 test cases now pass successfully Resolves test failures introduced by FFmpeg dynamic download system implementation. * test: fix cross-platform FFmpeg executable name test - Update test to handle platform-specific executable names (ffmpeg vs ffmpeg.exe) - Fix Windows CI test failure where test expected 'ffmpeg' but got 'ffmpeg.exe' - Test now correctly validates system FFmpeg fallback behavior on all platforms - Maintains test coverage while supporting cross-platform compatibility Resolves Windows CI test failure in FFmpegService integration tests.
- Add arm64 architecture support for Windows NSIS installer target - Add arm64 architecture support for Windows Portable target - Align Windows build targets with existing macOS and Linux ARM64 support - Enable native ARM64 builds for Windows on ARM devices This change allows the application to run natively on Windows ARM64 devices, providing better performance and compatibility for users with ARM-based Windows machines.
- Configure Windows and Linux to use native system title bars instead of custom titleBarOverlay - Remove excessive padding-right in Navbar component (140px for Windows, 120px for Linux) - Maintain custom title bar for macOS with traffic light buttons integration - Update ThemeService to only apply titleBarOverlay changes to macOS windows - Simplify Navbar styling to use consistent 12px padding across all platforms This change eliminates the spacing issue in the Windows header area and provides a more native user experience on Windows and Linux platforms. Fixes the header spacing issue reported for Windows platform.
WalkthroughRemoves bundled FFmpeg handling from build/release, adds a runtime FFmpeg download/management service with IPC and renderer UI. Introduces platform-aware title bar configs. Updates player engine to track/push active subtitle cue index and refine seek flow and logging. Adds i18n, tests, and CSS tweaks. Copies DB migrations during build. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UI as Renderer UI (Settings/Prompt)
participant Preload as Preload (window.api.ffmpeg)
participant IPC as Main IPC
participant Svc as FFmpegService
participant DL as FFmpegDownloadService
User->>UI: Open Plugins / confirm Download
UI->>Preload: ffmpeg.getInfo()
Preload->>IPC: Ffmpeg_GetInfo
IPC->>Svc: getFFmpegInfo()
Svc-->>IPC: info
IPC-->>Preload: info
Preload-->>UI: info (needsDownload?)
alt needsDownload
UI->>Preload: ffmpeg.download.download()
Preload->>IPC: FfmpegDownload_Download
IPC->>Svc: getDownloadService()
IPC->>DL: downloadFFmpeg()
note over DL: downloading -> extracting -> completed
loop poll progress
UI->>Preload: ffmpeg.download.getProgress()
Preload->>IPC: FfmpegDownload_GetProgress
IPC->>DL: getDownloadProgress()
DL-->>IPC: progress
IPC-->>Preload: progress
Preload-->>UI: progress
end
UI->>Preload: ffmpeg.getInfo()
Preload->>IPC: Ffmpeg_GetInfo
IPC->>Svc: getFFmpegInfo()
Svc-->>IPC: updated info
IPC-->>Preload: updated info
Preload-->>UI: updated info (installed)
else cancel
UI->>Preload: ffmpeg.download.cancel()
Preload->>IPC: FfmpegDownload_Cancel
IPC->>DL: cancelDownload()
end
sequenceDiagram
autonumber
participant User as User
participant Orchestrator as PlayerOrchestrator
participant Saver as PlayerSettingsSaver
participant Store as PlayerStore
participant Clock as MediaClock
participant Lock as SubtitleLockFSM
User->>Orchestrator: requestUserSeek(time)
Orchestrator->>Saver: markUserSeeking()
Orchestrator->>Store: setCurrentTime(time)
Orchestrator->>Clock: startSeeking()
Orchestrator->>Lock: lock("user_seek", index?)
Orchestrator->>Store: setActiveCueIndex(index?)
Note over Orchestrator: Perform actual seek...
Orchestrator->>Lock: unlock("user_seek") (after delay)
Saver->>Saver: resume autosave (after delay)
Saver->>Store: save current progress immediately
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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 |
There was a problem hiding this comment.
Actionable comments posted: 46
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (25)
src/main/services/AppUpdater.ts (3)
279-287: Clarify dialog UX, prefer detail for secondary text, and localize strings.
- Use “Restart and Install” to reflect quitAndInstall behavior.
- Move the “release notes” hint to
detailfor better readability on Windows.- Route strings through your i18n layer instead of hardcoded English.
.showMessageBox({ type: 'info', - title: 'Update available', + title: 'Update available', icon, - message: `A new version (${this.releaseInfo.version}) is available. Do you want to install it now?\n\nYou can view the release notes in Settings > About.`, - buttons: ['Later', 'Install'], + message: `A new version (${this.releaseInfo.version}) is ready to install.`, + detail: 'You can view the release notes in Settings > About.', + buttons: ['Later', 'Restart and Install'], defaultId: 1, cancelId: 0 })
48-50: Make logger second arg an object literal per guidelines.Several calls pass non-literal values. Normalize to
{ key: value }for consistency and structured logs.- logger.info('检测到新版本', releaseInfo) + logger.info('检测到新版本', { releaseInfo }) - logger.info('下载完成', releaseInfo) + logger.info('下载完成', { releaseInfo }) - logger.info('release info', release) + logger.info('release info', { release }) - logger.error('Failed to get latest not draft version from github:', error) + logger.error('Failed to get latest not draft version from github:', { error }) - logger.error('Failed to get ipinfo:', error) + logger.error('Failed to get ipinfo:', { error }) - logger.info('Setting feed URL - testPlan:', testPlan) + logger.info('Setting feed URL - testPlan:', { testPlan }) if (channel === UpgradeChannel.LATEST) { this.autoUpdater.channel = UpgradeChannel.LATEST this.autoUpdater.setFeedURL(FeedUrl.GITHUB_LATEST) - logger.info('Using GitHub latest releases for test plan') + logger.info('Using GitHub latest releases for test plan') - logger.info('Detected IP country:', ipCountry) + logger.info('Detected IP country:', { ipCountry }) if (ipCountry.toLowerCase() !== 'cn') { this.autoUpdater.setFeedURL(FeedUrl.GITHUB_LATEST) - logger.info('Using GitHub releases for non-CN region') + logger.info('Using GitHub releases for non-CN region') } - logger.error('Failed to download update:', error) + logger.error('Failed to download update:', { error }) - logger.info('downloadUpdate manual by check for updates', this.cancellationToken) + logger.info('downloadUpdate manual by check for updates', { cancellationToken: this.cancellationToken }) - logger.error('Failed to check for update:', error) + logger.error('Failed to check for update:', { error })Also applies to: 66-67, 96-99, 123-124, 164-169, 194-198, 212-213, 251-253, 265-266
79-91: Add timeout and UA to GitHub fetch to avoid hangs and rate-limit quirks.Mirror the
_getIpCountrytimeout pattern; includeUser-Agentfor GitHub API stability.): Promise<GithubReleaseInfo | null> { try { - logger.info('get pre release version from github', channel) + logger.info('get pre release version from github', { channel }) + const controller = new AbortController() + const timeoutId = setTimeout(() => controller.abort(), 5000) const responses = await fetch( 'https://api.github.com/repos/mkdir700/EchoPlayer/releases?per_page=8', { + signal: controller.signal, headers: { Accept: 'application/vnd.github+json', 'X-GitHub-Api-Version': '2022-11-28', - 'Accept-Language': 'en-US,en;q=0.9' + 'Accept-Language': 'en-US,en;q=0.9', + 'User-Agent': generateUserAgent() } } ) + clearTimeout(timeoutId) const data = (await responses.json()) as GithubReleaseInfo[]src/renderer/src/components/IndicatorLight.tsx (1)
50-57: Avoid hard-coded spacing; let parent control layout or use theme spacing.
Defaultmargin-left: 8px;leaks layout concerns into a leaf component.Minimal change (prefer parent spacing):
- margin-left: 8px; + /* spacing handled by parent container */src/main/services/WindowService.ts (1)
314-333: Critical: globally stripping X-Frame-Options/CSP invites clickjacking and injectionCurrent handler removes XFO/CSP for all requests. Scope this to a small allowlist (and dev server) or
file://only.private setupWebRequestHeaders(mainWindow: BrowserWindow) { mainWindow.webContents.session.webRequest.onHeadersReceived( { urls: ['*://*/*'] }, (details, callback) => { - if (details.responseHeaders?.['X-Frame-Options']) { - delete details.responseHeaders['X-Frame-Options'] - } - if (details.responseHeaders?.['x-frame-options']) { - delete details.responseHeaders['x-frame-options'] - } - if (details.responseHeaders?.['Content-Security-Policy']) { - delete details.responseHeaders['Content-Security-Policy'] - } - if (details.responseHeaders?.['content-security-policy']) { - delete details.responseHeaders['content-security-policy'] - } - callback({ cancel: false, responseHeaders: details.responseHeaders }) + const url = details.url + let origin = '' + try { + origin = new URL(url).origin + } catch {} + const allowlist = new Set([ + 'https://account.siliconflow.cn', + 'https://cloud.siliconflow.cn', + 'https://aihubmix.com' + ]) + const isDevServer = is.dev && origin === 'http://localhost:5173' + const isFile = url.startsWith('file://') + const shouldRelax = isFile || isDevServer || allowlist.has(origin) + + if (shouldRelax) { + if (details.responseHeaders?.['X-Frame-Options']) { + delete details.responseHeaders['X-Frame-Options'] + } + if (details.responseHeaders?.['x-frame-options']) { + delete details.responseHeaders['x-frame-options'] + } + if (details.responseHeaders?.['Content-Security-Policy']) { + delete details.responseHeaders['Content-Security-Policy'] + } + if (details.responseHeaders?.['content-security-policy']) { + delete details.responseHeaders['content-security-policy'] + } + } + callback({ cancel: false, responseHeaders: details.responseHeaders }) } ) }src/renderer/src/pages/player/components/VideoErrorRecovery.tsx (1)
162-169: Lock modal from ESC and remove no-oponCancel.You already disallow close buttons and mask clicks. To avoid ESC attempting to cancel, explicitly set
keyboard={false}and droponCancel={undefined}for clarity.Apply:
- open={open} - onCancel={undefined} + open={open} + keyboard={false}src/renderer/src/pages/player/engine/MediaClock.ts (1)
617-625: Same: centralize sampling for high-precision updates- // 在高精度模式下添加额外的调试信息 - 降低频率 - if (this.throttler.getMode() === ThrottleMode.HIGH_PRECISION && Math.random() < 0.1) { + // 在高精度模式下添加额外的调试信息 - 降低频率 + if (this.throttler.getMode() === ThrottleMode.HIGH_PRECISION && shouldSample(0.1)) { logger.debug('High-precision time update (sampled)', { previousTime, currentTime, epsilon, throttleMode: this.throttler.getMode() }) }If any tests assert logs, adjust them to not depend on sampled entries (or gate sampling off in test mode using the helper).
src/renderer/src/hooks/useVideoFileSelect.ts (3)
84-105: Avoid orphan DB entries: defer FileManager.addFile until after videoInfo succeeds.
addFilePromiseruns in parallel and completes even whenvideoInfois null, leaving a file row without a matching VideoLibrary record when you early-return for FFmpeg. Move the add to aftervideoInfois validated (or implement rollback). Also fix the timing markers accordingly.Apply:
- monitor.startTiming('文件数据库添加') - const addFilePromise = FileManager.addFile(file) - ... - // 等待并行操作完成 - const [addedFile, videoInfo] = await Promise.all([addFilePromise, videoInfoPromise]) - - monitor.endTiming('文件数据库添加') + // 等待解析完成,再入库,避免孤儿记录 + const videoInfo = await videoInfoPromise + if (!videoInfo) { + // 下方已有 FFmpeg 分支处理 + } + monitor.startTiming('文件数据库添加') + const addedFile = await FileManager.addFile(file) + monitor.endTiming('文件数据库添加')Also applies to: 101-113
160-178: Performance monitor leak/gap on early returns.When you early-return for FFmpeg prompt, the monitor session is never finished. Either call a lightweight
monitor.finish()before returning or restructure to create the monitor after FFmpeg checks.
217-221: Consider i18n for toasts.Hardcoded strings (“添加文件失败”, “视频文件添加成功!”) bypass i18n. Prefer t('...') to keep locale consistency.
src/renderer/src/i18n/locales/zh-cn.json (2)
47-106: Key style consistency: avoid mixing kebab-case and camelCase.
background-type/display-modevsbackgroundType/displayMode. Standardize to reduce i18n lookup mistakes.Also applies to: 259-287
6-21: Duplicate semantics for “取消”.
home.delete.button_cancelduplicatescommon.cancel. Reuse the common key to simplify maintenance.Also applies to: 27-35
src/renderer/src/pages/settings/SettingsPage.tsx (1)
93-93: Syntax error in styled component closing tagExtraneous bracket breaks compilation.
Apply this diff:
-]` +`src/renderer/src/state/stores/player.store.ts (1)
156-159: Incorrect default currentFavoriteIndex (out of bounds)favoriteRates defaults to [1.0] but index is 1 (invalid).
Apply this diff:
- favoriteRates: [1.0], // 默认常用速度 - currentFavoriteIndex: 1, // 默认选择 1.0x + favoriteRates: [1.0], // 默认常用速度 + currentFavoriteIndex: 0, // 默认选择 1.0xsrc/renderer/src/pages/player/hooks/usePlayerShortcuts.ts (1)
11-11: Logger context name mismatchUse file/hook name for accurate logs.
-const logger = loggerService.withContext('TransportBar') +const logger = loggerService.withContext('usePlayerShortcuts')src/renderer/src/pages/home/HomePage.tsx (2)
73-89: Zustand usage should adopt selectors (guideline compliance)Current destructuring from stores violates our rule to use selectors or tuple selectors with shallow. Refactor to selectors to avoid unnecessary re-renders and align with policy.
-import { message, Modal, Tooltip } from 'antd' +import { message, Modal, Tooltip } from 'antd' +import { shallow } from 'zustand/shallow' @@ - const { t } = useTranslation() - const { videoListViewMode, setVideoListViewMode } = useSettingsStore() - const { - refreshTrigger, - isLoading, - isInitialized, - cachedVideos, - setLoading, - setInitialized, - setCachedVideos - } = useVideoListStore() + const { t } = useTranslation() + const [videoListViewMode, setVideoListViewMode] = useSettingsStore( + (s) => [s.videoListViewMode, s.setVideoListViewMode], + shallow + ) + const refreshTrigger = useVideoListStore((s) => s.refreshTrigger) + const isLoading = useVideoListStore((s) => s.isLoading) + const isInitialized = useVideoListStore((s) => s.isInitialized) + const cachedVideos = useVideoListStore((s) => s.cachedVideos) + const setLoading = useVideoListStore((s) => s.setLoading) + const setInitialized = useVideoListStore((s) => s.setInitialized) + const setCachedVideos = useVideoListStore((s) => s.setCachedVideos)Also applies to: 186-201
150-160: Avoid dangerouslySetInnerHTML and emoji icon (security + UI guideline)
- Replace dangerouslySetInnerHTML with Trans to avoid XSS footguns.
- Replace
⚠️ with lucide-react icon per guideline.-import { useTranslation } from 'react-i18next' +import { Trans, useTranslation } from 'react-i18next' +import { AlertTriangle } from 'lucide-react' @@ - content: ( - <div> - <p - dangerouslySetInnerHTML={{ - __html: t('home.delete.confirm_content', { title: video.title }) - }} - /> - <p style={{ color: 'var(--color-status-warning)', fontSize: '14px', marginTop: '8px' }}> - ⚠️ {t('home.delete.confirm_warning')} - </p> - </div> - ), + content: ( + <div> + <p> + <Trans i18nKey="home.delete.confirm_content" values={{ title: video.title }} /> + </p> + <p style={{ color: 'var(--color-status-warning)', fontSize: 14, marginTop: 8, display: 'flex', alignItems: 'center', gap: 8 }}> + <AlertTriangle size={14} /> + {t('home.delete.confirm_warning')} + </p> + </div> + ),src/main/ipc.ts (2)
223-241: Logger usage: pass structured object as second arg (guideline compliance)Several logger.error calls pass Error directly or inline strings. Our convention requires an object literal as the second param.
- logger.error('Failed to clear cache:', error) + logger.error('Failed to clear cache:', { error }) @@ - logger.error(`Failed to calculate cache size for ${cachePath}: ${error.message}`) + logger.error('Failed to calculate cache size for path:', { cachePath, error }) @@ - logger.error('Failed to select app data path:', error) + logger.error('Failed to select app data path:', { error }) @@ - logger.error('Failed to copy user data:', error) + logger.error('Failed to copy user data:', { error })This pattern appears in other handlers too; consider a quick sweep.
Also applies to: 274-278, 331-333
381-385: Prefer ESM import over inline require for os.hostnameMinor cleanup: import hostname alongside arch to avoid require in ESM.
-import { arch } from 'os' +import { arch, hostname as osHostname } from 'os' @@ - ipcMain.handle(IpcChannel.System_GetHostname, () => require('os').hostname()) + ipcMain.handle(IpcChannel.System_GetHostname, () => osHostname())Also applies to: 15-16
src/main/services/FFmpegService.ts (3)
246-293: System FFmpeg detection is incorrect when only available via PATH
fastCheckFFmpegExistsusesfs.existsSynconffmpeg/ffmpeg.exe, which fails when FFmpeg is only on PATH (typical). This causes false negatives and cascades intocheckFFmpegExists()andautoDetectAndDownload()reportingneedsDownloadincorrectly.Apply a PATH-aware check: if
ffmpegPathisn’t absolute, probe withspawnSync(ffmpegPath, ['-version'])(status 0/1 indicates presence). Keep the existing file checks for absolute paths.-import { spawn } from 'child_process' +import { spawn, spawnSync } from 'child_process'public fastCheckFFmpegExists(): boolean { const startTime = Date.now() const ffmpegPath = this.getFFmpegPath() try { - // 检查文件是否存在 - if (!fs.existsSync(ffmpegPath)) { - logger.info('⚡ 快速检查: FFmpeg 文件不存在', { ffmpegPath }) - return false - } - - // 检查是否为文件(非目录) - const stats = fs.statSync(ffmpegPath) - if (!stats.isFile()) { - logger.info('⚡ 快速检查: FFmpeg 路径不是文件', { ffmpegPath }) - return false - } - - // 检查是否有执行权限 (Unix/Linux/macOS) - if (process.platform !== 'win32') { - const hasExecutePermission = (stats.mode & 0o111) !== 0 - if (!hasExecutePermission) { - logger.info('⚡ 快速检查: FFmpeg 没有执行权限', { - ffmpegPath, - mode: stats.mode.toString(8) - }) - return false - } - } + // 若为绝对路径或包含路径分隔符,执行文件系统级快速检查 + const looksLikePath = + path.isAbsolute(ffmpegPath) || ffmpegPath.includes(path.sep) || ffmpegPath.includes('/') + if (looksLikePath) { + if (!fs.existsSync(ffmpegPath)) { + logger.info('⚡ 快速检查: FFmpeg 文件不存在', { ffmpegPath }) + return false + } + const stats = fs.statSync(ffmpegPath) + if (!stats.isFile()) { + logger.info('⚡ 快速检查: FFmpeg 路径不是文件', { ffmpegPath }) + return false + } + if (process.platform !== 'win32') { + const hasExecutePermission = (stats.mode & 0o111) !== 0 + if (!hasExecutePermission) { + logger.info('⚡ 快速检查: FFmpeg 没有执行权限', { + ffmpegPath, + mode: stats.mode.toString(8) + }) + return false + } + } + } else { + // 否则,当作命令名处理:通过 PATH 进行探测 + const res = spawnSync(ffmpegPath, ['-version'], { stdio: 'ignore' }) + if (res.error) { + if ((res.error as any).code === 'ENOENT') { + logger.info('⚡ 快速检查: PATH 中未找到 FFmpeg', { ffmpegPath }) + return false + } + throw res.error + } + // status 0/1 都可认为可用(部分构建在无输出时返回 1) + if (res.status !== 0 && res.status !== 1) { + logger.info('⚡ 快速检查: FFmpeg 返回异常状态码', { + status: res.status + }) + return false + } + } const totalTime = Date.now() - startTime - logger.info(`⚡ 快速检查 FFmpeg 通过,耗时: ${totalTime}ms`, { + logger.info(`⚡ 快速检查 FFmpeg 通过,耗时: ${totalTime}ms`, { ffmpegPath, - 文件大小: `${Math.round((stats.size / 1024 / 1024) * 100) / 100}MB`, - 执行权限: process.platform !== 'win32' ? 'yes' : 'n/a' + 通过方式: looksLikePath ? 'filesystem' : 'PATH' }) return true
447-449: Fix bitrate conversion: string concatenation yields wrong value
bitrateMatch[1] + '000'concatenates strings. Convert kb/s to b/s numerically.- const bitrate = bitrateMatch ? bitrateMatch[1] + '000' : '0' // 转换为 bits/s + const bitrate = bitrateMatch ? String(parseInt(bitrateMatch[1], 10) * 1000) : '0' // kb/s → b/s
54-102: Reduce verbose info-level logging; avoid listing directory contents by defaultThe conversion helper logs directory listings and filename heuristics at info level, which is noisy and risks PII leakage. Gate this block behind a debug flag or lower the level.
Would you like a patch to wrap these logs with
if (process.env.DEBUG_FFMPEG_PATH === '1') { ... }?src/preload/index.ts (1)
571-574: Do not use console in TS/JS; route logs via logger pipelineReplace
console.errorwith the existing main-process logger IPC to comply with logging guidelines.- // eslint-disable-next-line no-restricted-syntax - console.error('[Preload]Failed to expose APIs:', error as Error) + // Route to main logger + void ipcRenderer.invoke( + IpcChannel.App_LogToMain, + { source: 'preload', context: 'expose' }, + LogLevel.ERROR, + '[Preload] Failed to expose APIs', + [{ error }] + )src/renderer/src/pages/player/engine/PlayerOrchestrator.ts (2)
440-448: Immediate store update without internal context update — align both and clamp once.Update internal context and store using the same clamped value before issuing seek; avoids one-tick inconsistencies during intent evaluation.
Apply this diff:
- // 立即更新 store 中的 currentTime,确保 UI 组件能立即响应 - if (this.stateUpdater) { - this.stateUpdater.setCurrentTime(to) - } - - // 执行跳转 - const clampedTime = Math.max(0, Math.min(this.context.duration || Infinity, to)) - this.videoController.seek(clampedTime) + // 计算并同步(内部 + 外部)后再执行跳转,避免一拍不一致 + const clampedTime = Math.max(0, Math.min(this.context.duration || Infinity, to)) + this.updateContext({ currentTime: clampedTime }) + this.stateUpdater?.setCurrentTime(clampedTime) + // 执行跳转 + this.videoController.seek(clampedTime)
485-513: Unlock timer tracking bug + unhandled import rejection in subtitle seek.
- You assign userSeekTaskId when scheduling unlock, but set it to null at Line 511 before the timer fires. Subsequent seeks cannot cancel the pending unlock, which can unlock a newer user lock unexpectedly.
- Same unhandled rejection for dynamic import as above.
Apply this diff:
this.userSeekTaskId = this.clockScheduler.scheduleAfter( 2000, // 2秒延迟 () => { this.subtitleLockFSM.unlock('user_seek') this.userSeekTaskId = null logger.debug('用户跳转锁定已自动解除') }, 'user_seek_unlock' ) - // 标记用户跳转状态,暂时禁用自动保存 - import('@renderer/services/PlayerSettingsSaver').then( - ({ playerSettingsPersistenceService }) => { - playerSettingsPersistenceService.markUserSeeking() - } - ) + // 标记用户跳转状态,暂时禁用自动保存 + import('@renderer/services/PlayerSettingsSaver') + .then(({ playerSettingsPersistenceService }) => { + playerSettingsPersistenceService.markUserSeeking() + }) + .catch((error) => { + logger.error('Failed to markUserSeeking()', { error }) + }) @@ - // 恢复正常状态 - this.userSeekTaskId = null + // 恢复正常状态(不要清空 userSeekTaskId;需保留以便后续 seek 能取消解锁任务)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (41)
electron-builder.yml(1 hunks)electron.vite.config.ts(0 hunks)package.json(2 hunks)packages/shared/IpcChannel.ts(1 hunks)src/main/__tests__/ipc.database.test.ts(1 hunks)src/main/ipc.ts(1 hunks)src/main/services/AppUpdater.ts(1 hunks)src/main/services/FFmpegDownloadService.ts(1 hunks)src/main/services/FFmpegService.ts(7 hunks)src/main/services/ThemeService.ts(2 hunks)src/main/services/WindowService.ts(2 hunks)src/main/services/__tests__/FFmpegDownloadService.test.ts(1 hunks)src/main/services/__tests__/FFmpegService.integration.test.ts(1 hunks)src/preload/index.ts(1 hunks)src/renderer/src/assets/styles/ant.scss(1 hunks)src/renderer/src/components/FFmpegDownloadPrompt.tsx(1 hunks)src/renderer/src/components/IndicatorLight.tsx(2 hunks)src/renderer/src/components/app/Navbar.tsx(1 hunks)src/renderer/src/hooks/useVideoFileSelect.ts(4 hunks)src/renderer/src/i18n/label.ts(0 hunks)src/renderer/src/i18n/locales/en-us.json(2 hunks)src/renderer/src/i18n/locales/zh-cn.json(6 hunks)src/renderer/src/infrastructure/constants/shortcuts.const.ts(0 hunks)src/renderer/src/pages/home/EmptyState.tsx(1 hunks)src/renderer/src/pages/home/HeaderNavbar.tsx(2 hunks)src/renderer/src/pages/home/HomePage.tsx(5 hunks)src/renderer/src/pages/home/VideoAddButton.tsx(1 hunks)src/renderer/src/pages/player/components/VideoErrorRecovery.tsx(1 hunks)src/renderer/src/pages/player/engine/MediaClock.ts(3 hunks)src/renderer/src/pages/player/engine/PlayerOrchestrator.ts(9 hunks)src/renderer/src/pages/player/engine/intent/IntentStrategyManager.ts(0 hunks)src/renderer/src/pages/player/hooks/usePlayerEngine.ts(1 hunks)src/renderer/src/pages/player/hooks/usePlayerShortcuts.ts(1 hunks)src/renderer/src/pages/player/hooks/useSubtitleEngine.ts(2 hunks)src/renderer/src/pages/player/hooks/useSubtitleOverlay.ts(2 hunks)src/renderer/src/pages/settings/FFmpegSettings.tsx(1 hunks)src/renderer/src/pages/settings/SettingsPage.tsx(3 hunks)src/renderer/src/services/Logger.ts(4 hunks)src/renderer/src/services/PlayerSettingsLoader.ts(2 hunks)src/renderer/src/services/PlayerSettingsSaver.ts(3 hunks)src/renderer/src/state/stores/player.store.ts(4 hunks)
💤 Files with no reviewable changes (4)
- src/renderer/src/pages/player/engine/intent/IntentStrategyManager.ts
- src/renderer/src/i18n/label.ts
- src/renderer/src/infrastructure/constants/shortcuts.const.ts
- electron.vite.config.ts
🧰 Additional context used
📓 Path-based instructions (9)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: 在组件/Hook 中避免硬编码尺寸和时长,优先使用 useTheme() 的 token(如 motionDurationMid、borderRadiusSM/MD 等)或集中定义的样式变量
定制 antd 组件样式时优先使用 styled-components(styled(Component) 包装),避免通过全局 SCSS 与全局 className 覆盖
项目使用 Zustand 结合 Immer 中间件与自定义中间件栈(持久化、DevTools、订阅选择器)进行状态管理
Files:
src/main/services/ThemeService.tssrc/main/services/__tests__/FFmpegService.integration.test.tssrc/renderer/src/services/PlayerSettingsSaver.tssrc/renderer/src/pages/player/engine/MediaClock.tssrc/renderer/src/pages/settings/FFmpegSettings.tsxsrc/renderer/src/pages/player/components/VideoErrorRecovery.tsxsrc/renderer/src/components/IndicatorLight.tsxsrc/renderer/src/components/FFmpegDownloadPrompt.tsxsrc/main/services/WindowService.tssrc/main/__tests__/ipc.database.test.tssrc/main/services/__tests__/FFmpegDownloadService.test.tssrc/renderer/src/pages/home/EmptyState.tsxsrc/renderer/src/components/app/Navbar.tsxpackages/shared/IpcChannel.tssrc/renderer/src/pages/player/hooks/usePlayerShortcuts.tssrc/renderer/src/services/PlayerSettingsLoader.tssrc/renderer/src/pages/player/hooks/useSubtitleEngine.tssrc/main/services/AppUpdater.tssrc/renderer/src/state/stores/player.store.tssrc/main/services/FFmpegDownloadService.tssrc/renderer/src/pages/player/engine/PlayerOrchestrator.tssrc/renderer/src/pages/home/HeaderNavbar.tsxsrc/renderer/src/pages/home/VideoAddButton.tsxsrc/renderer/src/pages/player/hooks/usePlayerEngine.tssrc/renderer/src/hooks/useVideoFileSelect.tssrc/renderer/src/pages/player/hooks/useSubtitleOverlay.tssrc/main/ipc.tssrc/preload/index.tssrc/renderer/src/pages/home/HomePage.tsxsrc/renderer/src/pages/settings/SettingsPage.tsxsrc/main/services/FFmpegService.tssrc/renderer/src/services/Logger.ts
**/*.{tsx,ts}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{tsx,ts}: Zustand 必须在组件/Hook 顶层使用 selector(useStore(selector));禁止在 useMemo/useEffect 内部调用 store Hook
避免使用返回对象的 useStore 选择器(如 useStore(s => ({ a: s.a, b: s.b }))),应使用单字段选择器或配合 shallow 比较器
Files:
src/main/services/ThemeService.tssrc/main/services/__tests__/FFmpegService.integration.test.tssrc/renderer/src/services/PlayerSettingsSaver.tssrc/renderer/src/pages/player/engine/MediaClock.tssrc/renderer/src/pages/settings/FFmpegSettings.tsxsrc/renderer/src/pages/player/components/VideoErrorRecovery.tsxsrc/renderer/src/components/IndicatorLight.tsxsrc/renderer/src/components/FFmpegDownloadPrompt.tsxsrc/main/services/WindowService.tssrc/main/__tests__/ipc.database.test.tssrc/main/services/__tests__/FFmpegDownloadService.test.tssrc/renderer/src/pages/home/EmptyState.tsxsrc/renderer/src/components/app/Navbar.tsxpackages/shared/IpcChannel.tssrc/renderer/src/pages/player/hooks/usePlayerShortcuts.tssrc/renderer/src/services/PlayerSettingsLoader.tssrc/renderer/src/pages/player/hooks/useSubtitleEngine.tssrc/main/services/AppUpdater.tssrc/renderer/src/state/stores/player.store.tssrc/main/services/FFmpegDownloadService.tssrc/renderer/src/pages/player/engine/PlayerOrchestrator.tssrc/renderer/src/pages/home/HeaderNavbar.tsxsrc/renderer/src/pages/home/VideoAddButton.tsxsrc/renderer/src/pages/player/hooks/usePlayerEngine.tssrc/renderer/src/hooks/useVideoFileSelect.tssrc/renderer/src/pages/player/hooks/useSubtitleOverlay.tssrc/main/ipc.tssrc/preload/index.tssrc/renderer/src/pages/home/HomePage.tsxsrc/renderer/src/pages/settings/SettingsPage.tsxsrc/main/services/FFmpegService.tssrc/renderer/src/services/Logger.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js,jsx}: 统一使用 loggerService 记录日志,禁止使用 console
logger 使用示例:logger.error('Error in :', { error }); 第二个参数必须是对象字面量 {}
Files:
src/main/services/ThemeService.tssrc/main/services/__tests__/FFmpegService.integration.test.tssrc/renderer/src/services/PlayerSettingsSaver.tssrc/renderer/src/pages/player/engine/MediaClock.tssrc/renderer/src/pages/settings/FFmpegSettings.tsxsrc/renderer/src/pages/player/components/VideoErrorRecovery.tsxsrc/renderer/src/components/IndicatorLight.tsxsrc/renderer/src/components/FFmpegDownloadPrompt.tsxsrc/main/services/WindowService.tssrc/main/__tests__/ipc.database.test.tssrc/main/services/__tests__/FFmpegDownloadService.test.tssrc/renderer/src/pages/home/EmptyState.tsxsrc/renderer/src/components/app/Navbar.tsxpackages/shared/IpcChannel.tssrc/renderer/src/pages/player/hooks/usePlayerShortcuts.tssrc/renderer/src/services/PlayerSettingsLoader.tssrc/renderer/src/pages/player/hooks/useSubtitleEngine.tssrc/main/services/AppUpdater.tssrc/renderer/src/state/stores/player.store.tssrc/main/services/FFmpegDownloadService.tssrc/renderer/src/pages/player/engine/PlayerOrchestrator.tssrc/renderer/src/pages/home/HeaderNavbar.tsxsrc/renderer/src/pages/home/VideoAddButton.tsxsrc/renderer/src/pages/player/hooks/usePlayerEngine.tssrc/renderer/src/hooks/useVideoFileSelect.tssrc/renderer/src/pages/player/hooks/useSubtitleOverlay.tssrc/main/ipc.tssrc/preload/index.tssrc/renderer/src/pages/home/HomePage.tsxsrc/renderer/src/pages/settings/SettingsPage.tsxsrc/main/services/FFmpegService.tssrc/renderer/src/services/Logger.ts
**/*.{test,spec}.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
测试使用 vitest 作为测试框架
Files:
src/main/services/__tests__/FFmpegService.integration.test.tssrc/main/__tests__/ipc.database.test.tssrc/main/services/__tests__/FFmpegDownloadService.test.ts
**/*.{tsx,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{tsx,jsx}: 图标统一使用 lucide-react,不使用 emoji 作为图标
优先使用 antd 组件库;如组件可被 antd 复用则优先用 antd 而非自研
Files:
src/renderer/src/pages/settings/FFmpegSettings.tsxsrc/renderer/src/pages/player/components/VideoErrorRecovery.tsxsrc/renderer/src/components/IndicatorLight.tsxsrc/renderer/src/components/FFmpegDownloadPrompt.tsxsrc/renderer/src/pages/home/EmptyState.tsxsrc/renderer/src/components/app/Navbar.tsxsrc/renderer/src/pages/home/HeaderNavbar.tsxsrc/renderer/src/pages/home/VideoAddButton.tsxsrc/renderer/src/pages/home/HomePage.tsxsrc/renderer/src/pages/settings/SettingsPage.tsx
**/*.{tsx,jsx,css,scss}
📄 CodeRabbit inference engine (CLAUDE.md)
布局实现优先使用 flex,避免将 grid 作为默认方案
Files:
src/renderer/src/pages/settings/FFmpegSettings.tsxsrc/renderer/src/pages/player/components/VideoErrorRecovery.tsxsrc/renderer/src/components/IndicatorLight.tsxsrc/renderer/src/components/FFmpegDownloadPrompt.tsxsrc/renderer/src/pages/home/EmptyState.tsxsrc/renderer/src/components/app/Navbar.tsxsrc/renderer/src/pages/home/HeaderNavbar.tsxsrc/renderer/src/pages/home/VideoAddButton.tsxsrc/renderer/src/assets/styles/ant.scsssrc/renderer/src/pages/home/HomePage.tsxsrc/renderer/src/pages/settings/SettingsPage.tsx
src/**/{stores,infrastructure,persistence}/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
状态管理分为 stores / infrastructure / persistence 三层结构,文件应按层次归档
Files:
src/renderer/src/state/stores/player.store.ts
src/renderer/src/assets/styles/**/*.{css,scss}
📄 CodeRabbit inference engine (CLAUDE.md)
src/renderer/src/assets/styles/**/*.{css,scss}: 优先使用 CSS 变量,而不是硬编码样式值(在样式文件中)
CSS/SCSS 样式文件应位于 src/renderer/src/assets/styles 目录下进行集中管理
Files:
src/renderer/src/assets/styles/ant.scss
src/renderer/src/assets/styles/**/*.scss
📄 CodeRabbit inference engine (CLAUDE.md)
src/renderer/src/assets/styles/**/*.scss: assets/styles 目录下的 SCSS(含 ant.scss)为全局引入,修改将全局生效,谨慎引入全局选择器与覆盖
禁止使用全局 SCSS 覆盖 antd 组件样式,避免样式污染与架构不一致
Files:
src/renderer/src/assets/styles/ant.scss
🧠 Learnings (6)
📚 Learning: 2025-09-07T16:43:49.564Z
Learnt from: CR
PR: mkdir700/EchoPlayer#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-07T16:43:49.564Z
Learning: Applies to **/*.{test,spec}.{ts,tsx,js,jsx} : 测试使用 vitest 作为测试框架
Applied to files:
src/main/services/__tests__/FFmpegService.integration.test.tssrc/main/services/__tests__/FFmpegDownloadService.test.ts
📚 Learning: 2025-09-07T16:43:49.564Z
Learnt from: CR
PR: mkdir700/EchoPlayer#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-07T16:43:49.564Z
Learning: 任何组件或页面都不要支持写入 currentTime,播放器控制应由编排器统一管理
Applied to files:
src/renderer/src/pages/player/engine/PlayerOrchestrator.ts
📚 Learning: 2025-09-07T16:43:49.564Z
Learnt from: CR
PR: mkdir700/EchoPlayer#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-07T16:43:49.564Z
Learning: Applies to src/renderer/src/assets/styles/**/*.scss : assets/styles 目录下的 SCSS(含 ant.scss)为全局引入,修改将全局生效,谨慎引入全局选择器与覆盖
Applied to files:
src/renderer/src/assets/styles/ant.scss
📚 Learning: 2025-09-07T16:43:49.564Z
Learnt from: CR
PR: mkdir700/EchoPlayer#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-07T16:43:49.564Z
Learning: Applies to src/renderer/src/assets/styles/**/*.scss : 禁止使用全局 SCSS 覆盖 antd 组件样式,避免样式污染与架构不一致
Applied to files:
src/renderer/src/assets/styles/ant.scss
📚 Learning: 2025-09-07T16:43:49.564Z
Learnt from: CR
PR: mkdir700/EchoPlayer#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-07T16:43:49.564Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : logger 使用示例:logger.error('Error in <context>:', { error }); 第二个参数必须是对象字面量 {}
Applied to files:
src/renderer/src/services/Logger.ts
📚 Learning: 2025-09-07T16:43:49.564Z
Learnt from: CR
PR: mkdir700/EchoPlayer#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-07T16:43:49.564Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : 统一使用 loggerService 记录日志,禁止使用 console
Applied to files:
src/renderer/src/services/Logger.ts
🧬 Code graph analysis (16)
src/main/services/ThemeService.ts (1)
src/main/constant.ts (1)
isMac(1-1)
src/renderer/src/services/PlayerSettingsSaver.ts (2)
src/renderer/src/state/stores/player.store.ts (1)
usePlayerStore(477-479)src/renderer/src/services/Logger.ts (1)
error(422-424)
src/renderer/src/pages/settings/FFmpegSettings.tsx (4)
src/renderer/src/services/Logger.ts (1)
loggerService(817-817)src/renderer/src/contexts/theme.context.tsx (1)
useTheme(124-124)src/renderer/src/pages/settings/AboutSettings.tsx (1)
SettingRowTitle(412-424)src/renderer/src/infrastructure/styles/theme.ts (6)
SPACING(43-58)BORDER_RADIUS(61-72)ANIMATION_DURATION(91-100)EASING(103-114)FONT_WEIGHTS(11-22)FONT_SIZES(25-40)
src/renderer/src/components/FFmpegDownloadPrompt.tsx (1)
src/renderer/src/infrastructure/styles/theme.ts (4)
SPACING(43-58)BORDER_RADIUS(61-72)FONT_SIZES(25-40)FONT_WEIGHTS(11-22)
src/main/services/WindowService.ts (1)
src/main/constant.ts (2)
isWin(2-2)isLinux(3-3)
src/main/services/__tests__/FFmpegDownloadService.test.ts (1)
src/main/services/FFmpegDownloadService.ts (1)
FFmpegDownloadService(120-588)
src/renderer/src/pages/home/EmptyState.tsx (1)
src/renderer/src/hooks/useVideoFileSelect.ts (1)
useVideoFileSelect(40-230)
src/renderer/src/pages/player/hooks/usePlayerShortcuts.ts (2)
src/renderer/src/pages/player/hooks/useSubtitleOverlay.ts (1)
useSubtitleOverlay(35-189)src/renderer/src/pages/player/hooks/index.ts (1)
useSubtitleOverlay(3-3)
src/renderer/src/pages/player/hooks/useSubtitleEngine.ts (1)
src/renderer/src/state/stores/player.store.ts (1)
usePlayerStore(477-479)
src/main/services/FFmpegDownloadService.ts (1)
src/renderer/src/services/Logger.ts (2)
loggerService(817-817)error(422-424)
src/renderer/src/pages/player/engine/PlayerOrchestrator.ts (1)
src/renderer/src/services/PlayerSettingsSaver.ts (1)
playerSettingsPersistenceService(184-184)
src/renderer/src/pages/home/VideoAddButton.tsx (1)
src/renderer/src/hooks/useVideoFileSelect.ts (1)
useVideoFileSelect(40-230)
src/renderer/src/pages/player/hooks/usePlayerEngine.ts (2)
src/renderer/src/state/stores/player.store.ts (1)
usePlayerStore(477-479)src/renderer/src/state/index.ts (1)
usePlayerStore(1-1)
src/main/ipc.ts (1)
src/main/services/FFmpegDownloadService.ts (1)
ffmpegDownloadService(591-591)
src/renderer/src/pages/home/HomePage.tsx (2)
src/renderer/src/pages/home/EmptyState.tsx (1)
EmptyState(15-71)src/renderer/src/components/FFmpegDownloadPrompt.tsx (1)
FFmpegDownloadPrompt(23-123)
src/main/services/FFmpegService.ts (1)
src/main/services/FFmpegDownloadService.ts (1)
ffmpegDownloadService(591-591)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: test (macos-latest, 20)
- GitHub Check: test (ubuntu-latest, 20)
- GitHub Check: test (windows-latest, 20)
🔇 Additional comments (39)
src/main/services/AppUpdater.ts (2)
289-293: Type-safety of app.isQuitting.Ensure you have an ambient type augmentation; otherwise TS will flag
isQuittingas unknown.Add a declaration file (e.g.,
src/types/electron.d.ts):declare global { namespace Electron { interface App { isQuitting?: boolean } } } export {}
255-258: Verify downstream reliance on injected releaseNotes.You now direct users to Settings > About. Confirm the renderer still reads
updateInfo.releaseNotesfromcheckForUpdates()to render that page; otherwise this fallback assignment is dead code.src/renderer/src/components/IndicatorLight.tsx (1)
2-2: LGTM: css helper import aligns with our styled-components usage.
No issues; consistent with renderer styling.src/main/services/WindowService.ts (1)
91-91: LGTM: platform-aware title bar options are cleanly injectedUsing
...getTitleBarConfig()here keeps BrowserWindow init concise and OS-specific.src/main/services/ThemeService.ts (2)
6-6: LGTM: platform guard importedImporting
isMacaligns this service with the platform-aware window config.
30-31: LGTM: apply titleBarOverlay only on macOSThis prevents unintended overlay behavior on Windows/Linux and matches the new window config.
src/renderer/src/pages/player/components/VideoErrorRecovery.tsx (1)
169-169: Verify Ant Design version (needs >= 5.27.0 for destroyOnHidden)destroyOnHidden is supported on ModalProps starting in antd v5.27.0. Confirm your repo/workspace uses antd >= 5.27.0 (check package.json files); if not, remove/guard this prop or upgrade antd. Location: src/renderer/src/pages/player/components/VideoErrorRecovery.tsx (around line 169).
electron-builder.yml (1)
41-45: Files filter may accidentally drop DB migration assets — verification blocked by failed buildTypecheck aborted the build and no unpacked output was produced (errors: "Cannot find type definition file for 'electron-vite/node'"; missing '@electron-toolkit/tsconfig/tsconfig.node.json'; 'bundler' requires module ≥ es2015/preserve). Cannot confirm migrations are packaged.
Actions:
- Fix the TS/tsconfig errors or provide a previously-built unpacked output.
- After build succeeds, run verification (adjust OUT if different):
npm run build:unpack OUT=out fd -t f -E node_modules -E .asar 'migrations' "$OUT" | sed -n '1,200p' rg -nP --glob '!**/node_modules/**' -C1 'CREATE TABLE|kysely|migrate' "$OUT" | sed -n '1,200p'- If compiled JS migrations are missing, explicitly include them in electron-builder/electron.vite copy rules (e.g., add an include for db/**/*.js or the specific migration output path).
src/renderer/src/components/app/Navbar.tsx (1)
106-116: Remove unused $isFullscreen prop & use design token for paddingNavbarMainContainer declares/accepts $isFullscreen but never references it; stop passing the prop and replace the hard-coded padding-right with a CSS token fallback. Remove the now-unused useFullscreen() call in NavbarMain.
Apply this diff (unchanged from suggestion):
-const NavbarMainContainer = styled.div<{ $isFullscreen: boolean }>` +const NavbarMainContainer = styled.div` flex: 1; display: flex; flex-direction: row; align-items: center; justify-content: space-between; padding: 0 ${isMac ? '20px' : 0}; font-weight: bold; color: var(--color-text-1); - padding-right: 12px; + /* Prefer design token or CSS var; fall back to 12px */ + padding-right: var(--space-sm, 12px); `Update usage:
// Before <NavbarMainContainer {...props} $isFullscreen={isFullscreen}> // After <NavbarMainContainer {...props}>Manually verify no visual regressions on Windows/Linux titlebar overlay when maximized/fullscreen.
src/renderer/src/services/PlayerSettingsSaver.ts (2)
87-98: Timer cleanup on detach is correctAll timers involved in seeking and debounced saves are cleared. This avoids orphaned writes after navigation.
165-171: Skip-save during seeking is correct; guard is in the right placeThis prevents churn and DB write spikes while the user is scrubbing.
packages/shared/IpcChannel.ts (1)
80-92: FFmpeg IPC additions: unable to verify main/preload wiring — manual check requiredrg search against src/main and src/preload returned no matches; I could not confirm handlers are registered or exposed.
File: packages/shared/IpcChannel.ts (lines 80–92):
Ffmpeg_GetInfo = 'ffmpeg:get-info', Ffmpeg_AutoDetectAndDownload = 'ffmpeg:auto-detect-and-download', // FFmpeg 下载相关 IPC 通道 / FFmpeg download related IPC channels FfmpegDownload_CheckExists = 'ffmpeg-download:check-exists', FfmpegDownload_GetVersion = 'ffmpeg-download:get-version', FfmpegDownload_Download = 'ffmpeg-download:download', FfmpegDownload_GetProgress = 'ffmpeg-download:get-progress', FfmpegDownload_Cancel = 'ffmpeg-download:cancel', FfmpegDownload_Remove = 'ffmpeg-download:remove', FfmpegDownload_GetAllVersions = 'ffmpeg-download:get-all-versions', FfmpegDownload_CleanupTemp = 'ffmpeg-download:cleanup-temp',Ensure the main process registers handlers for each channel above and preload exposes them to the renderer. To re-run an automatic check across the repo use:
rg -nP "ffmpeg:(get-info|auto-detect-and-download)|ffmpeg-download:(check-exists|get-version|download|get-progress|cancel|remove|get-all-versions|cleanup-temp)" -C2src/renderer/src/services/Logger.ts (2)
141-218: Lightweight serializer is a solid improvement.Depth limiting, array/key caps, and error/function handling reduce memory pressure while keeping useful context.
619-621: Console in logger internals.Given repo guidelines forbid console, it’s acceptable here only as logger’s internal sink. Ensure any console calls remain confined to this file.
src/renderer/src/i18n/locales/en-us.json (1)
202-203: LGTM: adds missing labels for cycling favorite ratesLabels read well and match feature semantics.
src/renderer/src/pages/settings/SettingsPage.tsx (1)
2-2: Route + import wiring for FFmpeg settings: OKImport, nav, and route mapping are consistent with i18n key settings.plugins.title.
Please verify zh-CN and any other locales contain settings.plugins.title to avoid runtime “missing key” warnings.
Also applies to: 10-10, 74-74
src/renderer/src/pages/player/hooks/usePlayerEngine.ts (1)
58-61: StateUpdater now syncs active subtitle index — good integrationMatches store API; logging at debug level is appropriate.
src/renderer/src/pages/player/hooks/useSubtitleEngine.ts (1)
22-22: Prioritize orchestrator-driven activeCueIndex with safe fallback — solidSelector usage follows Zustand guidelines; bounds check avoids OOB access.
Also applies to: 82-91
src/renderer/src/services/PlayerSettingsLoader.ts (1)
45-45: Do not persist runtime-only activeCueIndex — correctInitialized to -1 in defaults and DB mapping; excluded from persistence.
Also applies to: 183-183
src/renderer/src/pages/home/HeaderNavbar.tsx (1)
14-15: Prop-drilling onShowFFmpegPrompt: OKOptional callback is correctly typed and passed through to VideoAddButton.
Also applies to: 17-21, 36-36
src/renderer/src/state/stores/player.store.ts (1)
43-44: Active cue index state + setter: OKRuntime-only field and action align with the new engine flow.
Also applies to: 104-105, 155-156, 224-224
src/renderer/src/pages/player/hooks/usePlayerShortcuts.ts (1)
35-35: Refactor aligns with removal of display-mode shortcutsUsing only currentSubtitle here is correct post-removal.
src/renderer/src/pages/home/EmptyState.tsx (1)
25-29: Propagating FFmpeg prompt state upward — LGTMForwarding showFFmpegPrompt via onShowFFmpegPrompt keeps page-level state authoritative. No issues.
src/renderer/src/pages/home/HomePage.tsx (3)
135-141: Centralized prompt visibility handlers — LGTMClear, memoized handlers; fits the new prompt flow.
191-201: Prop wiring — LGTMHeaderNavbar and EmptyState receive the prompt toggler correctly.
288-289: Prompt mounting location — LGTMRendering FFmpegDownloadPrompt at the page root avoids z-index/container issues with Modals.
src/renderer/src/pages/home/VideoAddButton.tsx (1)
21-25: Propagating FFmpeg prompt state upward — LGTMEffect correctly reports prompt visibility to parent.
src/renderer/src/components/FFmpegDownloadPrompt.tsx (1)
37-46: Modal config — LGTMGood defaults (centered, maskClosable=false), null title suits custom header.
src/renderer/src/pages/player/hooks/useSubtitleOverlay.ts (2)
58-76: Memoized, stable subtitle object — LGTMGood approach to reduce unnecessary re-renders; dependencies are granular.
110-129: Display text derivation — LGTMClear precedence and fallback behavior; no unnecessary recomputations.
src/main/services/__tests__/FFmpegDownloadService.test.ts (1)
50-69: LGTM: platform-specific path resolution tests are preciseGood coverage for win/darwin/linux and unsupported platform error cases.
src/main/services/FFmpegService.ts (1)
531-595: ENOENT handling and contextual logging: solidGood UX-focused messaging and structured logs when FFmpeg is missing.
src/preload/index.ts (1)
185-201: FFmpeg preload surface: comprehensive and consistentAPI shape mirrors main IPC channels and keeps renderer sandboxed. LGTM.
src/renderer/src/pages/settings/FFmpegSettings.tsx (1)
118-171: Polling lifecycle logic: solidProgress polling is properly cleaned up and warmup is auto-triggered on completion. Nice.
src/renderer/src/pages/player/engine/PlayerOrchestrator.ts (5)
59-61: Add optional setActiveCueIndex to StateUpdater — good expansion, backward compatible.This cleanly exposes cue-index sync to the renderer without breaking old updaters.
230-234: Propagate activeCueIndex changes via updater — LGTM.Keeps store in lockstep with internal context.
624-626: Remove high-frequency silly logs — good for memory/GC.
641-644: Start seeking on onSeeking — correct signaling to MediaClock and UI.
734-735: Log message clarity — LGTM.
| - arm64 | ||
| - target: portable | ||
| arch: | ||
| - x64 | ||
| - arm64 | ||
| signtoolOptions: |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Win ARM64 target added — verify code signing and auto-update paths
Adding NSIS/portable arm64 is good, but please confirm:
- The sign script (scripts/win-sign.js) supports arm64 artifacts.
- electron-updater serves delta/full updates per-arch and does not cross-serve x64 to arm64.
- The NSIS include (build/nsis-installer.nsh) has no arch assumptions breaking arm64.
Run locally on a Windows ARM64 VM/device to validate install/update flows. If not feasible, at least confirm release assets include separate arm64 .yml and artifacts and that updater feed lists the correct arch.
🤖 Prompt for AI Agents
In electron-builder.yml around lines 58-63, adding arm64 portable/NSIS targets
requires verification that signing, updater feeds, and installer scripts handle
arm64: update and test scripts/win-sign.js to detect and sign arm64 artifacts
(add arch checks and appropriate signtool args), ensure build output produces
separate x64 and arm64 .yml and artifact files and that the electron-updater
feed/metadata serves per-arch delta/full updates (no cross-serving), and review
build/nsis-installer.nsh for any x64-specific paths or assumptions and make them
arch-conditional; finally validate end-to-end by running install/update on a
Windows ARM64 VM/device or, if not possible, confirm release assets include
distinct arm64 .yml and artifacts and that the updater feed lists the correct
arch entries.
| "release": "npm run build:release && electron-builder --publish onTagOrDraft", | ||
| "release:all": "npm run build:release && electron-builder --publish always", | ||
| "release:never": "npm run build:release && electron-builder --publish never", | ||
| "release:draft": "npm run build:release && electron-builder --publish onTagOrDraft", | ||
| "migrate": "tsx src/main/db/migration-cli.ts", |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Release scripts simplified — ensure build commands are consistent across OS targets
Good removal of FFmpeg pre-steps. For consistency and type safety, consider aligning platform builds to run the same typecheck + build sequence used by build:release.
-"build:mac": "electron-vite build && electron-builder --mac --publish never",
+"build:mac": "npm run build && electron-builder --mac --publish never",
-"build:linux": "electron-vite build && electron-builder --linux --publish never",
+"build:linux": "npm run build && electron-builder --linux --publish never",Confirm CI still publishes drafts correctly with --publish onTagOrDraft after this change and that no FFmpeg binaries are bundled in artifacts (by inspecting release assets).
Committable suggestion skipped: line range outside the PR's diff.
| getFFmpegPath: vi.fn(), | ||
| getDownloadService: vi.fn(() => ({ | ||
| checkFFmpegExists: vi.fn(), | ||
| getFFmpegVersion: vi.fn(), | ||
| downloadFFmpeg: vi.fn(), | ||
| getDownloadProgress: vi.fn(), | ||
| cancelDownload: vi.fn(), | ||
| removeFFmpeg: vi.fn(), | ||
| getAllSupportedVersions: vi.fn(), | ||
| cleanupTempFiles: vi.fn() | ||
| })) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Mock surface alignment for new FFmpeg download API
The nested getDownloadService mock mirrors production API well. To reduce drift, strongly type this mock against the service return type so CI breaks if the surface changes.
Example:
-vi.mock('../services/FFmpegService', () => ({
- default: vi.fn().mockImplementation(() => ({
+vi.mock('../services/FFmpegService', () => {
+ type DownloadSvc = ReturnType<import('../services/FFmpegService').default['getDownloadService']>;
+ const downloadSvc: DownloadSvc = {
+ checkFFmpegExists: vi.fn(),
+ getFFmpegVersion: vi.fn(),
+ downloadFFmpeg: vi.fn(),
+ getDownloadProgress: vi.fn(),
+ cancelDownload: vi.fn(),
+ removeFFmpeg: vi.fn(),
+ getAllSupportedVersions: vi.fn(),
+ cleanupTempFiles: vi.fn()
+ };
+ return {
+ default: vi.fn().mockImplementation(() => ({
checkFFmpegExists: vi.fn(),
getFFmpegVersion: vi.fn(),
downloadFFmpeg: vi.fn(),
getVideoInfo: vi.fn(),
transcodeVideo: vi.fn(),
cancelTranscode: vi.fn(),
getFFmpegPath: vi.fn(),
- getDownloadService: vi.fn(() => ({
- checkFFmpegExists: vi.fn(),
- getFFmpegVersion: vi.fn(),
- downloadFFmpeg: vi.fn(),
- getDownloadProgress: vi.fn(),
- cancelDownload: vi.fn(),
- removeFFmpeg: vi.fn(),
- getAllSupportedVersions: vi.fn(),
- cleanupTempFiles: vi.fn()
- }))
- }))
- }))
-}))
+ getDownloadService: vi.fn(() => downloadSvc)
+ }))
+ };
+})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| getFFmpegPath: vi.fn(), | |
| getDownloadService: vi.fn(() => ({ | |
| checkFFmpegExists: vi.fn(), | |
| getFFmpegVersion: vi.fn(), | |
| downloadFFmpeg: vi.fn(), | |
| getDownloadProgress: vi.fn(), | |
| cancelDownload: vi.fn(), | |
| removeFFmpeg: vi.fn(), | |
| getAllSupportedVersions: vi.fn(), | |
| cleanupTempFiles: vi.fn() | |
| })) | |
| vi.mock('../services/FFmpegService', () => { | |
| type DownloadSvc = ReturnType<import('../services/FFmpegService').default['getDownloadService']>; | |
| const downloadSvc: DownloadSvc = { | |
| checkFFmpegExists: vi.fn(), | |
| getFFmpegVersion: vi.fn(), | |
| downloadFFmpeg: vi.fn(), | |
| getDownloadProgress: vi.fn(), | |
| cancelDownload: vi.fn(), | |
| removeFFmpeg: vi.fn(), | |
| getAllSupportedVersions: vi.fn(), | |
| cleanupTempFiles: vi.fn() | |
| }; | |
| return { | |
| default: vi.fn().mockImplementation(() => ({ | |
| checkFFmpegExists: vi.fn(), | |
| getFFmpegVersion: vi.fn(), | |
| downloadFFmpeg: vi.fn(), | |
| getVideoInfo: vi.fn(), | |
| transcodeVideo: vi.fn(), | |
| cancelTranscode: vi.fn(), | |
| getFFmpegPath: vi.fn(), | |
| getDownloadService: vi.fn(() => downloadSvc) | |
| })) | |
| }; | |
| }); |
🤖 Prompt for AI Agents
In src/main/__tests__/ipc.database.test.ts around lines 169-179, the nested
getDownloadService mock isn't strongly typed to the real service interface so
tests won't catch API changes; update the mock to be typed to the download
service return type (import the service type or use ReturnType<typeof
getDownloadServiceFactory>) and declare the mock as returning that type so
TypeScript enforces the mock surface, then ensure each mocked method matches the
service signature (use vi.fn() for each method) and update any imports as
needed.
| ipcMain.handle(IpcChannel.Ffmpeg_GetInfo, async () => { | ||
| return ffmpegService.getFFmpegInfo() | ||
| }) | ||
| ipcMain.handle(IpcChannel.Ffmpeg_AutoDetectAndDownload, async () => { | ||
| return await ffmpegService.autoDetectAndDownload() | ||
| }) | ||
|
|
||
| // FFmpeg 下载服务 | ||
| const ffmpegDownloadService = ffmpegService.getDownloadService() | ||
| ipcMain.handle( | ||
| IpcChannel.FfmpegDownload_CheckExists, | ||
| async (_, platform?: string, arch?: string) => { | ||
| return ffmpegDownloadService.checkFFmpegExists(platform as any, arch as any) | ||
| } | ||
| ) | ||
| ipcMain.handle( | ||
| IpcChannel.FfmpegDownload_GetVersion, | ||
| async (_, platform?: string, arch?: string) => { | ||
| return ffmpegDownloadService.getFFmpegVersion(platform as any, arch as any) | ||
| } | ||
| ) | ||
| ipcMain.handle( | ||
| IpcChannel.FfmpegDownload_Download, | ||
| async (_, platform?: string, arch?: string) => { | ||
| return await ffmpegDownloadService.downloadFFmpeg(platform as any, arch as any) | ||
| } | ||
| ) | ||
| ipcMain.handle( | ||
| IpcChannel.FfmpegDownload_GetProgress, | ||
| async (_, platform?: string, arch?: string) => { | ||
| return ffmpegDownloadService.getDownloadProgress(platform as any, arch as any) | ||
| } | ||
| ) | ||
| ipcMain.handle(IpcChannel.FfmpegDownload_Cancel, async (_, platform?: string, arch?: string) => { | ||
| return ffmpegDownloadService.cancelDownload(platform as any, arch as any) | ||
| }) | ||
| ipcMain.handle(IpcChannel.FfmpegDownload_Remove, async (_, platform?: string, arch?: string) => { | ||
| return ffmpegDownloadService.removeFFmpeg(platform as any, arch as any) | ||
| }) | ||
| ipcMain.handle(IpcChannel.FfmpegDownload_GetAllVersions, async () => { | ||
| return ffmpegDownloadService.getAllSupportedVersions() | ||
| }) | ||
| ipcMain.handle(IpcChannel.FfmpegDownload_CleanupTemp, async () => { | ||
| return ffmpegDownloadService.cleanupTempFiles() | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Type the platform/arch params; avoid any-casts
Use Node’s platform/arch types to prevent invalid inputs and remove as any.
- const ffmpegDownloadService = ffmpegService.getDownloadService()
+ const ffmpegDownloadService = ffmpegService.getDownloadService()
@@
- async (_, platform?: string, arch?: string) => {
- return ffmpegDownloadService.checkFFmpegExists(platform as any, arch as any)
+ async (_, platform?: typeof process.platform, arch?: typeof process.arch) => {
+ return ffmpegDownloadService.checkFFmpegExists(platform, arch)
}
)
@@
- async (_, platform?: string, arch?: string) => {
- return ffmpegDownloadService.getFFmpegVersion(platform as any, arch as any)
+ async (_, platform?: typeof process.platform, arch?: typeof process.arch) => {
+ return ffmpegDownloadService.getFFmpegVersion(platform, arch)
}
)
@@
- async (_, platform?: string, arch?: string) => {
- return await ffmpegDownloadService.downloadFFmpeg(platform as any, arch as any)
+ async (_, platform?: typeof process.platform, arch?: typeof process.arch) => {
+ return await ffmpegDownloadService.downloadFFmpeg(platform, arch)
}
)
@@
- async (_, platform?: string, arch?: string) => {
- return ffmpegDownloadService.getDownloadProgress(platform as any, arch as any)
+ async (_, platform?: typeof process.platform, arch?: typeof process.arch) => {
+ return ffmpegDownloadService.getDownloadProgress(platform, arch)
}
)
- ipcMain.handle(IpcChannel.FfmpegDownload_Cancel, async (_, platform?: string, arch?: string) => {
- return ffmpegDownloadService.cancelDownload(platform as any, arch as any)
+ ipcMain.handle(IpcChannel.FfmpegDownload_Cancel, async (_, platform?: typeof process.platform, arch?: typeof process.arch) => {
+ return ffmpegDownloadService.cancelDownload(platform, arch)
})
- ipcMain.handle(IpcChannel.FfmpegDownload_Remove, async (_, platform?: string, arch?: string) => {
- return ffmpegDownloadService.removeFFmpeg(platform as any, arch as any)
+ ipcMain.handle(IpcChannel.FfmpegDownload_Remove, async (_, platform?: typeof process.platform, arch?: typeof process.arch) => {
+ return ffmpegDownloadService.removeFFmpeg(platform, arch)
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ipcMain.handle(IpcChannel.Ffmpeg_GetInfo, async () => { | |
| return ffmpegService.getFFmpegInfo() | |
| }) | |
| ipcMain.handle(IpcChannel.Ffmpeg_AutoDetectAndDownload, async () => { | |
| return await ffmpegService.autoDetectAndDownload() | |
| }) | |
| // FFmpeg 下载服务 | |
| const ffmpegDownloadService = ffmpegService.getDownloadService() | |
| ipcMain.handle( | |
| IpcChannel.FfmpegDownload_CheckExists, | |
| async (_, platform?: string, arch?: string) => { | |
| return ffmpegDownloadService.checkFFmpegExists(platform as any, arch as any) | |
| } | |
| ) | |
| ipcMain.handle( | |
| IpcChannel.FfmpegDownload_GetVersion, | |
| async (_, platform?: string, arch?: string) => { | |
| return ffmpegDownloadService.getFFmpegVersion(platform as any, arch as any) | |
| } | |
| ) | |
| ipcMain.handle( | |
| IpcChannel.FfmpegDownload_Download, | |
| async (_, platform?: string, arch?: string) => { | |
| return await ffmpegDownloadService.downloadFFmpeg(platform as any, arch as any) | |
| } | |
| ) | |
| ipcMain.handle( | |
| IpcChannel.FfmpegDownload_GetProgress, | |
| async (_, platform?: string, arch?: string) => { | |
| return ffmpegDownloadService.getDownloadProgress(platform as any, arch as any) | |
| } | |
| ) | |
| ipcMain.handle(IpcChannel.FfmpegDownload_Cancel, async (_, platform?: string, arch?: string) => { | |
| return ffmpegDownloadService.cancelDownload(platform as any, arch as any) | |
| }) | |
| ipcMain.handle(IpcChannel.FfmpegDownload_Remove, async (_, platform?: string, arch?: string) => { | |
| return ffmpegDownloadService.removeFFmpeg(platform as any, arch as any) | |
| }) | |
| ipcMain.handle(IpcChannel.FfmpegDownload_GetAllVersions, async () => { | |
| return ffmpegDownloadService.getAllSupportedVersions() | |
| }) | |
| ipcMain.handle(IpcChannel.FfmpegDownload_CleanupTemp, async () => { | |
| return ffmpegDownloadService.cleanupTempFiles() | |
| }) | |
| ipcMain.handle(IpcChannel.Ffmpeg_GetInfo, async () => { | |
| return ffmpegService.getFFmpegInfo() | |
| }) | |
| ipcMain.handle(IpcChannel.Ffmpeg_AutoDetectAndDownload, async () => { | |
| return await ffmpegService.autoDetectAndDownload() | |
| }) | |
| // FFmpeg 下载服务 | |
| const ffmpegDownloadService = ffmpegService.getDownloadService() | |
| ipcMain.handle( | |
| IpcChannel.FfmpegDownload_CheckExists, | |
| async (_, platform?: typeof process.platform, arch?: typeof process.arch) => { | |
| return ffmpegDownloadService.checkFFmpegExists(platform, arch) | |
| } | |
| ) | |
| ipcMain.handle( | |
| IpcChannel.FfmpegDownload_GetVersion, | |
| async (_, platform?: typeof process.platform, arch?: typeof process.arch) => { | |
| return ffmpegDownloadService.getFFmpegVersion(platform, arch) | |
| } | |
| ) | |
| ipcMain.handle( | |
| IpcChannel.FfmpegDownload_Download, | |
| async (_, platform?: typeof process.platform, arch?: typeof process.arch) => { | |
| return await ffmpegDownloadService.downloadFFmpeg(platform, arch) | |
| } | |
| ) | |
| ipcMain.handle( | |
| IpcChannel.FfmpegDownload_GetProgress, | |
| async (_, platform?: typeof process.platform, arch?: typeof process.arch) => { | |
| return ffmpegDownloadService.getDownloadProgress(platform, arch) | |
| } | |
| ) | |
| ipcMain.handle(IpcChannel.FfmpegDownload_Cancel, async (_, platform?: typeof process.platform, arch?: typeof process.arch) => { | |
| return ffmpegDownloadService.cancelDownload(platform, arch) | |
| }) | |
| ipcMain.handle(IpcChannel.FfmpegDownload_Remove, async (_, platform?: typeof process.platform, arch?: typeof process.arch) => { | |
| return ffmpegDownloadService.removeFFmpeg(platform, arch) | |
| }) | |
| ipcMain.handle(IpcChannel.FfmpegDownload_GetAllVersions, async () => { | |
| return ffmpegDownloadService.getAllSupportedVersions() | |
| }) | |
| ipcMain.handle(IpcChannel.FfmpegDownload_CleanupTemp, async () => { | |
| return ffmpegDownloadService.cleanupTempFiles() | |
| }) |
🤖 Prompt for AI Agents
In src/main/ipc.ts around lines 477-521, the IPC handlers accept platform and
arch as optional untyped params and then cast them with "as any"; change the
handler parameter types to use Node's platform/arch types (e.g. platform?:
NodeJS.Platform, arch?: NodeJS.Arch) and remove the "as any" casts when calling
ffmpegDownloadService methods; if necessary update the ffmpegDownloadService
method signatures to accept NodeJS.Platform and NodeJS.Arch so types align, and
ensure any ipcMain.handle generic type signatures reflect the correct
argument/return types to avoid implicit any.
| beforeEach(() => { | ||
| vi.clearAllMocks() | ||
|
|
||
| // Mock app.getPath | ||
| vi.mocked(app.getPath).mockReturnValue(mockUserDataPath) | ||
|
|
||
| // Mock fs.existsSync | ||
| vi.mocked(fs.existsSync).mockReturnValue(true) | ||
|
|
||
| // Mock fs.mkdirSync | ||
| vi.mocked(fs.mkdirSync).mockReturnValue(undefined) | ||
|
|
||
| // Mock path.join to return predictable paths | ||
| vi.mocked(path.join).mockImplementation((...args) => args.join('/')) | ||
|
|
||
| service = new FFmpegDownloadService() | ||
| }) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Prefer resetting mocks to avoid leakage across tests
Use vi.resetAllMocks() in beforeEach so per-test overrides (e.g., path.join stubs) don’t bleed into subsequent cases.
- vi.clearAllMocks()
+ vi.resetAllMocks()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| beforeEach(() => { | |
| vi.clearAllMocks() | |
| // Mock app.getPath | |
| vi.mocked(app.getPath).mockReturnValue(mockUserDataPath) | |
| // Mock fs.existsSync | |
| vi.mocked(fs.existsSync).mockReturnValue(true) | |
| // Mock fs.mkdirSync | |
| vi.mocked(fs.mkdirSync).mockReturnValue(undefined) | |
| // Mock path.join to return predictable paths | |
| vi.mocked(path.join).mockImplementation((...args) => args.join('/')) | |
| service = new FFmpegDownloadService() | |
| }) | |
| beforeEach(() => { | |
| vi.resetAllMocks() | |
| // Mock app.getPath | |
| vi.mocked(app.getPath).mockReturnValue(mockUserDataPath) | |
| // Mock fs.existsSync | |
| vi.mocked(fs.existsSync).mockReturnValue(true) | |
| // Mock fs.mkdirSync | |
| vi.mocked(fs.mkdirSync).mockReturnValue(undefined) | |
| // Mock path.join to return predictable paths | |
| vi.mocked(path.join).mockImplementation((...args) => args.join('/')) | |
| service = new FFmpegDownloadService() | |
| }) |
🤖 Prompt for AI Agents
In src/main/services/__tests__/FFmpegDownloadService.test.ts around lines 32 to
48, the beforeEach uses vi.clearAllMocks() which does not fully reset per-test
stubs so mocked implementations (like path.join) can leak; replace or supplement
vi.clearAllMocks() with vi.resetAllMocks() at the start of the beforeEach to
fully reset mocks and ensure each test gets fresh mock implementations, then
reapply any per-test mockReturnValue or mockImplementation calls after the
reset.
| </MenuItemLink> | ||
| <MenuItemLink to="/settings/plugins"> | ||
| <MenuItem className={isRoute('/settings/plugins')}> | ||
| <Monitor size={18} /> |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Avoid hardcoded icon sizes; use themed tokens
Follow repo guideline to use theme tokens over magic numbers.
Example (if tokens exist):
- <Monitor size={18} />
+ <Monitor size={theme.icons.md} />Or wrap with styled-components and size via CSS var.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/renderer/src/pages/settings/SettingsPage.tsx around line 57, the Monitor
icon is using a hardcoded size prop (size={18}); replace this magic number with
a themed token or CSS variable per repo guidelines—either read the theme token
(e.g., theme.tokens.icon.small or similar via useTheme/context) and pass that
value, or wrap the icon in a styled-component that sets width/height from a CSS
var (e.g., var(--icon-size-sm)) and reference the theme to set that var; update
imports if needed and remove the hardcoded size prop so the icon sizing follows
the theme token.
| if (!IS_DEV) { | ||
| return | ||
| } | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Production disables export history entirely—confirm product intent.
addToExportHistory returns early when !IS_DEV, making Settings > “export logs” effectively empty in production. If export is needed for user diagnostics, consider keeping a small, sampled ring buffer (e.g., 200 entries).
- if (!IS_DEV) {
- return
- }
+ if (!IS_DEV) {
+ // 生产环境保留小型环形缓冲区用于问题排查
+ this.maxHistorySize = Math.min(this.maxHistorySize, 200)
+ }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/renderer/src/services/Logger.ts around lines 507 to 521, the method
addToExportHistory currently returns immediately when !IS_DEV which makes
"export logs" empty in production; change this by replacing the hard early
return with a bounded, low-memory ring-buffer that is enabled in production via
a flag or config. Specifically: remove the unconditional return, introduce or
use an existing config flag (e.g., ENABLE_EXPORT_IN_PROD) to decide whether to
collect exports in production, implement a small fixed-size circular buffer
(recommend default 200 entries) that stores only minimal sanitized fields
(timestamp, level, message, small data summary, caller), overwrite oldest
entries when full, and keep all logic memory-safe and performant; ensure the
buffer is instantiated once at module init and that addToExportHistory pushes
entries into it only when the flag allows, preserving the original behavior in
dev.
| // 用户跳转时暂时禁用自动保存的标志位 | ||
| private isUserSeeking = false | ||
| private userSeekingTimer: NodeJS.Timeout | null = null | ||
| private currentVideoId: number | null = null | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
State flags for seeking look good; consider naming and doc clarity
Fields are clear and lifecycle-managed in detach(). Minor: rename currentVideoId to attachedVideoId to better convey ownership.
🤖 Prompt for AI Agents
In src/renderer/src/services/PlayerSettingsSaver.ts around lines 41 to 45,
rename the field currentVideoId to attachedVideoId to better express ownership;
update its type and initialization accordingly, replace all usages across the
class (constructors, attach/detach, timers, null checks) to use attachedVideoId,
ensure detach() clears attachedVideoId (sets to null) and any related
timers/flags, and run/adjust any imports/tests/types referencing the old name to
prevent compilation errors.
| /** | ||
| * 标记用户正在跳转,暂时禁用 currentTime 的自动保存 | ||
| */ | ||
| markUserSeeking() { | ||
| this.isUserSeeking = true | ||
|
|
||
| // 取消可能已排队的进度保存任务,避免与用户跳转状态竞争 | ||
| if (this.debounceCurrentTimeTimer) { | ||
| clearTimeout(this.debounceCurrentTimeTimer) | ||
| this.debounceCurrentTimeTimer = null | ||
| } | ||
|
|
||
| // 清除之前的定时器 | ||
| if (this.userSeekingTimer) { | ||
| clearTimeout(this.userSeekingTimer) | ||
| } | ||
|
|
||
| // 在略长于 debounceCurrentTimeMs 的时间后恢复自动保存并立即保存一次 | ||
| const resumeAfterMs = this.debounceCurrentTimeMs + 200 | ||
| this.userSeekingTimer = setTimeout(async () => { | ||
| this.isUserSeeking = false | ||
| this.userSeekingTimer = null | ||
|
|
||
| // 立即保存一次当前播放进度,确保用户跳转后的位置被记录 | ||
| if (this.currentVideoId) { | ||
| try { | ||
| const currentTime = usePlayerStore.getState().currentTime | ||
| await window.api.db.videoLibrary.updatePlayProgress(this.currentVideoId, currentTime) | ||
| logger.debug('用户跳转状态已恢复,立即保存当前进度', { | ||
| videoId: this.currentVideoId, | ||
| currentTime | ||
| }) | ||
| } catch (error) { | ||
| logger.error('用户跳转状态恢复时保存进度失败', { | ||
| videoId: this.currentVideoId, | ||
| error | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| logger.debug('用户跳转状态已恢复,重新启用进度自动保存') | ||
| }, resumeAfterMs) | ||
|
|
||
| logger.debug('已标记用户跳转状态,暂时禁用进度自动保存') | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Seeking guard correctly pauses auto-save; make resume window configurable
Behavior is sound. To avoid magic numbers, expose resumeAfterMs via a constant/config and derive from debounceCurrentTimeMs explicitly.
- const resumeAfterMs = this.debounceCurrentTimeMs + 200
+ const RESUME_GUARD_EXTRA_MS = 200
+ const resumeAfterMs = this.debounceCurrentTimeMs + RESUME_GUARD_EXTRA_MSEnsure PlayerOrchestrator calls markUserSeeking() on all user-driven seeks (drag, click-to-seek, keyboard jumps).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * 标记用户正在跳转,暂时禁用 currentTime 的自动保存 | |
| */ | |
| markUserSeeking() { | |
| this.isUserSeeking = true | |
| // 取消可能已排队的进度保存任务,避免与用户跳转状态竞争 | |
| if (this.debounceCurrentTimeTimer) { | |
| clearTimeout(this.debounceCurrentTimeTimer) | |
| this.debounceCurrentTimeTimer = null | |
| } | |
| // 清除之前的定时器 | |
| if (this.userSeekingTimer) { | |
| clearTimeout(this.userSeekingTimer) | |
| } | |
| // 在略长于 debounceCurrentTimeMs 的时间后恢复自动保存并立即保存一次 | |
| const resumeAfterMs = this.debounceCurrentTimeMs + 200 | |
| this.userSeekingTimer = setTimeout(async () => { | |
| this.isUserSeeking = false | |
| this.userSeekingTimer = null | |
| // 立即保存一次当前播放进度,确保用户跳转后的位置被记录 | |
| if (this.currentVideoId) { | |
| try { | |
| const currentTime = usePlayerStore.getState().currentTime | |
| await window.api.db.videoLibrary.updatePlayProgress(this.currentVideoId, currentTime) | |
| logger.debug('用户跳转状态已恢复,立即保存当前进度', { | |
| videoId: this.currentVideoId, | |
| currentTime | |
| }) | |
| } catch (error) { | |
| logger.error('用户跳转状态恢复时保存进度失败', { | |
| videoId: this.currentVideoId, | |
| error | |
| }) | |
| } | |
| } | |
| logger.debug('用户跳转状态已恢复,重新启用进度自动保存') | |
| }, resumeAfterMs) | |
| logger.debug('已标记用户跳转状态,暂时禁用进度自动保存') | |
| } | |
| /** | |
| * 标记用户正在跳转,暂时禁用 currentTime 的自动保存 | |
| */ | |
| markUserSeeking() { | |
| this.isUserSeeking = true | |
| // 取消可能已排队的进度保存任务,避免与用户跳转状态竞争 | |
| if (this.debounceCurrentTimeTimer) { | |
| clearTimeout(this.debounceCurrentTimeTimer) | |
| this.debounceCurrentTimeTimer = null | |
| } | |
| // 清除之前的定时器 | |
| if (this.userSeekingTimer) { | |
| clearTimeout(this.userSeekingTimer) | |
| } | |
| // 在略长于 debounceCurrentTimeMs 的时间后恢复自动保存并立即保存一次 | |
| const RESUME_GUARD_EXTRA_MS = 200 | |
| const resumeAfterMs = this.debounceCurrentTimeMs + RESUME_GUARD_EXTRA_MS | |
| this.userSeekingTimer = setTimeout(async () => { | |
| this.isUserSeeking = false | |
| this.userSeekingTimer = null | |
| // 立即保存一次当前播放进度,确保用户跳转后的位置被记录 | |
| if (this.currentVideoId) { | |
| try { | |
| const currentTime = usePlayerStore.getState().currentTime | |
| await window.api.db.videoLibrary.updatePlayProgress(this.currentVideoId, currentTime) | |
| logger.debug('用户跳转状态已恢复,立即保存当前进度', { | |
| videoId: this.currentVideoId, | |
| currentTime | |
| }) | |
| } catch (error) { | |
| logger.error('用户跳转状态恢复时保存进度失败', { | |
| videoId: this.currentVideoId, | |
| error | |
| }) | |
| } | |
| } | |
| logger.debug('用户跳转状态已恢复,重新启用进度自动保存') | |
| }, resumeAfterMs) | |
| logger.debug('已标记用户跳转状态,暂时禁用进度自动保存') | |
| } |
| setMuted: (m) => set((s: Draft<PlayerStore>) => void (s.muted = !!m)), | ||
| setPlaybackRate: (r) => | ||
| set((s: Draft<PlayerStore>) => void (s.playbackRate = Math.max(0.25, Math.min(3, r)))), | ||
| setActiveCueIndex: (index) => set((s: Draft<PlayerStore>) => void (s.activeCueIndex = index)), |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Optional: clamp activeCueIndex to -1..Number.MAX_SAFE_INTEGER
Defensive clamp prevents accidental NaN/float assignments from external callers.
-setActiveCueIndex: (index) => set((s: Draft<PlayerStore>) => void (s.activeCueIndex = index)),
+setActiveCueIndex: (index) =>
+ set((s: Draft<PlayerStore>) => {
+ s.activeCueIndex = Number.isInteger(index) ? index : -1
+ }),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| setActiveCueIndex: (index) => set((s: Draft<PlayerStore>) => void (s.activeCueIndex = index)), | |
| setActiveCueIndex: (index) => | |
| set((s: Draft<PlayerStore>) => { | |
| s.activeCueIndex = Number.isInteger(index) ? index : -1 | |
| }), |
🤖 Prompt for AI Agents
In src/renderer/src/state/stores/player.store.ts around line 224, the setter
currently assigns activeCueIndex directly; change it to defensively coerce and
clamp the incoming index: convert the value to a number, if it is NaN set -1,
otherwise truncate to an integer (Math.trunc), then clamp it to the range
-1..Number.MAX_SAFE_INTEGER before assigning to s.activeCueIndex to prevent
NaN/float/out-of-range values from external callers.
| // 立即同步当前的 activeCueIndex 到 store | ||
| if (updater.setActiveCueIndex) { | ||
| updater.setActiveCueIndex(this.context.activeCueIndex) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Sync activeCueIndex on connect — OK; consider initial full-state push.
Pushing the cue index immediately is good. If the updater can connect late, also consider pushing paused/currentTime/duration to avoid first-frame UI jitter.
🤖 Prompt for AI Agents
In src/renderer/src/pages/player/engine/PlayerOrchestrator.ts around lines
208-211, the code only calls
updater.setActiveCueIndex(this.context.activeCueIndex) on connect; update it to
push the initial full playback state to avoid UI jitter by checking for and
calling available updater methods (e.g., setPaused, setCurrentTime, setDuration)
in addition to setActiveCueIndex, using the corresponding values from
this.context; ensure you call them in a sensible order (setPaused, then
setCurrentTime, then setDuration, then setActiveCueIndex) and only invoke
methods that exist on the updater.
| // 标记用户跳转状态,暂时禁用自动保存 | ||
| import('@renderer/services/PlayerSettingsSaver').then( | ||
| ({ playerSettingsPersistenceService }) => { | ||
| playerSettingsPersistenceService.markUserSeeking() | ||
| } | ||
| ) | ||
|
|
There was a problem hiding this comment.
Unhandled rejection on dynamic import — add catch (can crash in Electron if unhandled).
If the import fails, the rejection is unobserved. Log it to maintain observability.
Apply this diff:
- import('@renderer/services/PlayerSettingsSaver').then(
- ({ playerSettingsPersistenceService }) => {
- playerSettingsPersistenceService.markUserSeeking()
- }
- )
+ import('@renderer/services/PlayerSettingsSaver')
+ .then(({ playerSettingsPersistenceService }) => {
+ playerSettingsPersistenceService.markUserSeeking()
+ })
+ .catch((error) => {
+ logger.error('Failed to markUserSeeking()', { error })
+ })Optional (outside this hunk): cache the module to avoid repeated dynamic imports during scrubbing:
// top-level helper
const getSettingsSaver = (() => {
let p: Promise<typeof import('@renderer/services/PlayerSettingsSaver')> | null = null
return () => (p ??= import('@renderer/services/PlayerSettingsSaver'))
})()🤖 Prompt for AI Agents
In src/renderer/src/pages/player/engine/PlayerOrchestrator.ts around lines 433
to 439, the dynamic import of '@renderer/services/PlayerSettingsSaver' is
missing error handling which can produce an unhandled rejection in Electron;
wrap the import promise with a .catch that logs the error via the existing
logger (or console.error) and prevents throwing, and optionally replace repeated
dynamic imports by using a module-level cached getter (a closure that holds the
import Promise) and call that cached getter here so repeated seeks don't
re-trigger imports.
| // 存储追踪记录(减少到最多 50 条,优化内存使用) | ||
| this._traceBuf.push(traceRecord) | ||
| if (this._traceBuf.length > 200) this._traceBuf.shift() | ||
| if (this._traceBuf.length > 50) this._traceBuf.shift() | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Trace buffer capped at 50 — pragmatic memory win.
If you ever need longer windows without O(n) shift cost, consider a ring buffer, but 50 is fine.
🤖 Prompt for AI Agents
In src/renderer/src/pages/player/engine/PlayerOrchestrator.ts around lines
1266-1269, the trace buffer currently uses Array.push + shift which is O(n) on
overflow; replace it with a fixed-size ring/circular buffer to avoid shifting:
add fields for buffer array (capacity 50), head index and count, implement push
that writes into buffer[(head+count)%capacity] when not full or overwrites
buffer[head] and increments head when full, update count = min(count+1,
capacity), and update any reads/iterators to traverse from head for count items
in order. Ensure semantics (most-recent ordering, max 50 entries) remain
identical.
| {(ffmpegStatus?.isDownloaded || ffmpegStatus?.isBundled) && | ||
| !ffmpegStatus?.isSystemFFmpeg && ( | ||
| <Popconfirm | ||
| title={t('settings.plugins.ffmpeg.uninstall.confirm_title')} | ||
| description={t('settings.plugins.ffmpeg.uninstall.confirm_description')} | ||
| onConfirm={handleUninstall} | ||
| okText={t('settings.plugins.ffmpeg.uninstall.confirm')} | ||
| cancelText={t('common.cancel')} | ||
| okType="danger" | ||
| > | ||
| <Button danger icon={<Trash2 size={14} />}> | ||
| {t('settings.plugins.ffmpeg.uninstall.button')} | ||
| </Button> | ||
| </Popconfirm> | ||
| )} | ||
| </ActionButtonGroup> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
“Uninstall” should not be shown for bundled/system FFmpeg
Current condition displays Uninstall for bundled builds, but the handler removes only downloaded binaries. Scope the button to downloaded-only to avoid broken UX.
- {(ffmpegStatus?.isDownloaded || ffmpegStatus?.isBundled) &&
- !ffmpegStatus?.isSystemFFmpeg && (
+ {ffmpegStatus?.isDownloaded && !ffmpegStatus?.isSystemFFmpeg && (
<Popconfirm📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {(ffmpegStatus?.isDownloaded || ffmpegStatus?.isBundled) && | |
| !ffmpegStatus?.isSystemFFmpeg && ( | |
| <Popconfirm | |
| title={t('settings.plugins.ffmpeg.uninstall.confirm_title')} | |
| description={t('settings.plugins.ffmpeg.uninstall.confirm_description')} | |
| onConfirm={handleUninstall} | |
| okText={t('settings.plugins.ffmpeg.uninstall.confirm')} | |
| cancelText={t('common.cancel')} | |
| okType="danger" | |
| > | |
| <Button danger icon={<Trash2 size={14} />}> | |
| {t('settings.plugins.ffmpeg.uninstall.button')} | |
| </Button> | |
| </Popconfirm> | |
| )} | |
| </ActionButtonGroup> | |
| {ffmpegStatus?.isDownloaded && !ffmpegStatus?.isSystemFFmpeg && ( | |
| <Popconfirm | |
| title={t('settings.plugins.ffmpeg.uninstall.confirm_title')} | |
| description={t('settings.plugins.ffmpeg.uninstall.confirm_description')} | |
| onConfirm={handleUninstall} | |
| okText={t('settings.plugins.ffmpeg.uninstall.confirm')} | |
| cancelText={t('common.cancel')} | |
| okType="danger" | |
| > | |
| <Button danger icon={<Trash2 size={14} />}> | |
| {t('settings.plugins.ffmpeg.uninstall.button')} | |
| </Button> | |
| </Popconfirm> | |
| )} | |
| </ActionButtonGroup> |
🤖 Prompt for AI Agents
In src/renderer/src/pages/settings/FFmpegSettings.tsx around lines 436 to 451,
the Uninstall button is currently shown for bundled builds because the condition
checks ffmpegStatus?.isDownloaded || ffmpegStatus?.isBundled; change the
condition so the Uninstall Popconfirm only appears for downloaded binaries
(remove isBundled) and still hide for system FFmpeg, i.e. require
ffmpegStatus?.isDownloaded && !ffmpegStatus?.isSystemFFmpeg; ensure the
uninstall handler remains targeted at downloaded installations only.
| display: flex; | ||
| align-items: center; | ||
| gap: 8px; | ||
| font-size: 14px; | ||
| ` |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Replace hardcoded spacing and font size with theme tokens
Follow styling guideline to avoid hardcoded values.
const StatusContainer = styled.div`
display: flex;
align-items: center;
- gap: 8px;
- font-size: 14px;
+ gap: ${SPACING.XS}px;
+ font-size: ${FONT_SIZES.SM}px;
`📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| display: flex; | |
| align-items: center; | |
| gap: 8px; | |
| font-size: 14px; | |
| ` | |
| const StatusContainer = styled.div` | |
| display: flex; | |
| align-items: center; | |
| gap: ${SPACING.XS}px; | |
| font-size: ${FONT_SIZES.SM}px; | |
| ` |
🤖 Prompt for AI Agents
In src/renderer/src/pages/settings/FFmpegSettings.tsx around lines 468-472, the
styled block uses hardcoded gap: 8px and font-size: 14px; replace these with
theme tokens instead (use the project's theme via props or useTheme) — swap gap:
8px for the appropriate theme spacing token (e.g., theme.spacing(x) or
theme.tokens.spacing.small) and replace font-size: 14px with the theme font-size
token (e.g., theme.fontSizes.small or theme.tokens.fontSize.body) so styling
follows the design system.
| const PathInputContainer = styled.div` | ||
| display: flex; | ||
| gap: 8px; | ||
| align-items: center; | ||
|
|
||
| .ant-input { | ||
| flex: 1; | ||
| max-width: 250px; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Avoid hardcoded spacing; use SPACING token
Align with design tokens.
const PathInputContainer = styled.div`
display: flex;
- gap: 8px;
+ gap: ${SPACING.XS}px;
align-items: center;
.ant-input {
flex: 1;
max-width: 250px;
}
`Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/renderer/src/pages/settings/FFmpegSettings.tsx around lines 595 to 603,
replace the hardcoded spacing values with design tokens: import the SPACING
token object from your design tokens module and use it instead of numeric
literals — e.g. change gap: 8px to gap: ${SPACING.SMALL} (or the appropriate
small token) and change .ant-input max-width: 250px to max-width: ${SPACING.XL}
(or the appropriate width token); ensure the import path matches your project
tokens module and use the token names consistent with your token definitions.
Summary by CodeRabbit