diff --git a/api/routes.py b/api/routes.py index 765857ca..70a25fcc 100644 --- a/api/routes.py +++ b/api/routes.py @@ -3718,6 +3718,8 @@ def handle_get(handler, parsed) -> bool: "current_sha": "abc1234", "latest_sha": "def5678", "branch": "master", + "repo_url": "https://github.com/nesquena/hermes-webui", + "compare_url": "https://github.com/nesquena/hermes-webui/compare/abc1234...def5678", }, "agent": { "name": "agent", @@ -3725,6 +3727,8 @@ def handle_get(handler, parsed) -> bool: "current_sha": "aaa0001", "latest_sha": "bbb0002", "branch": "master", + "repo_url": "https://github.com/NousResearch/hermes-agent", + "compare_url": "https://github.com/NousResearch/hermes-agent/compare/aaa0001...bbb0002", }, "checked_at": 0, }, diff --git a/api/updates.py b/api/updates.py index e3e025c2..c6b619cb 100644 --- a/api/updates.py +++ b/api/updates.py @@ -12,6 +12,7 @@ import subprocess import threading import time from pathlib import Path +from urllib.parse import urlparse from api.config import REPO_ROOT, STREAMS, STREAMS_LOCK @@ -169,6 +170,16 @@ def _normalize_remote_url(remote_url): return remote_url.rstrip('/') +def _build_compare_url(repo_url, current_sha, latest_sha): + """Return a safe browser compare URL, or None when any piece is missing.""" + if not (repo_url and current_sha and latest_sha): + return None + parsed = urlparse(repo_url) + if parsed.scheme not in ('http', 'https') or not parsed.netloc: + return None + return f"{repo_url}/compare/{current_sha}...{latest_sha}" + + def _split_remote_ref(ref): """Split 'origin/branch-name' into ('origin', 'branch-name'). @@ -262,6 +273,7 @@ def _check_repo(path, name): 'latest_sha': latest, 'branch': compare_ref, 'repo_url': remote_url, + 'compare_url': _build_compare_url(remote_url, current, latest), } diff --git a/docs/images/update-banner-whats-new-after.png b/docs/images/update-banner-whats-new-after.png new file mode 100644 index 00000000..7b0469f9 Binary files /dev/null and b/docs/images/update-banner-whats-new-after.png differ diff --git a/docs/images/update-banner-whats-new-before.png b/docs/images/update-banner-whats-new-before.png new file mode 100644 index 00000000..1940cfce Binary files /dev/null and b/docs/images/update-banner-whats-new-before.png differ diff --git a/static/index.html b/static/index.html index 8a530d16..6f8b22a6 100644 --- a/static/index.html +++ b/static/index.html @@ -342,7 +342,7 @@
- +
diff --git a/static/panels.js b/static/panels.js index d942fac0..62b91aab 100644 --- a/static/panels.js +++ b/static/panels.js @@ -5996,6 +5996,7 @@ async function checkUpdatesNow(){ if(typeof _showUpdateBanner==='function') _showUpdateBanner(data); } else { if(status){status.textContent=t('settings_up_to_date');status.style.color='var(--success)';} + if(typeof _showUpdateBanner==='function') _showUpdateBanner(data); } } } catch(e){ diff --git a/static/ui.js b/static/ui.js index d0f27aa8..076ec07b 100644 --- a/static/ui.js +++ b/static/ui.js @@ -3878,38 +3878,91 @@ function _formatUpdateTargetStatus(label,info){ const branch=info.branch?` (${info.branch})`:''; return `${label}${branch}: ${info.behind} update${info.behind>1?'s':''}`; } +function _isSafeUpdateCompareUrl(url){ + if(!url||!/^https?:\/\//i.test(url)) return false; + try{ + const parsed=new URL(url); + return parsed.protocol==='https:'||parsed.protocol==='http:'; + }catch(e){ + return false; + } +} +function _updateCompareUrl(info){ + if(!info) return null; + const compareUrl=info.compare_url||null; + if(compareUrl) return _isSafeUpdateCompareUrl(compareUrl)?compareUrl:null; + const repo_url=info.repo_url; + const currentSha=info.current_sha; + const latestSha=info.latest_sha; + if(!(repo_url&¤tSha&&latestSha)) return null; + const fallbackUrl=repo_url+'/compare/'+currentSha+'...'+latestSha; + return _isSafeUpdateCompareUrl(fallbackUrl)?fallbackUrl:null; +} +function _updateWhatsNewTargets(data){ + const targets=[ + {key:'webui',label:'WebUI',info:data&&data.webui}, + {key:'agent',label:'Agent',info:data&&data.agent}, + ]; + return targets.map((target)=>({ + key:target.key, + label:target.label, + info:target.info, + url:_updateCompareUrl(target.info), + })).filter((target)=>target.info&&target.info.behind>0&&target.url); +} +function _renderUpdateWhatsNewLinks(data){ + const container=$('updateWhatsNewLinks'); + if(!container) return; + container.replaceChildren(); + const targets=_updateWhatsNewTargets(data); + if(!targets.length){ + container.style.display='none'; + return; + } + container.style.display='block'; + if(targets.length===1){ + const target=targets[0]; + const link=document.createElement('a'); + link.href=target.url; + link.target='_blank'; + link.rel='noopener'; + link.style.color='var(--accent)'; + link.style.textDecoration='underline'; + link.textContent="What's new in "+target.label+'?'; + container.appendChild(link); + return; + } + container.appendChild(document.createTextNode("What's new: ")); + targets.forEach((target,idx)=>{ + if(idx>0) container.appendChild(document.createTextNode(' \u00b7 ')); + const link=document.createElement('a'); + link.href=target.url; + link.target='_blank'; + link.rel='noopener'; + link.style.color='var(--accent)'; + link.style.textDecoration='underline'; + link.textContent=target.label; + container.appendChild(link); + }); +} function _showUpdateBanner(data){ const parts=[]; const webuiPart=_formatUpdateTargetStatus('WebUI',data.webui); const agentPart=_formatUpdateTargetStatus('Agent',data.agent); if(webuiPart) parts.push(webuiPart); if(agentPart) parts.push(agentPart); - if(!parts.length)return; + window._updateData=data; + if(!parts.length){ + _renderUpdateWhatsNewLinks(data); + const staleBanner=$('updateBanner'); + if(staleBanner) staleBanner.classList.remove('visible'); + return; + } const msg=$('updateMsg'); if(msg) msg.textContent='\u2B06 '+parts.join(', ')+' available'; const banner=$('updateBanner'); if(banner) banner.classList.add('visible'); - window._updateData=data; - // Wire up "What's new?" link. - // - // Reset display:none + clear the href on every render — otherwise a stale - // link from a prior update banner can stay visible after we've moved past - // a state where the new payload no longer carries usable SHAs (#1579 case - // when the local HEAD diverges from upstream and the compare URL would 404). - const link=$('updateWhatsNew'); - if(link){ - link.style.display='none'; - link.removeAttribute('href'); - if(data.webui){ - const repoUrl=data.webui.repo_url; - const curSha=data.webui.current_sha; - const newSha=data.webui.latest_sha; - if(repoUrl && curSha && newSha){ - link.href=repoUrl+'/compare/'+curSha+'...'+newSha; - link.style.display='inline'; - } - } - } + _renderUpdateWhatsNewLinks(data); } function dismissUpdate(){ const b=$('updateBanner');if(b)b.classList.remove('visible'); diff --git a/tests/test_issue1579_whats_new_link_404.py b/tests/test_issue1579_whats_new_link_404.py index 6f8bb7db..9b851ac2 100644 --- a/tests/test_issue1579_whats_new_link_404.py +++ b/tests/test_issue1579_whats_new_link_404.py @@ -187,39 +187,35 @@ def _read_ui_js(): return (REPO_ROOT / 'static' / 'ui.js').read_text(encoding='utf-8') -def test_whats_new_link_resets_display_and_href_on_every_render(): +def test_whats_new_link_resets_display_and_contents_on_every_render(): """Without reset, a stale link from a prior banner can stay visible after a re-render where the new payload has current_sha=None. """ src = _read_ui_js() - # Find the "What's new" wiring block (~50-line window) - idx = src.find("Wire up \"What's new?\" link") - assert idx != -1, "What's-new link wiring block not found" - block = src[idx:idx + 800] + idx = src.find("function _renderUpdateWhatsNewLinks(data)") + assert idx != -1, "What's-new link renderer not found" + block = src[idx:idx + 1200] - # Reset must happen BEFORE the conditional href set - reset_idx = block.find("style.display='none'") - set_idx = block.find("style.display='inline'") - href_clear_idx = block.find("removeAttribute('href')") - href_set_idx = block.find("link.href=repoUrl") + clear_idx = block.find("container.replaceChildren()") + hide_idx = block.find("container.style.display='none'") + show_idx = block.find("container.style.display='block'") - assert reset_idx != -1, "Missing display='none' reset on every render" - assert href_clear_idx != -1, "Missing removeAttribute('href') reset on every render" - assert reset_idx < set_idx, "display reset must precede inline set" - assert href_clear_idx < href_set_idx, "href clear must precede href assignment" + assert clear_idx != -1, "Missing container contents reset on every render" + assert hide_idx != -1, "Missing display='none' reset when no safe links exist" + assert clear_idx < show_idx, "contents reset must precede link rendering" + assert hide_idx < show_idx, "hidden state must be handled before visible rendering" -def test_whats_new_link_suppressed_when_curSha_falsy(): - """The conditional must guard on all three of repoUrl/curSha/newSha.""" +def test_whats_new_link_suppressed_when_current_sha_falsy(): + """The legacy fallback must guard on all three of repo_url/current_sha/latest_sha.""" src = _read_ui_js() - idx = src.find("Wire up \"What's new?\" link") - block = src[idx:idx + 800] - # Match "if(repoUrl && curSha && newSha)" with arbitrary whitespace - pattern = re.compile(r'if\s*\(\s*repoUrl\s*&&\s*curSha\s*&&\s*newSha\s*\)') - assert pattern.search(block), ( - "Link must require all three of repoUrl, curSha, newSha to be truthy. " - "If any is null/empty, link stays display:none." - ) + idx = src.find("function _updateCompareUrl(info)") + assert idx != -1, "Compare URL helper not found" + block = src[idx:idx + 500] + compact = re.sub(r"\s+", "", block) + assert "if(!(repo_url&¤tSha&&latestSha))returnnull;" in compact + assert "constfallbackUrl=repo_url+'/compare/'+currentSha+'...'+latestSha;" in compact + assert "return_isSafeUpdateCompareUrl(fallbackUrl)?fallbackUrl:null;" in compact # ── 3. End-to-end: simulate the exact reporter URL shape ── diff --git a/tests/test_update_banner_fixes.py b/tests/test_update_banner_fixes.py index 5eaee61e..274b7af4 100644 --- a/tests/test_update_banner_fixes.py +++ b/tests/test_update_banner_fixes.py @@ -29,6 +29,81 @@ def read(rel): # ── api/updates.py ──────────────────────────────────────────────────────────── class TestUpdateChecker: + def test_build_compare_url_requires_all_pieces(self): + import api.updates as upd + + assert upd._build_compare_url( + 'https://github.com/nesquena/hermes-webui', 'abc1234', 'def5678' + ) == 'https://github.com/nesquena/hermes-webui/compare/abc1234...def5678' + assert upd._build_compare_url(None, 'abc1234', 'def5678') is None + assert upd._build_compare_url('https://github.com/nesquena/hermes-webui', None, 'def5678') is None + assert upd._build_compare_url('https://github.com/nesquena/hermes-webui', 'abc1234', None) is None + + def test_build_compare_url_rejects_unsafe_remote_urls(self): + import api.updates as upd + + assert upd._build_compare_url('javascript:alert(1)', 'abc1234', 'def5678') is None + assert upd._build_compare_url('file:///tmp/hermes-webui', 'abc1234', 'def5678') is None + assert upd._build_compare_url('https:github.com/nesquena/hermes-webui', 'abc1234', 'def5678') is None + assert upd._build_compare_url('https://github.com/nesquena/hermes-webui', 'abc1234', 'def5678') + + def test_check_repo_includes_compare_url_from_normalized_remote_and_merge_base(self, tmp_path, monkeypatch): + import api.updates as upd + + (tmp_path / '.git').mkdir() + + def fake_run(args, cwd, timeout=10): + if args[0] == 'fetch': + return '', True + if args[:2] == ['rev-parse', '--abbrev-ref']: + return 'origin/master', True + if args[:2] == ['rev-list', '--count']: + return '2', True + if args[0] == 'merge-base': + return 'abcdef1234567890', True + if args[:3] == ['rev-parse', '--short', 'abcdef1234567890']: + return 'abcdef1', True + if args[:3] == ['rev-parse', '--short', 'origin/master']: + return 'def5678', True + if args[:2] == ['remote', 'get-url']: + return 'git@github.com:NousResearch/hermes-agent.git', True + return '', True + + monkeypatch.setattr(upd, '_run_git', fake_run) + result = upd._check_repo(tmp_path, 'agent') + + assert result['repo_url'] == 'https://github.com/NousResearch/hermes-agent' + assert result['current_sha'] == 'abcdef1' + assert result['latest_sha'] == 'def5678' + assert result['compare_url'] == 'https://github.com/NousResearch/hermes-agent/compare/abcdef1...def5678' + + def test_check_repo_omits_compare_url_when_merge_base_missing(self, tmp_path, monkeypatch): + import api.updates as upd + + (tmp_path / '.git').mkdir() + + def fake_run(args, cwd, timeout=10): + if args[0] == 'fetch': + return '', True + if args[:2] == ['rev-parse', '--abbrev-ref']: + return 'origin/master', True + if args[:2] == ['rev-list', '--count']: + return '2', True + if args[0] == 'merge-base': + return 'fatal: no merge base', False + if args[:3] == ['rev-parse', '--short', 'origin/master']: + return 'def5678', True + if args[:2] == ['remote', 'get-url']: + return 'https://github.com/nesquena/hermes-webui.git', True + return '', True + + monkeypatch.setattr(upd, '_run_git', fake_run) + result = upd._check_repo(tmp_path, 'webui') + + assert result['current_sha'] is None + assert result['latest_sha'] == 'def5678' + assert result['compare_url'] is None + def test_repo_url_strips_only_dot_git_suffix(self, tmp_path, monkeypatch): import api.updates as upd @@ -603,6 +678,53 @@ class TestSequentialUpdateRestartCoordination: ) + +class TestUpdateCompareSource: + def test_simulated_update_check_payload_includes_both_safe_compare_urls(self): + src = read('api/routes.py') + assert '"repo_url": "https://github.com/nesquena/hermes-webui"' in src + assert '"compare_url": "https://github.com/nesquena/hermes-webui/compare/abc1234...def5678"' in src + assert '"repo_url": "https://github.com/NousResearch/hermes-agent"' in src + assert '"compare_url": "https://github.com/NousResearch/hermes-agent/compare/aaa0001...bbb0002"' in src + + def test_update_banner_html_uses_multi_target_links_container(self): + src = read('static/index.html') + assert 'id="updateWhatsNewLinks"' in src + assert 'id="updateWhatsNew"' not in src + + def test_update_banner_frontend_uses_data_driven_compare_helpers(self): + src = read('static/ui.js') + assert 'function _isSafeUpdateCompareUrl(url)' in src + assert 'function _updateCompareUrl(info)' in src + assert 'function _updateWhatsNewTargets(data)' in src + assert 'function _renderUpdateWhatsNewLinks(data)' in src + assert "$('updateWhatsNewLinks')" in src + assert "compare_url" in src + assert "repo_url+'/compare/'+currentSha+'...'+latestSha" in src + assert "_isSafeUpdateCompareUrl(compareUrl)?compareUrl:null" in src + assert "_renderUpdateWhatsNewLinks(data);" in src + assert "data.webui.repo_url" not in src + assert "$('updateWhatsNew')" not in src + + def test_update_banner_clears_stale_links_when_no_updates_remain(self): + src = read('static/ui.js') + start = src.find('function _showUpdateBanner(data)') + assert start != -1, "_showUpdateBanner not found" + fn = src[start:src.find('function dismissUpdate()', start)] + empty_idx = fn.find('if(!parts.length)') + assert empty_idx != -1, "_showUpdateBanner must handle empty update payloads" + empty_block = fn[empty_idx:fn.find('return;', empty_idx) + len('return;')] + assert '_renderUpdateWhatsNewLinks(data);' in empty_block + assert "classList.remove('visible')" in empty_block + + def test_manual_up_to_date_check_clears_update_banner(self): + src = read('static/panels.js') + up_to_date_idx = src.find("settings_up_to_date") + assert up_to_date_idx != -1, "manual update up-to-date branch not found" + block = src[up_to_date_idx:up_to_date_idx + 300] + assert "_showUpdateBanner(data)" in block + + # ── Regression: force button reset on retry ────────────────────────────────── class TestForceButtonResetOnRetry: