mirror of
https://github.com/ChuckBuilds/LEDMatrix.git
synced 2026-08-06 11:18:06 +00:00
fix: address CodeRabbit review findings
Verified each finding against the code; fixes for the valid ones: - install_dependencies_apt.py: the installer listed 'freetype', but the declared dependency is freetype-py — an apt miss would pip-install the wrong PyPI package. Now installs freetype-py with an import-name mapping (pre-existing bug, surfaced by the review). - api_v3.py: pixel_mapper_config is validated as a string before being saved to display.hardware (JSON callers could previously store an object/list the matrix library can't use). - .cursorrules: the Plugin Loading Process and File Organization sections still said discovery scans plugins/ — now consistent with the corrected overview (configured directory, default plugin-repos/). - README.md: removed the stale '(except the core calendar)' claim — no core calendar exists in src/ — and qualified the plugin inventory (official plugins in the monorepo; third-party from their own repos). - CONFIG_REFERENCE.md: hardware_mapping now shows the code fallback (adafruit-hat-pwm) alongside the template value. - PLUGIN_REGISTRY_SETUP_GUIDE.md: check_plugin.py takes --plugin, not a positional id. - scripts/fix_perms/fix_*.sh: exec bits set so the documented 'sudo ./...' invocations work. - Guard tests hardened: template guard now catches multi-line render_template() calls; widget guard parses actual <script> src values and fails if the widgets dir goes missing; type hints and docstrings added per repo coding guidelines. Skipped with reasons (noted on the PR): limit_refresh_rate_hz 100-vs-90 is documented as intentional in CONFIG_REFERENCE.md; the psutil comment already names the enforcing manifest; docs/archive/ findings are out of scope per the docs policy (archive may rot). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SXb4mKcAkVaxkeTb3YnAdr
This commit is contained in:
+2
-2
@@ -227,7 +227,7 @@ Common optional fields:
|
|||||||
|
|
||||||
## Plugin Loading Process
|
## Plugin Loading Process
|
||||||
|
|
||||||
1. **Discovery**: PluginManager scans `plugins/` directory for directories containing `manifest.json`
|
1. **Discovery**: PluginManager scans the configured `plugin_system.plugins_directory` (default `plugin-repos/`) for directories containing `manifest.json`
|
||||||
2. **Validation**: Validates manifest structure and required fields
|
2. **Validation**: Validates manifest structure and required fields
|
||||||
3. **Loading**: Imports plugin module and instantiates plugin class
|
3. **Loading**: Imports plugin module and instantiates plugin class
|
||||||
4. **Configuration**: Loads plugin config from `config/config.json`
|
4. **Configuration**: Loads plugin config from `config/config.json`
|
||||||
@@ -301,7 +301,7 @@ Common optional fields:
|
|||||||
## File Organization
|
## File Organization
|
||||||
|
|
||||||
```
|
```
|
||||||
plugins/
|
plugin-repos/ # default install dir (plugins/ when using dev symlinks)
|
||||||
<plugin-id>/
|
<plugin-id>/
|
||||||
manifest.json # Plugin metadata
|
manifest.json # Plugin metadata
|
||||||
manager.py # Main plugin class
|
manager.py # Main plugin class
|
||||||
|
|||||||
@@ -54,10 +54,11 @@ LEDMatrix is a plugin platform: the displays below are plugins installed
|
|||||||
from the built-in Plugin Store (web interface → Plugins), where each can be
|
from the built-in Plugin Store (web interface → Plugins), where each can be
|
||||||
individually enabled, ordered, and configured — display durations, teams,
|
individually enabled, ordered, and configured — display durations, teams,
|
||||||
stocks, weather, timezones, and more. The core repo ships with just two
|
stocks, weather, timezones, and more. The core repo ships with just two
|
||||||
bundled plugins (`starlark-apps` and `web-ui-info`); everything else lives
|
bundled plugins (`starlark-apps` and `web-ui-info`); the official plugins
|
||||||
in the [ledmatrix-plugins](https://github.com/ChuckBuilds/ledmatrix-plugins)
|
live in the [ledmatrix-plugins](https://github.com/ChuckBuilds/ledmatrix-plugins)
|
||||||
monorepo and installs with one click. Displays available in the store
|
monorepo and install with one click, and third-party plugins can be
|
||||||
include:
|
installed from their own GitHub repositories. Displays available in the
|
||||||
|
store include:
|
||||||
|
|
||||||
### Time and Weather
|
### Time and Weather
|
||||||
- Real-time clock display (2x 64x32 Displays 4mm Pixel Pitch)
|
- Real-time clock display (2x 64x32 Displays 4mm Pixel Pitch)
|
||||||
@@ -428,7 +429,7 @@ I recommend using the web-ui "Quick Actions" to control the Display.
|
|||||||
## Plugins
|
## Plugins
|
||||||
|
|
||||||
<details>
|
<details>
|
||||||
LEDMatrix uses a plugin-based architecture where all display functionality (except the core calendar) is implemented as plugins. All managers that were previously built into the core system are now available as plugins through the Plugin Store.
|
LEDMatrix uses a plugin-based architecture where all display functionality is implemented as plugins. All managers that were previously built into the core system are now available as plugins through the Plugin Store.
|
||||||
|
|
||||||
### Plugin Store
|
### Plugin Store
|
||||||
See the [Plugin Store documentation](https://github.com/ChuckBuilds/ledmatrix-plugins) for detailed installation instructions.
|
See the [Plugin Store documentation](https://github.com/ChuckBuilds/ledmatrix-plugins) for detailed installation instructions.
|
||||||
|
|||||||
@@ -55,7 +55,7 @@ in `DisplayManager` (`src/display_manager.py`, ~lines 270–295).
|
|||||||
| `chain_length` | int, `2` |
|
| `chain_length` | int, `2` |
|
||||||
| `parallel` | int, `1` |
|
| `parallel` | int, `1` |
|
||||||
| `brightness` | int, `90` |
|
| `brightness` | int, `90` |
|
||||||
| `hardware_mapping` | string, `"adafruit-hat"` |
|
| `hardware_mapping` | string, `"adafruit-hat"` (code default `"adafruit-hat-pwm"`) |
|
||||||
| `scan_mode` | int, `0` |
|
| `scan_mode` | int, `0` |
|
||||||
| `pwm_bits` | int, `9` (code default 10) |
|
| `pwm_bits` | int, `9` (code default 10) |
|
||||||
| `pwm_dither_bits` | int, `1` |
|
| `pwm_dither_bits` | int, `1` |
|
||||||
|
|||||||
@@ -333,7 +333,7 @@ python3 scripts/setup_plugin_repos.py
|
|||||||
python3 scripts/audit_plugins.py
|
python3 scripts/audit_plugins.py
|
||||||
|
|
||||||
# Validate a single plugin
|
# Validate a single plugin
|
||||||
python3 scripts/check_plugin.py <plugin-id>
|
python3 scripts/check_plugin.py --plugin <plugin-id>
|
||||||
```
|
```
|
||||||
|
|
||||||
Registry regeneration (`update_registry.py`) lives in the
|
Registry regeneration (`update_registry.py`) lives in the
|
||||||
|
|||||||
Regular → Executable
Regular → Executable
Regular → Executable
Regular → Executable
@@ -44,7 +44,7 @@ def install_via_apt(package_name: str) -> Tuple[bool, str]:
|
|||||||
apt_package_map = {
|
apt_package_map = {
|
||||||
'flask': 'python3-flask',
|
'flask': 'python3-flask',
|
||||||
'PIL': 'python3-pil',
|
'PIL': 'python3-pil',
|
||||||
'freetype': 'python3-freetype',
|
'freetype-py': 'python3-freetype',
|
||||||
'psutil': 'python3-psutil',
|
'psutil': 'python3-psutil',
|
||||||
'werkzeug': 'python3-werkzeug',
|
'werkzeug': 'python3-werkzeug',
|
||||||
'numpy': 'python3-numpy',
|
'numpy': 'python3-numpy',
|
||||||
@@ -95,7 +95,9 @@ def install_via_pip(package_name: str) -> Tuple[bool, str]:
|
|||||||
|
|
||||||
|
|
||||||
# Distribution (pip/apt) names whose importable module name differs.
|
# Distribution (pip/apt) names whose importable module name differs.
|
||||||
IMPORT_NAME_MAP = {}
|
IMPORT_NAME_MAP = {
|
||||||
|
'freetype-py': 'freetype',
|
||||||
|
}
|
||||||
|
|
||||||
|
|
||||||
def check_package_installed(package_name: str) -> bool:
|
def check_package_installed(package_name: str) -> bool:
|
||||||
@@ -139,7 +141,7 @@ def main():
|
|||||||
required_packages = [
|
required_packages = [
|
||||||
'flask',
|
'flask',
|
||||||
'PIL',
|
'PIL',
|
||||||
'freetype',
|
'freetype-py',
|
||||||
'psutil',
|
'psutil',
|
||||||
'werkzeug',
|
'werkzeug',
|
||||||
'numpy',
|
'numpy',
|
||||||
|
|||||||
@@ -6,20 +6,23 @@ links inside fenced code blocks.
|
|||||||
"""
|
"""
|
||||||
import re
|
import re
|
||||||
from pathlib import Path
|
from pathlib import Path
|
||||||
|
from typing import Iterator
|
||||||
|
|
||||||
PROJECT_ROOT = Path(__file__).resolve().parent.parent
|
PROJECT_ROOT = Path(__file__).resolve().parent.parent
|
||||||
LINK_RE = re.compile(r'\[[^\]]*\]\(([^)\s]+)\)')
|
LINK_RE = re.compile(r'\[[^\]]*\]\(([^)\s]+)\)')
|
||||||
FENCE_RE = re.compile(r'^(```|~~~)')
|
FENCE_RE = re.compile(r'^(```|~~~)')
|
||||||
|
|
||||||
|
|
||||||
def _md_files():
|
def _md_files() -> Iterator[Path]:
|
||||||
|
"""Yield active markdown files (repo root + docs/, excluding docs/archive/)."""
|
||||||
yield from PROJECT_ROOT.glob('*.md')
|
yield from PROJECT_ROOT.glob('*.md')
|
||||||
for path in PROJECT_ROOT.glob('docs/**/*.md'):
|
for path in PROJECT_ROOT.glob('docs/**/*.md'):
|
||||||
if 'archive' not in path.parts:
|
if 'archive' not in path.parts:
|
||||||
yield path
|
yield path
|
||||||
|
|
||||||
|
|
||||||
def test_relative_markdown_links_resolve():
|
def test_relative_markdown_links_resolve() -> None:
|
||||||
|
"""Every relative markdown link outside code fences must resolve on disk."""
|
||||||
broken = []
|
broken = []
|
||||||
for md in _md_files():
|
for md in _md_files():
|
||||||
in_fence = False
|
in_fence = False
|
||||||
|
|||||||
@@ -6,28 +6,32 @@ templates were removed when those displays became plugins).
|
|||||||
"""
|
"""
|
||||||
import re
|
import re
|
||||||
from pathlib import Path
|
from pathlib import Path
|
||||||
|
from typing import Iterator
|
||||||
|
|
||||||
PROJECT_ROOT = Path(__file__).resolve().parent.parent
|
PROJECT_ROOT = Path(__file__).resolve().parent.parent
|
||||||
TEMPLATE_ROOT = PROJECT_ROOT / 'web_interface' / 'templates'
|
TEMPLATE_ROOT = PROJECT_ROOT / 'web_interface' / 'templates'
|
||||||
RENDER_RE = re.compile(r"""render_template\(\s*['"]([^'"]+)['"]""")
|
RENDER_RE = re.compile(r"""render_template\(\s*['"]([^'"]+)['"]""")
|
||||||
|
|
||||||
|
|
||||||
def _python_sources():
|
def _python_sources() -> Iterator[Path]:
|
||||||
|
"""Yield every Python source that can call render_template()."""
|
||||||
yield PROJECT_ROOT / 'web_interface' / 'app.py'
|
yield PROJECT_ROOT / 'web_interface' / 'app.py'
|
||||||
yield from (PROJECT_ROOT / 'web_interface' / 'blueprints').glob('*.py')
|
yield from (PROJECT_ROOT / 'web_interface' / 'blueprints').glob('*.py')
|
||||||
|
|
||||||
|
|
||||||
def test_all_literal_render_template_targets_exist():
|
def test_all_literal_render_template_targets_exist() -> None:
|
||||||
|
"""Every string-literal render_template() target must exist on disk."""
|
||||||
missing = []
|
missing = []
|
||||||
for source in _python_sources():
|
for source in _python_sources():
|
||||||
text = source.read_text(encoding='utf-8')
|
text = source.read_text(encoding='utf-8')
|
||||||
for lineno, line in enumerate(text.splitlines(), 1):
|
for match in RENDER_RE.finditer(text):
|
||||||
for target in RENDER_RE.findall(line):
|
target = match.group(1)
|
||||||
if not (TEMPLATE_ROOT / target).is_file():
|
if not (TEMPLATE_ROOT / target).is_file():
|
||||||
|
lineno = text.count('\n', 0, match.start()) + 1
|
||||||
missing.append(
|
missing.append(
|
||||||
f'{source.relative_to(PROJECT_ROOT)}:{lineno} -> {target}'
|
f'{source.relative_to(PROJECT_ROOT)}:{lineno} -> {target}'
|
||||||
)
|
)
|
||||||
assert not missing, (
|
assert not missing, (
|
||||||
'render_template() references templates that do not exist under '
|
'render_template() references templates that do not exist under '
|
||||||
f'web_interface/templates/:\n' + '\n'.join(missing)
|
'web_interface/templates/:\n' + '\n'.join(missing)
|
||||||
)
|
)
|
||||||
|
|||||||
+30
-12
@@ -6,12 +6,19 @@ config schema declares that widget (the field renders as an empty container
|
|||||||
that polls the registry forever). base.html's widget list is maintained by
|
that polls the registry forever). base.html's widget list is maintained by
|
||||||
hand, so this test keeps it honest.
|
hand, so this test keeps it honest.
|
||||||
"""
|
"""
|
||||||
|
import re
|
||||||
from pathlib import Path
|
from pathlib import Path
|
||||||
|
from typing import Set
|
||||||
|
|
||||||
PROJECT_ROOT = Path(__file__).resolve().parent.parent
|
PROJECT_ROOT = Path(__file__).resolve().parent.parent
|
||||||
WIDGETS_DIR = PROJECT_ROOT / 'web_interface' / 'static' / 'v3' / 'js' / 'widgets'
|
WIDGETS_DIR = PROJECT_ROOT / 'web_interface' / 'static' / 'v3' / 'js' / 'widgets'
|
||||||
BASE_HTML = PROJECT_ROOT / 'web_interface' / 'templates' / 'v3' / 'base.html'
|
BASE_HTML = PROJECT_ROOT / 'web_interface' / 'templates' / 'v3' / 'base.html'
|
||||||
|
|
||||||
|
# Matches url_for('static', filename='...') inside actual <script> tags.
|
||||||
|
SCRIPT_SRC_RE = re.compile(
|
||||||
|
r"""<script\s[^>]*src="\{\{\s*url_for\(\s*'static'\s*,\s*filename='([^']+)'\s*\)\s*\}\}[^"]*"""
|
||||||
|
)
|
||||||
|
|
||||||
# Files that must NOT be script-included, with the reason.
|
# Files that must NOT be script-included, with the reason.
|
||||||
ALLOWLIST = {
|
ALLOWLIST = {
|
||||||
# Documentation example (docs/widget-guide.md); registers the name
|
# Documentation example (docs/widget-guide.md); registers the name
|
||||||
@@ -20,14 +27,26 @@ ALLOWLIST = {
|
|||||||
}
|
}
|
||||||
|
|
||||||
|
|
||||||
def test_every_widget_script_is_included_in_base_html():
|
def _included_widget_scripts() -> Set[str]:
|
||||||
|
"""Return widget JS basenames referenced by real <script> tags in base.html."""
|
||||||
base_html = BASE_HTML.read_text(encoding='utf-8')
|
base_html = BASE_HTML.read_text(encoding='utf-8')
|
||||||
missing = []
|
return {
|
||||||
for js_file in sorted(WIDGETS_DIR.glob('*.js')):
|
Path(filename).name
|
||||||
if js_file.name in ALLOWLIST:
|
for filename in SCRIPT_SRC_RE.findall(base_html)
|
||||||
continue
|
if filename.startswith('v3/js/widgets/')
|
||||||
if f'v3/js/widgets/{js_file.name}' not in base_html:
|
}
|
||||||
missing.append(js_file.name)
|
|
||||||
|
|
||||||
|
def test_every_widget_script_is_included_in_base_html() -> None:
|
||||||
|
"""Every non-allowlisted widget file must be loaded by a <script> tag."""
|
||||||
|
assert WIDGETS_DIR.is_dir(), f'Widget directory missing: {WIDGETS_DIR}'
|
||||||
|
included = _included_widget_scripts()
|
||||||
|
assert included, 'No widget <script> tags found in base.html — regex or template drift?'
|
||||||
|
missing = [
|
||||||
|
js_file.name
|
||||||
|
for js_file in sorted(WIDGETS_DIR.glob('*.js'))
|
||||||
|
if js_file.name not in ALLOWLIST and js_file.name not in included
|
||||||
|
]
|
||||||
assert not missing, (
|
assert not missing, (
|
||||||
'Widget files exist but are never <script>-included in base.html '
|
'Widget files exist but are never <script>-included in base.html '
|
||||||
'(plugins declaring these widgets get blank config fields): '
|
'(plugins declaring these widgets get blank config fields): '
|
||||||
@@ -37,11 +56,10 @@ def test_every_widget_script_is_included_in_base_html():
|
|||||||
)
|
)
|
||||||
|
|
||||||
|
|
||||||
def test_allowlisted_widgets_are_not_included():
|
def test_allowlisted_widgets_are_not_included() -> None:
|
||||||
base_html = BASE_HTML.read_text(encoding='utf-8')
|
"""Allowlisted (must-not-load) widget files must stay out of base.html."""
|
||||||
wrongly_included = [
|
included = _included_widget_scripts()
|
||||||
name for name in ALLOWLIST if f'v3/js/widgets/{name}' in base_html
|
wrongly_included = [name for name in ALLOWLIST if name in included]
|
||||||
]
|
|
||||||
assert not wrongly_included, (
|
assert not wrongly_included, (
|
||||||
'Allowlisted (must-not-load) widget files are script-included in '
|
'Allowlisted (must-not-load) widget files are script-included in '
|
||||||
'base.html: ' + ', '.join(wrongly_included)
|
'base.html: ' + ', '.join(wrongly_included)
|
||||||
|
|||||||
@@ -1,36 +1,44 @@
|
|||||||
"""Tests for the web interface's in-memory cache helpers."""
|
"""Tests for the web interface's in-memory cache helpers."""
|
||||||
|
from typing import Iterator
|
||||||
|
|
||||||
import pytest
|
import pytest
|
||||||
|
|
||||||
from web_interface.cache import delete_cached, get_cached, invalidate_cache, set_cached
|
from web_interface.cache import delete_cached, get_cached, invalidate_cache, set_cached
|
||||||
|
|
||||||
|
|
||||||
@pytest.fixture(autouse=True)
|
@pytest.fixture(autouse=True)
|
||||||
def clean_cache():
|
def clean_cache() -> Iterator[None]:
|
||||||
|
"""Start and finish every test with an empty cache."""
|
||||||
invalidate_cache()
|
invalidate_cache()
|
||||||
yield
|
yield
|
||||||
invalidate_cache()
|
invalidate_cache()
|
||||||
|
|
||||||
|
|
||||||
def test_set_and_get():
|
def test_set_and_get() -> None:
|
||||||
|
"""A cached value is returned before its TTL expires."""
|
||||||
set_cached('key', 'value')
|
set_cached('key', 'value')
|
||||||
assert get_cached('key') == 'value'
|
assert get_cached('key') == 'value'
|
||||||
|
|
||||||
|
|
||||||
def test_get_missing_returns_none():
|
def test_get_missing_returns_none() -> None:
|
||||||
|
"""Reading an unknown key returns None."""
|
||||||
assert get_cached('missing') is None
|
assert get_cached('missing') is None
|
||||||
|
|
||||||
|
|
||||||
def test_delete_cached_removes_key():
|
def test_delete_cached_removes_key() -> None:
|
||||||
|
"""delete_cached removes exactly the named key."""
|
||||||
set_cached('fonts_catalog', ['a-font'])
|
set_cached('fonts_catalog', ['a-font'])
|
||||||
delete_cached('fonts_catalog')
|
delete_cached('fonts_catalog')
|
||||||
assert get_cached('fonts_catalog') is None
|
assert get_cached('fonts_catalog') is None
|
||||||
|
|
||||||
|
|
||||||
def test_delete_cached_missing_key_is_noop():
|
def test_delete_cached_missing_key_is_noop() -> None:
|
||||||
delete_cached('never-set') # must not raise
|
"""Deleting a key that was never set must not raise."""
|
||||||
|
delete_cached('never-set')
|
||||||
|
|
||||||
|
|
||||||
def test_invalidate_cache_pattern():
|
def test_invalidate_cache_pattern() -> None:
|
||||||
|
"""Pattern invalidation removes matching keys and keeps the rest."""
|
||||||
set_cached('fonts_catalog', 1)
|
set_cached('fonts_catalog', 1)
|
||||||
set_cached('plugins_list', 2)
|
set_cached('plugins_list', 2)
|
||||||
invalidate_cache('fonts')
|
invalidate_cache('fonts')
|
||||||
|
|||||||
@@ -838,6 +838,10 @@ def save_main_config():
|
|||||||
except (ValueError, TypeError):
|
except (ValueError, TypeError):
|
||||||
return jsonify({'status': 'error', 'message': f"Invalid multiplexing value '{data['multiplexing']}'. Must be an integer from 0 to 22."}), 400
|
return jsonify({'status': 'error', 'message': f"Invalid multiplexing value '{data['multiplexing']}'. Must be an integer from 0 to 22."}), 400
|
||||||
|
|
||||||
|
# Validate pixel_mapper_config (free-form mapper string, e.g. "U-mapper;Rotate:90")
|
||||||
|
if 'pixel_mapper_config' in data and not isinstance(data['pixel_mapper_config'], str):
|
||||||
|
return jsonify({'status': 'error', 'message': 'pixel_mapper_config must be a string (e.g. "U-mapper;Rotate:90" or empty)'}), 400
|
||||||
|
|
||||||
# Validate row_address_type
|
# Validate row_address_type
|
||||||
if 'row_address_type' in data:
|
if 'row_address_type' in data:
|
||||||
try:
|
try:
|
||||||
|
|||||||
Reference in New Issue
Block a user