fix(update-banner): address review findings — lock, returncode checks, update_available logic, a11y, button state

- Add _update_check_lock (threading.Lock) around all reads/writes to
  _update_check_cache in check_for_update() and git_pull, preventing
  races on concurrent requests
- Validate returncode for git fetch, rev-parse HEAD, and rev-parse
  origin/main; raise RuntimeError on failure so errors are caught and
  returned as error payloads instead of silently producing stale/empty SHAs
- Set update_available = commits_behind > 0 (was unconditionally True
  when local_sha != remote_sha); prevents false positive when local is
  ahead of remote
- Add type="button" and aria-label="Dismiss update" to the icon-only
  dismiss button
- Restore btn.innerHTML and btn.disabled in both success and error paths
  of applyUpdate(); only hide the banner and clear sessionStorage when
  data.status === 'success'

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
Chuck
2026-04-29 19:15:52 -04:00
parent 3ad331efce
commit 96edce3a3c
2 changed files with 39 additions and 19 deletions

View File

@@ -8,6 +8,7 @@ import time
import hashlib import hashlib
import uuid import uuid
import logging import logging
import threading
from datetime import datetime from datetime import datetime
from pathlib import Path from pathlib import Path
from typing import Optional, Tuple, Dict, Any, Type from typing import Optional, Tuple, Dict, Any, Type
@@ -1343,29 +1344,40 @@ def get_system_version():
_update_check_cache: Dict = {} _update_check_cache: Dict = {}
_UPDATE_CHECK_TTL = 300 # 5 minutes _UPDATE_CHECK_TTL = 300 # 5 minutes
_update_check_lock = threading.Lock()
@api_v3.route('/system/check-update', methods=['GET']) @api_v3.route('/system/check-update', methods=['GET'])
def check_for_update(): def check_for_update():
"""Check if a newer version is available on the remote.""" """Check if a newer version is available on the remote."""
import time as _time now = time.time()
now = _time.time() with _update_check_lock:
if _update_check_cache.get('ts', 0) + _UPDATE_CHECK_TTL > now: if _update_check_cache.get('ts', 0) + _UPDATE_CHECK_TTL > now:
return jsonify(_update_check_cache['data']) return jsonify(_update_check_cache['data'])
project_dir = str(PROJECT_ROOT) project_dir = str(PROJECT_ROOT)
try: try:
subprocess.run( fetch_result = subprocess.run(
['git', 'fetch', 'origin', 'main'], ['git', 'fetch', 'origin', 'main'],
capture_output=True, text=True, timeout=15, cwd=project_dir capture_output=True, text=True, timeout=15, cwd=project_dir
) )
local_sha = subprocess.run( if fetch_result.returncode != 0:
raise RuntimeError(f"git fetch failed: {fetch_result.stderr.strip()}")
local_result = subprocess.run(
['git', 'rev-parse', 'HEAD'], ['git', 'rev-parse', 'HEAD'],
capture_output=True, text=True, timeout=5, cwd=project_dir capture_output=True, text=True, timeout=5, cwd=project_dir
).stdout.strip() )
remote_sha = subprocess.run( if local_result.returncode != 0:
raise RuntimeError(f"git rev-parse HEAD failed: {local_result.stderr.strip()}")
local_sha = local_result.stdout.strip()
remote_result = subprocess.run(
['git', 'rev-parse', 'origin/main'], ['git', 'rev-parse', 'origin/main'],
capture_output=True, text=True, timeout=5, cwd=project_dir capture_output=True, text=True, timeout=5, cwd=project_dir
).stdout.strip() )
if remote_result.returncode != 0:
raise RuntimeError(f"git rev-parse origin/main failed: {remote_result.stderr.strip()}")
remote_sha = remote_result.stdout.strip()
if local_sha == remote_sha: if local_sha == remote_sha:
data = {'status': 'success', 'update_available': False, data = {'status': 'success', 'update_available': False,
@@ -1376,18 +1388,20 @@ def check_for_update():
capture_output=True, text=True, timeout=5, cwd=project_dir capture_output=True, text=True, timeout=5, cwd=project_dir
) )
lines = [l for l in log_result.stdout.strip().split('\n') if l] lines = [l for l in log_result.stdout.strip().split('\n') if l]
commits_behind = len(lines)
data = { data = {
'status': 'success', 'status': 'success',
'update_available': True, 'update_available': commits_behind > 0,
'local_sha': local_sha[:8], 'local_sha': local_sha[:8],
'remote_sha': remote_sha[:8], 'remote_sha': remote_sha[:8],
'commits_behind': len(lines), 'commits_behind': commits_behind,
'latest_message': lines[0].split(' ', 1)[1] if lines else '', 'latest_message': lines[0].split(' ', 1)[1] if lines else '',
} }
except Exception as e: except Exception as e:
logger.warning("[System] check-update failed: %s", e) logger.warning("[System] check-update failed: %s", e)
data = {'status': 'error', 'update_available': False, 'message': str(e)} data = {'status': 'error', 'update_available': False, 'message': str(e)}
with _update_check_lock:
_update_check_cache['ts'] = now _update_check_cache['ts'] = now
_update_check_cache['data'] = data _update_check_cache['data'] = data
return jsonify(data) return jsonify(data)
@@ -1502,6 +1516,7 @@ def execute_system_action():
) )
# Invalidate update-check cache so the banner hides immediately # Invalidate update-check cache so the banner hides immediately
with _update_check_lock:
_update_check_cache.clear() _update_check_cache.clear()
# Return custom response for git_pull # Return custom response for git_pull

View File

@@ -948,9 +948,9 @@
update-banner-action transition-colors duration-150"> update-banner-action transition-colors duration-150">
<i class="fas fa-download mr-1"></i> Update Now <i class="fas fa-download mr-1"></i> Update Now
</button> </button>
<button onclick="dismissUpdateBanner()" <button type="button" onclick="dismissUpdateBanner()"
class="update-banner-dismiss rounded p-1 transition-colors duration-150" class="update-banner-dismiss rounded p-1 transition-colors duration-150"
title="Dismiss"> title="Dismiss" aria-label="Dismiss update">
<i class="fas fa-times text-sm"></i> <i class="fas fa-times text-sm"></i>
</button> </button>
</div> </div>
@@ -4947,6 +4947,7 @@
window.applyUpdate = function() { window.applyUpdate = function() {
var btn = document.getElementById('update-banner-btn'); var btn = document.getElementById('update-banner-btn');
var originalHTML = '<i class="fas fa-download mr-1"></i> Update Now';
btn.innerHTML = '<i class="fas fa-spinner fa-spin mr-1"></i> Updating...'; btn.innerHTML = '<i class="fas fa-spinner fa-spin mr-1"></i> Updating...';
btn.disabled = true; btn.disabled = true;
fetch('/api/v3/system/action', { fetch('/api/v3/system/action', {
@@ -4956,14 +4957,18 @@
}) })
.then(function(r) { return r.json(); }) .then(function(r) { return r.json(); })
.then(function(data) { .then(function(data) {
btn.innerHTML = originalHTML;
btn.disabled = false;
if (data.status === 'success') {
document.getElementById('update-banner').style.display = 'none'; document.getElementById('update-banner').style.display = 'none';
try { sessionStorage.removeItem('update-dismissed'); } catch(e) {}
}
if (typeof showNotification === 'function') { if (typeof showNotification === 'function') {
showNotification(data.message || 'Update complete', data.status || 'success'); showNotification(data.message || 'Update complete', data.status || 'success');
} }
try { sessionStorage.removeItem('update-dismissed'); } catch(e) {}
}) })
.catch(function() { .catch(function() {
btn.innerHTML = '<i class="fas fa-download mr-1"></i> Update Now'; btn.innerHTML = originalHTML;
btn.disabled = false; btn.disabled = false;
if (typeof showNotification === 'function') { if (typeof showNotification === 'function') {
showNotification('Update failed — check your connection', 'error'); showNotification('Update failed — check your connection', 'error');