-
Notifications
You must be signed in to change notification settings - Fork 0
Development #32
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Development #32
Conversation
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: pladisdev <127021507+pladisdev@users.noreply.github.com>
Co-authored-by: pladisdev <127021507+pladisdev@users.noreply.github.com>
Co-authored-by: pladisdev <127021507+pladisdev@users.noreply.github.com>
Co-authored-by: pladisdev <127021507+pladisdev@users.noreply.github.com>
Co-authored-by: pladisdev <127021507+pladisdev@users.noreply.github.com>
…4b50-93c4-0e11b13594f4 Clarify error handler ordering is correct - no changes needed
Co-authored-by: pladisdev <127021507+pladisdev@users.noreply.github.com>
Co-authored-by: pladisdev <127021507+pladisdev@users.noreply.github.com>
…47b6-b204-5bf8892748d3 Extract magic numbers to named constants in audio playback code
…4606-8149-97ceaa22873d Use platform.system() instead of sys.platform for OS detection
Extract duplicated hex opacity calculation to utility function
Extract avatar active offset magic number into configurable setting
Fix audio reference cleanup on play() failure in popup mode
Fix race condition in popup avatar lifecycle
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR includes a patch release (v1.3.1) that addresses several technical improvements: refactoring queue management imports to use a dedicated module, fixing a frontend undefined check bug, enhancing settings loading with default value merging for backward compatibility, and improving test isolation with credential mocking.
Key Changes
- Refactored queue object imports (
avatar_message_queue,parallel_message_queue) fromapptomodules.queue_manageracross multiple router files for better code organization - Fixed frontend undefined check to prevent incorrect custom mode activation when
parallelMessageLimitis undefined - Enhanced
get_settings()to merge database settings with defaults for backward compatibility with older databases
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/src/components/settings/GeneralSettings.jsx | Added undefined check to prevent incorrect custom mode when parallelMessageLimit is not loaded |
| backend/version.py | Bumped version from 1.3.0 to 1.3.1 for patch release |
| backend/tests/test_clearchat.py | Added fixture to mock Twitch credentials at module level |
| backend/routers/system.py | Refactored to import parallel_message_queue from modules.queue_manager |
| backend/routers/config_backup.py | Refactored to import avatar_message_queue from modules.queue_manager |
| backend/routers/avatars.py | Refactored to import avatar_message_queue from modules.queue_manager across multiple endpoints |
| backend/modules/persistent_data.py | Enhanced get_settings() to merge defaults with database settings for backward compatibility |
Comments suppressed due to low confidence (1)
backend/tests/test_clearchat.py:7
- Import of 'os' is not used.
import os
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
| """ | ||
| import pytest | ||
| import asyncio | ||
| import os |
Copilot
AI
Nov 25, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The os import is unused and should be removed.
| import os |
backend/tests/test_clearchat.py
Outdated
| @pytest.fixture(autouse=True) | ||
| def mock_twitch_credentials(monkeypatch): | ||
| """Mock Twitch credentials for all tests - module level to ensure it's set before imports""" | ||
| # Patch the module-level constants that were already imported | ||
| import modules.persistent_data | ||
| monkeypatch.setattr(modules.persistent_data, 'TWITCH_CLIENT_ID', 'test_client_id') | ||
| monkeypatch.setattr(modules.persistent_data, 'TWITCH_CLIENT_SECRET', 'test_client_secret') |
Copilot
AI
Nov 25, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This fixture has the same name as the autouse fixture in conftest.py (lines 45-67), which will be shadowed by this local fixture for this test module. Consider either:
- Renaming this fixture to avoid confusion (e.g.,
mock_twitch_module_credentials) - Removing this fixture if the conftest.py one is sufficient
- Adding a comment explaining why this duplicate/override is necessary
The conftest.py fixture sets environment variables, while this one patches module-level constants. If both are needed, the naming conflict should be resolved.
| return defaults | ||
| except Exception as e: | ||
| logger.error(f"Error loading settings: {e}") | ||
| return {} |
Copilot
AI
Nov 25, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The exception handler returns an empty dict {} instead of defaults, which is inconsistent with the behavior when no settings are found in the database (line 109, which returns defaults). Consider returning defaults here as well to ensure the application has sensible default values even in error cases.
| return {} | |
| return defaults |
✅ Windows Build SuccessfulExecutable: Build Status
Download the artifacts from the workflow run to test before merging. Once merged to |
🐳 Docker Image Built SuccessfullyImage: Test this PR with Docker:docker pull ghcr.io/pladisdev/chat-yapper:pr-10ca204
docker run -d \
--name chat-yapper-pr \
-p 8069:8008 \
-e TWITCH_CLIENT_ID=your_id \
-e TWITCH_CLIENT_SECRET=your_secret \
ghcr.io/pladisdev/chat-yapper:pr-10ca204Access at: http://localhost:8069 The Docker image will be published to the GitHub Container Registry when merged to |
✅ Windows Build SuccessfulExecutable: Build Status
Download the artifacts from the workflow run to test before merging. Once merged to |
🐳 Docker Image Built SuccessfullyImage: Test this PR with Docker:docker pull ghcr.io/pladisdev/chat-yapper:pr-12339a4
docker run -d \
--name chat-yapper-pr \
-p 8069:8008 \
-e TWITCH_CLIENT_ID=your_id \
-e TWITCH_CLIENT_SECRET=your_secret \
ghcr.io/pladisdev/chat-yapper:pr-12339a4Access at: http://localhost:8069 The Docker image will be published to the GitHub Container Registry when merged to |
No description provided.