mirror of
https://github.com/ChuckBuilds/LEDMatrix.git
synced 2026-04-11 21:33:00 +00:00
fix: address CodeRabbit review findings across plugin system
- plugin_manager.py: clear plugin_manifests/plugin_directories before update to prevent ghost entries for uninstalled plugins persisting across scans - state_reconciliation.py: remove 'enabled' key check that skipped legacy plugin configs, default to enabled=True matching PluginManager.load_plugin - app.py: add threading.Lock around reconciliation start guard to prevent race condition spawning duplicate threads; add -> None return annotation - store_manager.py: use resolved registry ID (alt_id) instead of original plugin_id when reinstalling during monorepo migration - base.html: check Response.ok in loadPluginsDirect fallback; trigger fallback on tab click when HTMX unavailable; remove active-tab check from 5-second timeout so content preloads regardless Skipped: api_v3.py secret redaction suggestion — the caller at line 2539 already tries schema-based mask_secret_fields() before falling back to _conservative_mask_config, making the suggested change redundant. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -136,11 +136,14 @@ class PluginManager:
|
|||||||
except (OSError, PermissionError) as e:
|
except (OSError, PermissionError) as e:
|
||||||
self.logger.error("Error scanning directory %s: %s", directory, e, exc_info=True)
|
self.logger.error("Error scanning directory %s: %s", directory, e, exc_info=True)
|
||||||
|
|
||||||
# Update shared state under lock
|
# Replace shared state under lock so uninstalled plugins don't linger
|
||||||
with self._discovery_lock:
|
with self._discovery_lock:
|
||||||
|
self.plugin_manifests.clear()
|
||||||
self.plugin_manifests.update(new_manifests)
|
self.plugin_manifests.update(new_manifests)
|
||||||
if not hasattr(self, 'plugin_directories'):
|
if not hasattr(self, 'plugin_directories'):
|
||||||
self.plugin_directories = {}
|
self.plugin_directories = {}
|
||||||
|
else:
|
||||||
|
self.plugin_directories.clear()
|
||||||
self.plugin_directories.update(new_directories)
|
self.plugin_directories.update(new_directories)
|
||||||
|
|
||||||
return plugin_ids
|
return plugin_ids
|
||||||
|
|||||||
@@ -182,10 +182,8 @@ class StateReconciliation:
|
|||||||
continue
|
continue
|
||||||
if plugin_id in self._SYSTEM_CONFIG_KEYS:
|
if plugin_id in self._SYSTEM_CONFIG_KEYS:
|
||||||
continue
|
continue
|
||||||
if 'enabled' not in plugin_config:
|
|
||||||
continue
|
|
||||||
state[plugin_id] = {
|
state[plugin_id] = {
|
||||||
'enabled': plugin_config.get('enabled', False),
|
'enabled': plugin_config.get('enabled', True),
|
||||||
'version': plugin_config.get('version'),
|
'version': plugin_config.get('version'),
|
||||||
'exists_in_config': True
|
'exists_in_config': True
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -1785,11 +1785,13 @@ class PluginStoreManager:
|
|||||||
self.fetch_registry(force_refresh=True)
|
self.fetch_registry(force_refresh=True)
|
||||||
plugin_info_remote = self.get_plugin_info(plugin_id, fetch_latest_from_github=True, force_refresh=True)
|
plugin_info_remote = self.get_plugin_info(plugin_id, fetch_latest_from_github=True, force_refresh=True)
|
||||||
# Try without 'ledmatrix-' prefix (monorepo migration)
|
# Try without 'ledmatrix-' prefix (monorepo migration)
|
||||||
|
resolved_id = plugin_id
|
||||||
if not plugin_info_remote and plugin_id.startswith('ledmatrix-'):
|
if not plugin_info_remote and plugin_id.startswith('ledmatrix-'):
|
||||||
alt_id = plugin_id[len('ledmatrix-'):]
|
alt_id = plugin_id[len('ledmatrix-'):]
|
||||||
plugin_info_remote = self.get_plugin_info(alt_id, fetch_latest_from_github=True, force_refresh=True)
|
plugin_info_remote = self.get_plugin_info(alt_id, fetch_latest_from_github=True, force_refresh=True)
|
||||||
if plugin_info_remote:
|
if plugin_info_remote:
|
||||||
self.logger.info(f"Plugin {plugin_id} found in registry as {alt_id}")
|
resolved_id = alt_id
|
||||||
|
self.logger.info(f"Plugin {plugin_id} found in registry as {resolved_id}")
|
||||||
remote_branch = None
|
remote_branch = None
|
||||||
remote_sha = None
|
remote_sha = None
|
||||||
|
|
||||||
@@ -1804,13 +1806,13 @@ class PluginStoreManager:
|
|||||||
local_remote = git_info.get('remote_url', '')
|
local_remote = git_info.get('remote_url', '')
|
||||||
if local_remote and registry_repo and self._normalize_repo_url(local_remote) != self._normalize_repo_url(registry_repo):
|
if local_remote and registry_repo and self._normalize_repo_url(local_remote) != self._normalize_repo_url(registry_repo):
|
||||||
self.logger.info(
|
self.logger.info(
|
||||||
f"Plugin {plugin_id} git remote ({local_remote}) differs from registry ({registry_repo}). "
|
f"Plugin {resolved_id} git remote ({local_remote}) differs from registry ({registry_repo}). "
|
||||||
f"Reinstalling from registry to migrate to new source."
|
f"Reinstalling from registry to migrate to new source."
|
||||||
)
|
)
|
||||||
if not self._safe_remove_directory(plugin_path):
|
if not self._safe_remove_directory(plugin_path):
|
||||||
self.logger.error(f"Failed to remove old plugin directory for {plugin_id}")
|
self.logger.error(f"Failed to remove old plugin directory for {resolved_id}")
|
||||||
return False
|
return False
|
||||||
return self.install_plugin(plugin_id)
|
return self.install_plugin(resolved_id)
|
||||||
|
|
||||||
# Check if already up to date
|
# Check if already up to date
|
||||||
if remote_sha and local_sha and remote_sha.startswith(local_sha):
|
if remote_sha and local_sha and remote_sha.startswith(local_sha):
|
||||||
|
|||||||
@@ -653,8 +653,10 @@ def _initialize_health_monitor():
|
|||||||
|
|
||||||
_reconciliation_done = False
|
_reconciliation_done = False
|
||||||
_reconciliation_started = False
|
_reconciliation_started = False
|
||||||
|
import threading as _threading
|
||||||
|
_reconciliation_lock = _threading.Lock()
|
||||||
|
|
||||||
def _run_startup_reconciliation():
|
def _run_startup_reconciliation() -> None:
|
||||||
"""Run state reconciliation in background to auto-repair missing plugins."""
|
"""Run state reconciliation in background to auto-repair missing plugins."""
|
||||||
global _reconciliation_done, _reconciliation_started
|
global _reconciliation_done, _reconciliation_started
|
||||||
from src.logging_config import get_logger
|
from src.logging_config import get_logger
|
||||||
@@ -678,10 +680,12 @@ def _run_startup_reconciliation():
|
|||||||
_reconciliation_done = True
|
_reconciliation_done = True
|
||||||
else:
|
else:
|
||||||
_logger.warning("[Reconciliation] Finished with unresolved issues, will retry")
|
_logger.warning("[Reconciliation] Finished with unresolved issues, will retry")
|
||||||
_reconciliation_started = False
|
with _reconciliation_lock:
|
||||||
|
_reconciliation_started = False
|
||||||
except Exception as e:
|
except Exception as e:
|
||||||
_logger.error("[Reconciliation] Error: %s", e, exc_info=True)
|
_logger.error("[Reconciliation] Error: %s", e, exc_info=True)
|
||||||
_reconciliation_started = False
|
with _reconciliation_lock:
|
||||||
|
_reconciliation_started = False
|
||||||
|
|
||||||
# Initialize health monitor and run reconciliation on first request
|
# Initialize health monitor and run reconciliation on first request
|
||||||
@app.before_request
|
@app.before_request
|
||||||
@@ -690,10 +694,10 @@ def check_health_monitor():
|
|||||||
global _reconciliation_started
|
global _reconciliation_started
|
||||||
if not _health_monitor_initialized:
|
if not _health_monitor_initialized:
|
||||||
_initialize_health_monitor()
|
_initialize_health_monitor()
|
||||||
if not _reconciliation_started:
|
with _reconciliation_lock:
|
||||||
_reconciliation_started = True
|
if not _reconciliation_started:
|
||||||
import threading
|
_reconciliation_started = True
|
||||||
threading.Thread(target=_run_startup_reconciliation, daemon=True).start()
|
_threading.Thread(target=_run_startup_reconciliation, daemon=True).start()
|
||||||
|
|
||||||
if __name__ == '__main__':
|
if __name__ == '__main__':
|
||||||
app.run(host='0.0.0.0', port=5000, debug=True)
|
app.run(host='0.0.0.0', port=5000, debug=True)
|
||||||
|
|||||||
@@ -402,7 +402,10 @@
|
|||||||
content.setAttribute('data-loaded', 'true');
|
content.setAttribute('data-loaded', 'true');
|
||||||
console.log('Loading plugins directly via fetch (HTMX fallback)...');
|
console.log('Loading plugins directly via fetch (HTMX fallback)...');
|
||||||
fetch('/v3/partials/plugins')
|
fetch('/v3/partials/plugins')
|
||||||
.then(r => r.text())
|
.then(r => {
|
||||||
|
if (!r.ok) throw new Error(r.status + ' ' + r.statusText);
|
||||||
|
return r.text();
|
||||||
|
})
|
||||||
.then(html => {
|
.then(html => {
|
||||||
content.innerHTML = html;
|
content.innerHTML = html;
|
||||||
// Trigger full initialization chain
|
// Trigger full initialization chain
|
||||||
@@ -426,13 +429,9 @@
|
|||||||
setTimeout(() => {
|
setTimeout(() => {
|
||||||
if (typeof htmx === 'undefined') {
|
if (typeof htmx === 'undefined') {
|
||||||
console.warn('HTMX not loaded after 5 seconds, using direct fetch for plugins');
|
console.warn('HTMX not loaded after 5 seconds, using direct fetch for plugins');
|
||||||
const appElement = document.querySelector('[x-data="app()"]');
|
// Load plugins tab content directly regardless of active tab,
|
||||||
if (appElement && appElement._x_dataStack && appElement._x_dataStack[0]) {
|
// so it's ready when the user navigates to it
|
||||||
const activeTab = appElement._x_dataStack[0].activeTab;
|
loadPluginsDirect();
|
||||||
if (activeTab === 'plugins') {
|
|
||||||
loadPluginsDirect();
|
|
||||||
}
|
|
||||||
}
|
|
||||||
}
|
}
|
||||||
}, 5000);
|
}, 5000);
|
||||||
</script>
|
</script>
|
||||||
@@ -976,7 +975,7 @@
|
|||||||
<!-- Second row - Plugin tabs (populated dynamically) -->
|
<!-- Second row - Plugin tabs (populated dynamically) -->
|
||||||
<div id="plugin-tabs-row" class="border-b border-gray-200">
|
<div id="plugin-tabs-row" class="border-b border-gray-200">
|
||||||
<nav class="-mb-px flex flex-wrap gap-y-2 gap-x-2 lg:gap-x-3 xl:gap-x-4">
|
<nav class="-mb-px flex flex-wrap gap-y-2 gap-x-2 lg:gap-x-3 xl:gap-x-4">
|
||||||
<button @click="activeTab = 'plugins'"
|
<button @click="activeTab = 'plugins'; if (typeof htmx === 'undefined') { $nextTick(() => loadPluginsDirect()); }"
|
||||||
:class="activeTab === 'plugins' ? 'nav-tab-active' : ''"
|
:class="activeTab === 'plugins' ? 'nav-tab-active' : ''"
|
||||||
class="nav-tab">
|
class="nav-tab">
|
||||||
<i class="fas fa-plug"></i>Plugin Manager
|
<i class="fas fa-plug"></i>Plugin Manager
|
||||||
|
|||||||
Reference in New Issue
Block a user