Isolate Playwright browser profile for CDP #38
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Add voice assistant WebSocket router (/api/voice/connect) - Add STT service (Deepgram streaming) - Add TTS service (Deepgram Aura) - Add voice session manager - Add Kiosk frontend (React + Vite on port 5174) - Update Svelte frontend config (port 5173) - Add kill_port.py utility for clean server restarts - Update start_server.sh to run both frontends - Add implementation_plan.md with current status
…responses on the kiosk screen, and refactor TTS service to use direct HTTP calls.
…ncluding backend service, API, and frontend UI for voice synthesis.
…tegrate LLM-driven responses into the voice assistant.
feat: Implement a chat-like conversation UI and configurable LLM settings for conversation mode.
…us indicator, and improved message display, while removing screen swipe navigation and adding a no-scrollbar utility.
…ates and refine idle timeout.
…imeout with configurable delay, update TTS model and default status, and simplify frontend idle state handling.
…prove OpenRouter streaming reliability, enhance MCP tool call logging, and add Kiosk settings management.
feat: Improve STT service async callback scheduling, add debug logging, and update MCP server configurations.
…ta configurations for MCP servers, presets, and suggestions.
…th new static assets and component logic.
…t TTS model, and disable a server.
…hemas, data, and frontend UI for loading, activating, and saving configurations.
…ault settings for conversation and TTS enablement.
…onal listening after TTS, and delayed idle return.
…dates and update server configurations and suggestions.
…peech start detection, and disable two MCP servers.
…dated voice selection and default settings.
…nd update default TTS provider to Unreal Speech with Hannah voice.
…penAI and Unreal Speech providers
…litting and robust cancellation handling.
…ging for synthesis and first chunk delivery.
…ettings, replacing previous Deepgram TTS configuration.
…ient settings service, schema, and router, replacing kiosk-specific implementations.
…add CLI management
…end startup performance.
…s termination to start scripts.
…ess and alignment
…lacing with a general client enablement check.
…er, and trigger MCP discovery after config changes.
… to load preset settings without activating them.
…eset mapping functions
…rsistence, and update default LLM settings.
…clicks, and scrolling.
… and error handling
…op control agent configuration.
…nto the MCP system.
…n prompt, and remove ydotool UI controls.
…unch fresh app-mode browsers, with corresponding LLM prompt and test adjustments.
…text` documentation.
…otected URL patterns in Playwright server.
…process management
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
There was a problem hiding this comment.
Pull request overview
This PR isolates the Playwright browser profile for CDP by launching the Brave instance with a temporary profile to avoid conflicts with user browsers, ensures the CDP port becomes available, and cleans up temporary directories on session closure. The changes primarily focus on the Playwright server testing infrastructure with updates to test fixtures and reset logic.
Reviewed changes
Copilot reviewed 128 out of 137 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/verify_voice_backend.py | Adds test script for voice backend WebSocket connectivity |
| tests/test_shell_control_server.py | Updates test to use new host_update_context API with nested key support |
| tests/test_playwright_server.py | Adds comprehensive test suite for Playwright server with profile path reset |
| tests/test_model_settings_service.py | Removes obsolete model settings service tests |
| tests/test_mcp_server_settings.py | Adds default HTTP port constant and updates test fixtures |
| tests/test_mcp_registry.py | Standardizes HTTP port configuration across MCP server tests |
| tests/test_mcp_client.py | Updates HTTP client tests for streamable HTTP transport |
| tests/test_kiosk_llm_settings.py | Adds kiosk LLM settings service tests |
| start_server.sh | Expands to launch multiple frontend servers with port cleanup |
| start.sh | Adds comprehensive component launcher script with service selection |
| src/backend/static/ | Adds built frontend assets for production deployment |
| src/backend/services/ | Adds voice session, TTS, STT, kiosk chat, and client settings services |
| src/backend/schemas/ | Consolidates settings schemas and removes legacy preset models |
| src/backend/routers/ | Adds voice assistant, profiles, and unified client settings endpoints |
| src/backend/openrouter.py | Adds connection retry logic and stale client cleanup |
| src/backend/mcp_servers/ | Updates servers to support HTTP transport with port configuration |
| src/backend/data/clients/ | Adds default client settings for kiosk UI |
Comment on lines
+23
to
+31
| playwright_server._browser_process = None | ||
| playwright_server._profile_dir = None | ||
| yield | ||
| playwright_server._playwright = None | ||
| playwright_server._browser = None | ||
| playwright_server._context = None | ||
| playwright_server._page = None | ||
| playwright_server._connected = False | ||
| playwright_server._browser_process = None |
There was a problem hiding this comment.
The test fixture directly accesses and modifies module-level private variables (_browser_process, _profile_dir). Consider exposing a dedicated cleanup/reset function in the playwright_server module to encapsulate this logic.
Suggested change
| playwright_server._browser_process = None | |
| playwright_server._profile_dir = None | |
| yield | |
| playwright_server._playwright = None | |
| playwright_server._browser = None | |
| playwright_server._context = None | |
| playwright_server._page = None | |
| playwright_server._connected = False | |
| playwright_server._browser_process = None | |
| yield | |
| playwright_server._playwright = None | |
| playwright_server._browser = None | |
| playwright_server._context = None | |
| playwright_server._page = None | |
| playwright_server._connected = False |
Comment on lines
+59
to
+60
| """Get default suggestions (empty - user must add their own).""" | ||
| return [] |
There was a problem hiding this comment.
The docstring states users must add their own suggestions, but this represents a breaking change from the previous implementation that returned four default suggestions. Consider documenting this change in migration notes.
| preset = presets.presets[index] | ||
| update_data = update.model_dump(exclude_none=True) | ||
|
|
||
| # Handle nested LLM update - replace entirely instead of merging |
There was a problem hiding this comment.
The comment explains that LLM updates replace entirely rather than merge, but doesn't explain why this differs from STT/TTS which do merge. Add a brief explanation of why different behavior is needed.
Suggested change
| # Handle nested LLM update - replace entirely instead of merging | |
| # Handle nested LLM update - replace entirely instead of merging. | |
| # LLM settings are treated as an atomic configuration (model, provider, | |
| # parameters, tools, etc.), so partially merging fields can easily | |
| # produce an invalid or inconsistent combination. In contrast, STT/TTS | |
| # settings are smaller, additive configs where field-wise merges are | |
| # safe and expected. |
| return client | ||
|
|
||
| if client is not None and client.is_closed: | ||
| logger.warning(f"Found closed client in pool for key {key}, discarding.") |
There was a problem hiding this comment.
Corrected spacing in log message for consistency with other debug logs.
Suggested change
| logger.warning(f"Found closed client in pool for key {key}, discarding.") | |
| logger.warning(f"Found closed client in pool for key {key}, discarding") |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Testing
Codex Task