Skip to content

feat(web): update-available banner in web UI #311

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.

Sign up for GitHub

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

Open
ChuckBuilds wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat(web): update-available banner in web UI #311

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 54 additions & 0 deletions web_interface/blueprints/api_v3.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1341,6 +1341,57 @@ def get_system_version():
logger.exception("[System] get_system_version failed")
return jsonify({'status': 'error', 'message': 'Failed to get system version'}), 500

_update_check_cache: Dict = {}
_UPDATE_CHECK_TTL = 300 # 5 minutes
Comment on lines +1344 to +1345
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Synchronize the update cache.

This cache is shared across requests, but ts and data are read/written independently. A second request can observe ts before data is set, or hit git_pull's clear() between the freshness check and the read, which makes check_for_update() intermittently raise KeyError or serve stale state.

🔒 Suggested fix 
+import threading
+
 _update_check_cache: Dict = {}
+_update_check_lock = threading.Lock()
 _UPDATE_CHECK_TTL = 300  # 5 minutes
@@
     import time as _time
     now = _time.time()
-    if _update_check_cache.get('ts', 0) + _UPDATE_CHECK_TTL > now:
-        return jsonify(_update_check_cache['data'])
+    with _update_check_lock:
+        cached = _update_check_cache.copy()
+    if cached.get('ts', 0) + _UPDATE_CHECK_TTL > now and 'data' in cached:
+        return jsonify(cached['data'])
@@
-    _update_check_cache['ts'] = now
-    _update_check_cache['data'] = data
+    with _update_check_lock:
+        _update_check_cache.clear()
+        _update_check_cache.update({'ts': now, 'data': data})
     return jsonify(data)
@@
-            _update_check_cache.clear()
+            with _update_check_lock:
+                _update_check_cache.clear()

Also applies to: 1352-1353, 1391-1392, 1504-1505

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@web_interface/blueprints/api_v3.py` around lines 1344 - 1345, The shared
cache _update_check_cache is accessed concurrently causing race conditions;
protect all reads and writes (including clear() and checks using
_UPDATE_CHECK_TTL) by introducing a dedicated threading.Lock (e.g.,
_update_check_lock) and wrapping every access to _update_check_cache and its
keys in a with _update_check_lock block inside check_for_update(), git_pull(),
and any other functions that touch the cache so ts/data are updated and read
atomically.


@api_v3.route('/system/check-update', methods=['GET'])
def check_for_update():
"""Check if a newer version is available on the remote."""
import time as _time
now = _time.time()
if _update_check_cache.get('ts', 0) + _UPDATE_CHECK_TTL > now:
return jsonify(_update_check_cache['data'])

project_dir = str(PROJECT_ROOT)
try:
subprocess.run(
['git', 'fetch', 'origin', 'main'],
capture_output=True, text=True, timeout=15, cwd=project_dir
)
local_sha = subprocess.run(
['git', 'rev-parse', 'HEAD'],
capture_output=True, text=True, timeout=5, cwd=project_dir
).stdout.strip()
remote_sha = subprocess.run(
['git', 'rev-parse', 'origin/main'],
capture_output=True, text=True, timeout=5, cwd=project_dir
).stdout.strip()

if local_sha == remote_sha:
data = {'status': 'success', 'update_available': False,
'local_sha': local_sha[:8], 'remote_sha': remote_sha[:8]}
else:
log_result = subprocess.run(
['git', 'log', 'HEAD..origin/main', '--oneline'],
capture_output=True, text=True, timeout=5, cwd=project_dir
)
Comment on lines +1357 to +1377
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Check each git command result before building a success payload.

These subprocess calls only fail via non-zero returncode; they do not raise by default. Right now a failed fetch/rev-parse/log can still fall through as "status": "success" with empty or stale SHAs, and that bad payload gets cached for 5 minutes.

🛑 Suggested fix 
     project_dir = str(PROJECT_ROOT)
     try:
-        subprocess.run(
+        fetch_result = subprocess.run(
             ['git', 'fetch', 'origin', 'main'],
             capture_output=True, text=True, timeout=15, cwd=project_dir
         )
-        local_sha = subprocess.run(
+        if fetch_result.returncode != 0:
+            raise RuntimeError(fetch_result.stderr.strip() or 'git fetch origin main failed')
+
+        local_result = subprocess.run(
             ['git', 'rev-parse', 'HEAD'],
             capture_output=True, text=True, timeout=5, cwd=project_dir
-        ).stdout.strip()
-        remote_sha = subprocess.run(
+        )
+        if local_result.returncode != 0:
+            raise RuntimeError(local_result.stderr.strip() or 'git rev-parse HEAD failed')
+        local_sha = local_result.stdout.strip()
+
+        remote_result = subprocess.run(
             ['git', 'rev-parse', 'origin/main'],
             capture_output=True, text=True, timeout=5, cwd=project_dir
-        ).stdout.strip()
+        )
+        if remote_result.returncode != 0:
+            raise RuntimeError(remote_result.stderr.strip() or 'git rev-parse origin/main failed')
+        remote_sha = remote_result.stdout.strip()

As per coding guidelines, "Validate inputs and handle errors early (Fail Fast principle)".

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
subprocess.run(
['git', 'fetch', 'origin', 'main'],
capture_output=True, text=True, timeout=15, cwd=project_dir
)
local_sha = subprocess.run(
['git', 'rev-parse', 'HEAD'],
capture_output=True, text=True, timeout=5, cwd=project_dir
).stdout.strip()
remote_sha = subprocess.run(
['git', 'rev-parse', 'origin/main'],
capture_output=True, text=True, timeout=5, cwd=project_dir
).stdout.strip()
if local_sha == remote_sha:
data = {'status': 'success', 'update_available': False,
'local_sha': local_sha[:8], 'remote_sha': remote_sha[:8]}
else:
log_result = subprocess.run(
['git', 'log', 'HEAD..origin/main', '--oneline'],
capture_output=True, text=True, timeout=5, cwd=project_dir
)
fetch_result = subprocess.run(
['git', 'fetch', 'origin', 'main'],
capture_output=True, text=True, timeout=15, cwd=project_dir
)
if fetch_result.returncode != 0:
raise RuntimeError(fetch_result.stderr.strip() or 'git fetch origin main failed')
local_result = subprocess.run(
['git', 'rev-parse', 'HEAD'],
capture_output=True, text=True, timeout=5, cwd=project_dir
)
if local_result.returncode != 0:
raise RuntimeError(local_result.stderr.strip() or 'git rev-parse HEAD failed')
local_sha = local_result.stdout.strip()
remote_result = subprocess.run(
['git', 'rev-parse', 'origin/main'],
capture_output=True, text=True, timeout=5, cwd=project_dir
)
if remote_result.returncode != 0:
raise RuntimeError(remote_result.stderr.strip() or 'git rev-parse origin/main failed')
remote_sha = remote_result.stdout.strip()
if local_sha == remote_sha:
data = {'status': 'success', 'update_available': False,
'local_sha': local_sha[:8], 'remote_sha': remote_sha[:8]}
else:
log_result = subprocess.run(
['git', 'log', 'HEAD..origin/main', '--oneline'],
capture_output=True, text=True, timeout=5, cwd=project_dir
)
🧰 Tools
🪛 Ruff (0.15.10)

[error] 1358-1358: Starting a process with a partial executable path

(S607)

[error] 1362-1362: Starting a process with a partial executable path

(S607)

[error] 1366-1366: Starting a process with a partial executable path

(S607)

[error] 1375-1375: Starting a process with a partial executable path

(S607)

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@web_interface/blueprints/api_v3.py` around lines 1357 - 1377, The git
subprocess calls in the update-check block (the fetch, the two rev-parse calls
that populate local_sha/remote_sha, and the git log that assigns log_result)
must have their return codes validated before building a success payload; update
the code around subprocess.run calls in api_v3.py to either use check=True or
inspect each CompletedProcess.returncode and on failure log/process stderr (from
.stderr) and return an error payload (status: "error" or similar) instead of
treating empty/stale SHAs as success, ensuring you don't cache a success
response when any of fetch, rev-parse, or log failed; reference variables: the
subprocess.run for fetch, local_sha assignment, remote_sha assignment, and
log_result when adding these checks.

lines = [l for l in log_result.stdout.strip().split('\n') if l]
data = {
'status': 'success',
'update_available': True,
'local_sha': local_sha[:8],
'remote_sha': remote_sha[:8],
'commits_behind': len(lines),
'latest_message': lines[0].split(' ', 1)[1] if lines else '',
}
Comment on lines +1370 to +1386
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Only mark updates available when this checkout is actually behind origin/main.

local_sha != remote_sha is also true when the repo is ahead of origin/main or has diverged from it. In those cases this endpoint still reports update_available=True, which doesn't match the feature contract and will surface a false banner.

↕️ Suggested fix 
-        if local_sha == remote_sha:
+        counts_result = subprocess.run(
+            ['git', 'rev-list', '--left-right', '--count', 'HEAD...origin/main'],
+            capture_output=True, text=True, timeout=5, cwd=project_dir
+        )
+        ahead_count, behind_count = map(int, counts_result.stdout.strip().split())
+
+        if behind_count == 0:
             data = {'status': 'success', 'update_available': False,
                     'local_sha': local_sha[:8], 'remote_sha': remote_sha[:8]}
         else:
             log_result = subprocess.run(
                 ['git', 'log', 'HEAD..origin/main', '--oneline'],
                 capture_output=True, text=True, timeout=5, cwd=project_dir
             )
             lines = [l for l in log_result.stdout.strip().split('\n') if l]
             data = {
                 'status': 'success',
                 'update_available': True,
                 'local_sha': local_sha[:8],
                 'remote_sha': remote_sha[:8],
-                'commits_behind': len(lines),
+                'commits_behind': behind_count,
                 'latest_message': lines[0].split(' ', 1)[1] if lines else '',
             }
🧰 Tools
🪛 Ruff (0.15.10)

[error] 1375-1375: Starting a process with a partial executable path

(S607)

[error] 1378-1378: Ambiguous variable name: l

(E741)

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@web_interface/blueprints/api_v3.py` around lines 1370 - 1386, The current
logic treats any local_sha != remote_sha as an available update; change it to
detect whether origin/main is actually ahead of the checkout by running a git
compare and only set update_available=True when the remote has commits the local
lacks. For example, after obtaining local_sha and remote_sha (existing
variables), run a git rev-list --left-right --count local_sha...origin/main (or
use git rev-list local_sha..origin/main) in project_dir and parse the counts to
determine commits_behind (remote-only count); set update_available = True only
when commits_behind > 0 and populate latest_message/commits_behind from that
result (use the existing log_result parsing for commit messages when
commits_behind > 0).

except Exception as e:
logger.warning("[System] check-update failed: %s", e)
data = {'status': 'error', 'update_available': False, 'message': str(e)}

_update_check_cache['ts'] = now
_update_check_cache['data'] = data
return jsonify(data)

@api_v3.route('/system/action', methods=['POST'])
def execute_system_action():
"""Execute system actions (start/stop/reboot/etc)"""
Expand Down Expand Up @@ -1450,6 +1501,9 @@ def execute_system_action():
cwd=project_dir
)

# Invalidate update-check cache so the banner hides immediately
_update_check_cache.clear()

# Return custom response for git_pull
if result.returncode == 0:
pull_message = "Code updated successfully."
Expand Down
36 changes: 36 additions & 0 deletions web_interface/static/v3/app.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1004,3 +1004,39 @@ button.bg-white {
[data-theme="dark"] .theme-toggle-btn {
color: #fbbf24;
}

/* Update available banner */
.update-banner {
background-color: #eff6ff;
border-color: #bfdbfe;
color: #1e40af;
}
.update-banner-action {
background-color: #3b82f6;
color: #fff;
}
.update-banner-action:hover {
background-color: #2563eb;
}
.update-banner-dismiss {
color: #1e40af;
opacity: 0.6;
}
.update-banner-dismiss:hover {
opacity: 1;
}

[data-theme="dark"] .update-banner {
background-color: #1e293b;
border-color: #334155;
color: #93c5fd;
}
[data-theme="dark"] .update-banner-action {
background-color: #2563eb;
}
[data-theme="dark"] .update-banner-action:hover {
background-color: #3b82f6;
}
[data-theme="dark"] .update-banner-dismiss {
color: #93c5fd;
}
93 changes: 93 additions & 0 deletions web_interface/templates/v3/base.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -931,6 +931,33 @@ <h1 class="text-xl font-bold text-gray-900">
</div>
</header>

<!-- Update available banner -->
<div id="update-banner" style="display:none"
class="update-banner border-b transition-all duration-300 ease-in-out">
<div class="mx-auto px-4 sm:px-6 lg:px-8 xl:px-12 2xl:px-16 py-2" style="max-width:100%">
<div class="flex items-center justify-between">
<div class="flex items-center space-x-3">
<i class="fas fa-arrow-circle-up text-lg"></i>
<span class="text-sm font-medium" id="update-banner-text">
A new LEDMatrix update is available
</span>
</div>
<div class="flex items-center space-x-3">
<button onclick="applyUpdate()" id="update-banner-btn"
class="inline-flex items-center px-3 py-1 text-xs font-semibold rounded-md
update-banner-action transition-colors duration-150">
<i class="fas fa-download mr-1"></i> Update Now
</button>
<button onclick="dismissUpdateBanner()"
class="update-banner-dismiss rounded p-1 transition-colors duration-150"
title="Dismiss">
<i class="fas fa-times text-sm"></i>
</button>
Comment on lines +951 to +955
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Add an explicit accessible label to the icon-only dismiss button.

The close control is icon-only and currently relies on title. Please add aria-label (and preferably type="button" for safety if this ever moves inside a form).

♿ Suggested patch 
-                    <button onclick="dismissUpdateBanner()"
+                    <button type="button"
+                            onclick="dismissUpdateBanner()"
                             class="update-banner-dismiss rounded p-1 transition-colors duration-150"
-                            title="Dismiss">
+                            title="Dismiss"
+                            aria-label="Dismiss update banner">
                         <i class="fas fa-times text-sm"></i>
                     </button>
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@web_interface/templates/v3/base.html` around lines 951 - 955, The dismiss
button markup that invokes dismissUpdateBanner() and uses class
"update-banner-dismiss" is icon-only and needs explicit accessibility
attributes: add aria-label="Dismiss update" (or similar descriptive text) to the
<button> and also include type="button" to avoid default form-submit behavior if
moved inside a form; keep the existing title if desired for tooltip support.

</div>
</div>
</div>
</div>

<!-- Main content -->
<main class="mx-auto px-4 sm:px-6 lg:px-8 xl:px-12 2xl:px-16 py-8" style="max-width: 100%;">
<!-- Navigation tabs -->
Expand Down Expand Up @@ -4888,6 +4915,72 @@ <h3 id="on-demand-modal-title" class="text-lg font-semibold">Run Plugin On-Deman
</form>
</div>
</div>
<!-- Update banner logic -->
<script>
(function() {
var CHECK_INTERVAL = 30 * 60 * 1000; // 30 minutes
var _dismissed = false;

function checkForUpdate() {
fetch('/api/v3/system/check-update')
.then(function(r) { return r.json(); })
.then(function(data) {
if (data.update_available && !_dismissed) {
var n = data.commits_behind || 0;
var msg = 'A new LEDMatrix update is available';
if (n > 0) msg += ' (' + n + ' commit' + (n > 1 ? 's' : '') + ')';
document.getElementById('update-banner-text').textContent = msg;
document.getElementById('update-banner').style.display = '';
try { sessionStorage.setItem('update-sha', data.remote_sha); } catch(e) {}
} else {
document.getElementById('update-banner').style.display = 'none';
}
})
.catch(function() {});
}

window.dismissUpdateBanner = function() {
_dismissed = true;
document.getElementById('update-banner').style.display = 'none';
try { sessionStorage.setItem('update-dismissed', '1'); } catch(e) {}
};

window.applyUpdate = function() {
var btn = document.getElementById('update-banner-btn');
btn.innerHTML = '<i class="fas fa-spinner fa-spin mr-1"></i> Updating...';
btn.disabled = true;
fetch('/api/v3/system/action', {
method: 'POST',
headers: { 'Content-Type': 'application/json' },
body: JSON.stringify({ action: 'git_pull' })
})
.then(function(r) { return r.json(); })
.then(function(data) {
document.getElementById('update-banner').style.display = 'none';
if (typeof showNotification === 'function') {
showNotification(data.message || 'Update complete', data.status || 'success');
}
try { sessionStorage.removeItem('update-dismissed'); } catch(e) {}
})
.catch(function() {
btn.innerHTML = '<i class="fas fa-download mr-1"></i> Update Now';
btn.disabled = false;
if (typeof showNotification === 'function') {
showNotification('Update failed — check your connection', 'error');
}
});
};
Comment on lines +4948 to +4972
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Only hide the banner on successful update, and always restore button state.

applyUpdate() currently hides the banner for any JSON response and only re-enables the button in .catch(). If the API returns an error payload (or the banner reappears later), the button can stay stuck in “Updating…”.

🐛 Suggested patch 
     window.applyUpdate = function() {
         var btn = document.getElementById('update-banner-btn');
         btn.innerHTML = '<i class="fas fa-spinner fa-spin mr-1"></i> Updating...';
         btn.disabled = true;
         fetch('/api/v3/system/action', {
             method: 'POST',
             headers: { 'Content-Type': 'application/json' },
             body: JSON.stringify({ action: 'git_pull' })
         })
-        .then(function(r) { return r.json(); })
-        .then(function(data) {
-            document.getElementById('update-banner').style.display = 'none';
-            if (typeof showNotification === 'function') {
-                showNotification(data.message || 'Update complete', data.status || 'success');
-            }
-            try { sessionStorage.removeItem('update-dismissed'); } catch(e) {}
+        .then(function(r) {
+            return r.json().then(function(data) {
+                return { ok: r.ok, data: data };
+            });
+        })
+        .then(function(res) {
+            var data = res.data || {};
+            var isSuccess = res.ok && data.status === 'success';
+            if (isSuccess) {
+                document.getElementById('update-banner').style.display = 'none';
+                try { sessionStorage.removeItem('update-dismissed'); } catch(e) {}
+            }
+            if (typeof showNotification === 'function') {
+                showNotification(
+                    data.message || (isSuccess ? 'Update complete' : 'Update failed'),
+                    isSuccess ? 'success' : 'error'
+                );
+            }
         })
         .catch(function() {
-            btn.innerHTML = '<i class="fas fa-download mr-1"></i> Update Now';
-            btn.disabled = false;
             if (typeof showNotification === 'function') {
                 showNotification('Update failed — check your connection', 'error');
             }
+        })
+        .finally(function() {
+            btn.innerHTML = '<i class="fas fa-download mr-1"></i> Update Now';
+            btn.disabled = false;
         });
     };
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
window.applyUpdate = function() {
var btn = document.getElementById('update-banner-btn');
btn.innerHTML = '<i class="fas fa-spinner fa-spin mr-1"></i> Updating...';
btn.disabled = true;
fetch('/api/v3/system/action', {
method: 'POST',
headers: { 'Content-Type': 'application/json' },
body: JSON.stringify({ action: 'git_pull' })
})
.then(function(r) { return r.json(); })
.then(function(data) {
document.getElementById('update-banner').style.display = 'none';
if (typeof showNotification === 'function') {
showNotification(data.message || 'Update complete', data.status || 'success');
}
try { sessionStorage.removeItem('update-dismissed'); } catch(e) {}
})
.catch(function() {
btn.innerHTML = '<i class="fas fa-download mr-1"></i> Update Now';
btn.disabled = false;
if (typeof showNotification === 'function') {
showNotification('Update failed — check your connection', 'error');
}
});
};
window.applyUpdate = function() {
var btn = document.getElementById('update-banner-btn');
btn.innerHTML = '<i class="fas fa-spinner fa-spin mr-1"></i> Updating...';
btn.disabled = true;
fetch('/api/v3/system/action', {
method: 'POST',
headers: { 'Content-Type': 'application/json' },
body: JSON.stringify({ action: 'git_pull' })
})
.then(function(r) {
return r.json().then(function(data) {
return { ok: r.ok, data: data };
});
})
.then(function(res) {
var data = res.data || {};
var isSuccess = res.ok && data.status === 'success';
if (isSuccess) {
document.getElementById('update-banner').style.display = 'none';
try { sessionStorage.removeItem('update-dismissed'); } catch(e) {}
}
if (typeof showNotification === 'function') {
showNotification(
data.message || (isSuccess ? 'Update complete' : 'Update failed'),
isSuccess ? 'success' : 'error'
);
}
})
.catch(function() {
if (typeof showNotification === 'function') {
showNotification('Update failed — check your connection', 'error');
}
})
.finally(function() {
btn.innerHTML = '<i class="fas fa-download mr-1"></i> Update Now';
btn.disabled = false;
});
};
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@web_interface/templates/v3/base.html` around lines 4948 - 4972, applyUpdate
currently hides the `#update-banner` and removes sessionStorage regardless of
whether the API succeeded, and only restores the `#update-banner-btn` state in the
.catch branch, which can leave the button stuck; change applyUpdate to check the
real success condition (e.g., inspect fetch Response.ok before calling r.json()
or check data.status from the parsed JSON) and only hide `#update-banner` and
remove sessionStorage when the response indicates success, while ensuring the
button (element id update-banner-btn) is always restored to its original
innerHTML and disabled=false in both success and error paths (use a
finally-equivalent or duplicate the restore logic), and still call
showNotification with the message/status returned by the API.


// On load: skip if dismissed this session for the same SHA
try {
if (sessionStorage.getItem('update-dismissed') === '1') _dismissed = true;
} catch(e) {}

// Initial check shortly after page load, then periodic
setTimeout(checkForUpdate, 2000);
setInterval(checkForUpdate, CHECK_INTERVAL);
})();
</script>
</body>
</html>