Conversation
- Updated manifest.json to include side panel permissions and default path. - Enhanced vite.config.js to support side panel input and alias. - Introduced new side panel settings in configSchema.js for user preferences and behavior. - Added side panel action constants in messageActions.js for improved messaging capabilities.
- Added side panel service initialization and registration in background scripts. - Updated message handler to support new side panel actions for word selection and state management. - Enhanced content scripts to handle side panel interactions, including word selection and video control. - Introduced new message actions for side panel communication and state updates. - Improved user experience by integrating side panel features into existing workflows and UI components.
- Added a new message action for synchronizing word selections between the content script and the side panel. - Enhanced the message handler to process selection sync messages and forward them appropriately. - Updated the side panel service to handle selection sync requests and manage active connections. - Improved content scripts to clear selections on subtitle changes and synchronize state with the side panel. - Refactored word selection handling to accommodate new selection actions and ensure consistent state management.
- Added a new alias for shared components in vite.config.js. - Implemented selection synchronization in SidePanelService to manage word selections more effectively. - Updated content scripts to handle selection updates and synchronize state with the side panel. - Refactored word selection logic to ensure consistent state management and prevent race conditions. - Introduced a spinner animation in side panel CSS for improved user feedback during loading states.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @QuellaMC, your pull request is larger than the review limit of 150000 diff characters
There was a problem hiding this comment.
Pull Request Overview
This PR migrates the AI analysis modal to a Chrome Side Panel implementation, modernizing the UI architecture with React while adding comprehensive side panel functionality. The changes introduce a new side panel feature for AI context analysis with word selection and state management across tabs.
Key Changes:
- Added complete Chrome Side Panel implementation with React components and hooks
- Implemented word selection synchronization between content scripts and side panel
- Added Disney+ platform-specific playback controls (pause/resume)
- Reformatted translation strings across all locale files for improved readability
Reviewed Changes
Copilot reviewed 62 out of 68 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| manifest.json | Added sidePanel permission and configuration |
| vite.config.js | Added sidepanel entry point and path aliases |
| sidepanel/* | New React-based side panel implementation with hooks and components |
| background/services/sidePanelService.js | New service for managing side panel state and messaging |
| content_scripts/core/BaseContentScript.js | Added side panel integration and message handlers |
| content_scripts/shared/subtitleUtilities.js | Enhanced subtitle change event handling for side panel sync |
| video_platforms/disneyPlusPlatform.js | Added platform-specific playback control methods |
| _locales/*/messages.json | Reformatted Vertex AI translation strings |
| options/components/sections/* | Added Word Lists and Advanced Settings sections |
Comments suppressed due to low confidence (1)
background/services/sidePanelService.js:1
- Magic number
3600000(1 hour in milliseconds) should be defined as a named constant for better readability and maintainability.
/**
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
sidepanel/hooks/useWordSelection.js
Outdated
| if (inFlightRef.current) { | ||
| return; // prevent parallel requests | ||
| } |
There was a problem hiding this comment.
The throttling mechanism using inFlightRef and minSyncIntervalMs could lead to lost sync requests if multiple calls occur rapidly. Consider using a queue-based approach or debouncing instead of dropping requests entirely.
| font-size: var(--font-size-sm); | ||
| font-weight: 600; | ||
| padding: var(--spacing-1) var(--spacing-2); | ||
| border-radius: 6px; |
There was a problem hiding this comment.
Hardcoded border-radius value of 6px inconsistent with CSS design system variables. Consider using var(--radius-default) or defining a new radius variable for word tags.
| border-radius: 6px; | |
| border-radius: var(--radius-default); |
| function getActiveVideoElement() { | ||
| const isDisney = typeof location !== 'undefined' && location.hostname.includes('disneyplus.com'); |
There was a problem hiding this comment.
Platform detection logic duplicated in multiple places (here and in pauseVideoAggressively). This should be extracted to a shared utility function or constant to maintain DRY principle.
| if (isDisney) { | ||
| // Route to content script/platform handler to avoid direct pause bugs on Disney+ | ||
| try { | ||
| const resp = await chrome.runtime.sendMessage({ action: 'sidePanelPauseVideo', source: 'interactive' }); |
There was a problem hiding this comment.
Missing error handling for the chrome.runtime.sendMessage call. If the background script is unavailable or the message fails, the catch block swallows the error silently. Consider logging the specific error or providing user feedback.
| * Set service dependencies (will be injected after services are created) | ||
| */ | ||
| setServices(translationService, subtitleService, aiContextService = null) { | ||
| setServices(translationService, subtitleService, aiContextService = null, sidePanelService = null) { |
There was a problem hiding this comment.
The method signature is becoming unwieldy with 4 parameters. Consider using an options object pattern for better maintainability: setServices({ translationService, subtitleService, aiContextService, sidePanelService })
| setServices(translationService, subtitleService, aiContextService = null, sidePanelService = null) { | |
| /** | |
| * Set service dependencies (will be injected after services are created) | |
| * @param {Object} services | |
| * @param {ServiceProtocol} services.translationService | |
| * @param {ServiceProtocol} services.subtitleService | |
| * @param {ServiceProtocol} [services.aiContextService] | |
| * @param {ServiceProtocol} [services.sidePanelService] | |
| */ | |
| setServices({ translationService, subtitleService, aiContextService = null, sidePanelService = null }) { |
| const btn = root.querySelector('button') || root.querySelector('[role="button"]'); | ||
| if (!btn) return false; | ||
| btn.click(); | ||
| await new Promise((r) => setTimeout(r, 160)); |
There was a problem hiding this comment.
Magic number 160 milliseconds for wait time should be defined as a named constant (e.g., PLAYBACK_TRANSITION_DELAY_MS) to document why this specific delay is needed and make it easier to adjust if necessary.
| )} | ||
| > | ||
| <div className="info-message"> | ||
| <span className="material-symbols-outlined" style={{ color: '#f59e0b', marginRight: '0.5rem' }}>warning</span> |
There was a problem hiding this comment.
Inline styles with hardcoded color value #f59e0b and spacing. These should use CSS variables from the design system (e.g., var(--color-warning) and var(--spacing-2)) for consistency.
| <span className="material-symbols-outlined" style={{ color: '#f59e0b', marginRight: '0.5rem' }}>warning</span> | |
| <span className="material-symbols-outlined" style={{ color: 'var(--color-warning)', marginRight: 'var(--spacing-2)' }}>warning</span> |
- Updated the `setServices` method in `MessageHandler` to accept an options object for better clarity and flexibility. - Adjusted related tests to align with the new service injection format. - Enhanced logging to reflect the updated service structure. - Improved compatibility with existing positional arguments for backward compatibility.
… and side panel - Added event listener for selection cleared events in AIContextManager to notify the side panel. - Implemented _handleSelectionCleared method to send messages to the side panel when selection is cleared. - Updated SelectionPersistenceManager to clear selection on content changes and log relevant information. - Modified subtitleUtilities to dispatch content change events when subtitles are updated or cleared. - Integrated selection clear event handling in the side panel to ensure consistent state management.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 67 out of 73 changed files in this pull request and generated 8 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| : 'debug') && | ||
| (typeof message === 'string'); |
There was a problem hiding this comment.
This expression has no effect.
| : 'debug') && | |
| (typeof message === 'string'); | |
| : 'debug'); |
sidepanel/SidePanelApp.jsx
Outdated
| return null; | ||
| } | ||
| const [activeTab, setActiveTab] = useState('ai-analysis'); | ||
| const { theme, toggleTheme } = useTheme(); |
There was a problem hiding this comment.
Unused variable toggleTheme.
| } = useSidePanelContext(); | ||
|
|
||
| const { analyzeWords, retryAnalysis, settings } = useAIAnalysis(); | ||
| const { toggleWord, clearSelection } = useWordSelection(); |
There was a problem hiding this comment.
Unused variable clearSelection.
sidepanel/hooks/useWordSelection.js
Outdated
| targetLanguage, | ||
| } = useSidePanelContext(); | ||
|
|
||
| const { onMessage, sendToActiveTab, sendMessage, getActiveTab } = |
There was a problem hiding this comment.
Unused variable getActiveTab.
sidepanel/hooks/useWordSelection.js
Outdated
| /** | ||
| * Handle word selected event from content script | ||
| */ | ||
| const handleWordSelected = useCallback( |
There was a problem hiding this comment.
Unused variable handleWordSelected.
sidepanel/hooks/useWordSelection.js
Outdated
| return; | ||
| } | ||
|
|
||
| const { word, sourceLanguage, targetLanguage } = data; |
There was a problem hiding this comment.
Unused variable word.
| } | ||
| } | ||
|
|
||
| const normalizedWord = (word || '').trim(); |
There was a problem hiding this comment.
This use of variable 'word' always evaluates to true.
- Updated message handling in MessageHandler to forward selection data more robustly. - Improved SidePanelService to clear previous mappings for active connections and handle selection state more effectively. - Enhanced AIContextModalEvents to send word selection updates to the background for synchronization. - Refactored SidePanelContext to manage tab states and selection updates, ensuring consistent state across active tabs. - Added platform-specific playback controls for Netflix to support video pause and resume functionality.
- Implemented a fallback mechanism for original language subtitles when the requested language is unavailable. - Added logging to inform users when a fallback language is used or when no available languages are found. - Updated the handling of original track selection to ensure robust subtitle fetching and processing.
- Enhanced the message handling in MessageHandler to streamline open reason storage and prevent UI flips. - Improved SidePanelService to manage selection synchronization more effectively, including robust error handling and connection management. - Updated SidePanelApp to ignore background-driven activeTab changes, ensuring a stable user experience. - Refactored useWordSelection hook to utilize setSelectedWords for state management, reducing race conditions and improving clarity. - Implemented auto-reconnect and heartbeat mechanisms in useSidePanelCommunication for better connection reliability.
…dow management - Introduced window-scoped connection tracking in SidePanelService to manage active connections by window. - Updated message handling to include windowId and panelInstanceId for better context during side panel registration. - Refactored tab activation handling to notify only active side panel connections in the same window. - Enhanced useSidePanelCommunication to support sending messages to the currently bound tab, improving message routing. - Added new configuration options for side panel behavior and scope policies in configSchema. - Updated hooks to utilize the new communication methods and ensure consistent state management across tabs.
… improved tab handling - Changed permissions in manifest.json to include 'tabs' for enhanced functionality. - Removed outdated hydration logic in useWordSelection hook to streamline initialization. - Introduced activeTabIdRef to track the currently active tab, improving synchronization with tab updates. - Refactored tab update handling to ensure actions are only taken on the active tab, enhancing performance and reliability.
- Introduced forced tab binding in SidePanelService to ensure UI updates reflect the active tab, even when 'follow active tab' is disabled. - Updated SidePanelContext to handle forced tab binding messages, improving responsiveness to user interactions. - Refactored useWordSelection hook to streamline word selection updates and ensure synchronization with the currently active tab. - Improved error handling and logging for better debugging and user feedback during selection synchronization.
…selection from content scripts and tracking connections per window.
…allowing zero values for delay, and refactor initialization logging.
… several synchronization and event handling bugs.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 75 out of 82 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…elements and messages.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
….com/QuellaMC/DualSub into migrate/ai-analysis-modal-to-sidebar
…ompts to allow LLM to infer source language.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 79 out of 86 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import { useTheme } from './hooks/useTheme.js'; | ||
| import { useSettings } from './hooks/useSettings.js'; | ||
| import { SidePanelProvider } from './hooks/SidePanelContext.jsx'; | ||
| import { useSidePanelCommunication } from './hooks/useSidePanelCommunication.js'; |
There was a problem hiding this comment.
Unused import useSidePanelCommunication.
| ]); | ||
| const { t } = useTranslation(); | ||
|
|
||
| const handleTabChange = useCallback( |
There was a problem hiding this comment.
Unused variable handleTabChange.
| } = useSidePanelContext(); | ||
|
|
||
| const { analyzeWords, retryAnalysis, settings } = useAIAnalysis(); | ||
| const { toggleWord, clearSelection } = useWordSelection(); |
There was a problem hiding this comment.
Unused variable clearSelection.
| } | ||
| } | ||
|
|
||
| const normalizedWord = (word || '').trim(); |
There was a problem hiding this comment.
This use of variable 'word' always evaluates to true.
This pull request introduces a major migration of the popup and options pages to React, modernizing the UI architecture and build system for better maintainability and performance. It also updates translation files for improved formatting and clarity across all supported languages, and adds documentation for the migration. The most important changes are grouped below.
React migration and build system updates:
Localization and translation improvements:
Documentation and technical details:
Service dependency injection:
MessageHandlerclass to support injection of a newsidePanelServicedependency, improving service management and debugging.Bug fixes and UI improvements: