fix(workspace): align Workspace Todos rendering with sidebar

2026-07-16 12:40:18 +00:00 · 2026-06-29 08:05:49 +10:00
parent b01ff5a9b5
commit dd658d7a04
4 changed files with 172 additions and 45 deletions
@@ -3159,7 +3159,7 @@ function loadTodos() {

  if (!todos.length) {
    if (typeof _todosLastRenderedHash !== 'undefined' && _todosLastRenderedHash === '__empty__') return;
-    panel.innerHTML = `<div style="color:var(--muted);font-size:12px;padding:4px 0">${esc(t('todos_no_active'))}</div>`;
+    panel.innerHTML = renderTodoEmptyState();
    if (typeof _todosLastRenderedHash !== 'undefined') _todosLastRenderedHash = '__empty__';
    return;
  }
@@ -3170,18 +3170,9 @@ function loadTodos() {
    _todosLastRenderedHash = hash;
  }

-  const statusIcon = {pending:li('square',14), in_progress:li('loader',14), completed:li('check',14), cancelled:li('x',14)};
-  const statusColor = {pending:'var(--muted)', in_progress:'var(--blue)', completed:'rgba(100,200,100,.8)', cancelled:'rgba(200,100,100,.5)'};
  // Single innerHTML join is the cheapest correct way to materialize
  // ~10–50 leaf nodes.  All user-controlled content goes through esc().
-  panel.innerHTML = todos.map(td => `
-    <div style="display:flex;align-items:flex-start;gap:10px;padding:6px 0;border-bottom:1px solid var(--border);">
-      <span style="font-size:14px;display:inline-flex;align-items:center;flex-shrink:0;margin-top:1px;color:${statusColor[td.status]||'var(--muted)'}">${statusIcon[td.status]||li('square',14)}</span>
-      <div style="flex:1;min-width:0">
-        <div style="font-size:13px;color:${td.status==='completed'?'var(--muted)':td.status==='in_progress'?'var(--text)':'var(--text)'};${td.status==='completed'?'text-decoration:line-through;opacity:.5':''};line-height:1.4">${esc(td.content)}</div>
-        <div style="font-size:10px;color:var(--muted);margin-top:2px;opacity:.6">${esc(td.id)} · ${esc(td.status)}</div>
-      </div>
-    </div>`).join('');
+  panel.innerHTML = renderTodoRows(todos, {metadata:true});
 }

 // Legacy fallback: reverse-scan settled tool messages for the most
@@ -7215,7 +7215,7 @@ function clearInflightState(sid){
 //      `S.session = ...` settle point so cross-session navigation never
 //      leaves a stale list visible.
 //
-// The hash is keyed on (id, content, status); the render itself uses
+// The hash is keyed on (id, content/text, status); the render itself uses
 // `esc()` for any user-controlled string, so XSS surface is the same as
 // any other innerHTML path in this file.
 let _todosLastRenderedHash=null;
@@ -7232,11 +7232,73 @@ function _todosHash(items){
  let h=items.length+'|';
  for(let i=0;i<items.length;i++){
    const t=items[i]||{};
-    h+=String(t.id==null?'':t.id)+'\x1f'+String(t.content==null?'':t.content)+'\x1f'+String(t.status==null?'':t.status)+'\x1e';
+    const content=t.content==null?(t.text==null?'':t.text):t.content;
+    h+=String(t.id==null?'':t.id)+'\x1f'+String(content)+'\x1f'+String(t.status==null?'':t.status)+'\x1e';
  }
  return h;
 }

+const TODO_STATUS_RENDERING=Object.freeze({
+  pending:Object.freeze({icon:'square',color:'var(--muted)'}),
+  in_progress:Object.freeze({icon:'loader',color:'var(--blue)'}),
+  completed:Object.freeze({icon:'check',color:'rgba(100,200,100,.8)'}),
+  cancelled:Object.freeze({icon:'x',color:'rgba(200,100,100,.5)'}),
+});
+
+function todoStatusKey(status){
+  const key=String(status||'pending');
+  return Object.prototype.hasOwnProperty.call(TODO_STATUS_RENDERING,key)?key:'pending';
+}
+
+function todoStatusVisual(status){
+  const key=todoStatusKey(status);
+  return TODO_STATUS_RENDERING[key];
+}
+
+function renderTodoStatusIcon(status,size=14){
+  const visual=todoStatusVisual(status);
+  return typeof li==='function'?li(visual.icon,size):'';
+}
+
+function todoContent(todo){
+  if(!todo) return '';
+  return todo.content==null?(todo.text==null?'':todo.text):todo.content;
+}
+
+function renderTodoEmptyState(options={}){
+  const centered=!!(options&&options.centered);
+  const style=centered
+    ? 'padding:24px 12px;text-align:center;color:var(--muted);font-size:12px'
+    : 'color:var(--muted);font-size:12px;padding:4px 0';
+  return `<div style="${style}">${esc(t('todos_no_active'))}</div>`;
+}
+
+function renderTodoRow(todo,options={}){
+  const td=todo||{};
+  const status=todoStatusKey(td.status);
+  const visual=todoStatusVisual(status);
+  const showMetadata=!(options&&options.metadata===false);
+  const isCompleted=status==='completed';
+  const contentColor=isCompleted?'var(--muted)':'var(--text)';
+  const completedStyle=isCompleted?'text-decoration:line-through;opacity:.5':'';
+  const metadata=showMetadata
+    ? `<div style="font-size:10px;color:var(--muted);margin-top:2px;opacity:.6">${esc(td.id)} · ${esc(status)}</div>`
+    : '';
+  return `
+    <div style="display:flex;align-items:flex-start;gap:10px;padding:6px 0;border-bottom:1px solid var(--border);">
+      <span style="font-size:14px;display:inline-flex;align-items:center;flex-shrink:0;margin-top:1px;color:${visual.color}">${renderTodoStatusIcon(status,14)}</span>
+      <div style="flex:1;min-width:0">
+        <div style="font-size:13px;color:${contentColor};${completedStyle};line-height:1.4">${esc(todoContent(td))}</div>
+        ${metadata}
+      </div>
+    </div>`;
+}
+
+function renderTodoRows(todos,options={}){
+  const items=Array.isArray(todos)?todos:[];
+  return items.map(td=>renderTodoRow(td,options)).join('');
+}
+
 function _todosPanelIsActive(){
  if(typeof document==='undefined') return false;
  const panel=document.getElementById('panelTodos');
@@ -378,30 +378,10 @@ function _loadWorkspacePanelTodos(){
    }
  }catch(e){ todos = []; }
  if(!todos.length){
-    if(_workspaceTodosLastRenderedHash === '__empty__') return;
-    panel.innerHTML = '<div style="padding:24px 12px;text-align:center;color:var(--muted);font-size:12px">No active tasks</div>';
-    _workspaceTodosLastRenderedHash = '__empty__';
+    panel.innerHTML = renderTodoEmptyState({centered:true});
    return;
  }
-  const hash = _workspaceTodosHash(todos);
-  if(hash === _workspaceTodosLastRenderedHash) return;
-  _workspaceTodosLastRenderedHash = hash;
-  const statusIcon = (s) => {
-    if(s === 'completed') return '<svg width="14" height="14" viewBox="0 0 24 24" fill="none" stroke="var(--text)" stroke-width="2.5" stroke-linecap="round" stroke-linejoin="round" aria-hidden="true"><polyline points="20 6 9 17 4 12"/></svg>';
-    if(s === 'in_progress') return '<svg width="14" height="14" viewBox="0 0 24 24" fill="none" stroke="var(--blue)" stroke-width="2.5" stroke-linecap="round" stroke-linejoin="round" aria-hidden="true"><circle cx="12" cy="12" r="10"/><polyline points="12 6 12 12 16 14"/></svg>';
-    if(s === 'cancelled') return '<svg width="14" height="14" viewBox="0 0 24 24" fill="none" stroke="var(--muted)" stroke-width="2.5" stroke-linecap="round" stroke-linejoin="round" aria-hidden="true"><line x1="18" y1="6" x2="6" y2="18"/><line x1="6" y1="6" x2="18" y2="18"/></svg>';
-    // pending
-    return '<svg width="14" height="14" viewBox="0 0 24 24" fill="none" stroke="var(--muted)" stroke-width="2.5" stroke-linecap="round" stroke-linejoin="round" aria-hidden="true"><circle cx="12" cy="12" r="10"/></svg>';
-  };
-  const items = todos.map(t => {
-    const s = t.status || 'pending';
-    const isDone = s === 'completed' || s === 'cancelled';
-    return `<div style="display:flex;align-items:flex-start;gap:8px;padding:6px 0;border-bottom:1px solid var(--border)">`+
-      `<span style="flex-shrink:0;margin-top:2px">${statusIcon(s)}</span>`+
-      `<span style="font-size:12px;color:${isDone?'var(--muted)':'var(--text)'};text-decoration:${s==='cancelled'?'line-through':'none'}">${_escHtml(t.content||t.text||'')}</span>`+
-      `</div>`;
-  }).join('');
-  panel.innerHTML = `<div style="padding:4px 0">${items}</div>`;
+  panel.innerHTML = renderTodoRows(todos, {metadata:true});
 }

 function _escHtml(s){
@@ -1,5 +1,9 @@
+import shutil
+import subprocess
 from pathlib import Path

+import pytest
+

 REPO_ROOT = Path(__file__).parent.parent

@@ -39,7 +43,7 @@ def test_legacy_todos_fallback_still_uses_raw_session_messages():
 def test_workspace_todos_tab_prefers_live_sse_snapshot_before_cold_load_sidecar():
    src = (REPO_ROOT / "static" / "workspace.js").read_text(encoding="utf-8")
    start = src.find("function _loadWorkspacePanelTodos()")
-    end = src.find("function _escHtml", start)
+    end = src.find("const ARTIFACT_IGNORE_RE", start)

    assert start != -1
    assert end != -1
@@ -54,17 +58,107 @@ def test_workspace_todos_tab_prefers_live_sse_snapshot_before_cold_load_sidecar(
    )


-def test_workspace_todos_render_uses_separate_hash_cache():
+def test_todo_panels_delegate_rendering_to_shared_helpers():
+    ui = (REPO_ROOT / "static" / "ui.js").read_text(encoding="utf-8")
+    panels = (REPO_ROOT / "static" / "panels.js").read_text(encoding="utf-8")
+    workspace = (REPO_ROOT / "static" / "workspace.js").read_text(encoding="utf-8")
+
+    assert "const TODO_STATUS_RENDERING=Object.freeze({" in ui
+    assert "function renderTodoStatusIcon(status,size=14)" in ui
+    assert "function renderTodoRow(todo,options={})" in ui
+    assert "function renderTodoRows(todos,options={})" in ui
+    assert "function renderTodoEmptyState(options={})" in ui
+
+    left_start = panels.find("function loadTodos()")
+    left_end = panels.find("function _legacyTodosFromMessages()", left_start)
+    workspace_start = workspace.find("function _loadWorkspacePanelTodos()")
+    workspace_end = workspace.find("const ARTIFACT_IGNORE_RE", workspace_start)
+
+    assert left_start != -1 and left_end != -1
+    assert workspace_start != -1 and workspace_end != -1
+    left_block = panels[left_start:left_end]
+    workspace_block = workspace[workspace_start:workspace_end]
+
+    assert "renderTodoEmptyState()" in left_block
+    assert "renderTodoRows(todos, {metadata:true})" in left_block
+    assert "renderTodoEmptyState({centered:true})" in workspace_block
+    assert "renderTodoRows(todos, {metadata:true})" in workspace_block
+
+
+def test_workspace_todos_no_longer_defines_local_icon_or_empty_text_mapping():
    src = (REPO_ROOT / "static" / "workspace.js").read_text(encoding="utf-8")
    start = src.find("function _loadWorkspacePanelTodos()")
-    end = src.find("function _escHtml", start)
+    end = src.find("const ARTIFACT_IGNORE_RE", start)

-    assert "let _workspaceTodosLastRenderedHash = null;" in src
-    assert "function _resetWorkspaceTodosRenderCache()" in src
-    assert start != -1
-    assert end != -1
+    assert start != -1 and end != -1
    helper = src[start:end]

-    assert "_workspaceTodosLastRenderedHash === '__empty__'" in helper
-    assert "_workspaceTodosHash(todos)" in helper
-    assert "if(hash === _workspaceTodosLastRenderedHash) return;" in helper
+    assert "No active tasks" not in helper
+    assert "const statusIcon" not in helper
+    assert "<svg" not in helper
+    assert "renderTodoEmptyState({centered:true})" in helper
+
+
+def test_workspace_files_and_artifacts_paths_stay_outside_todo_render_change():
+    src = (REPO_ROOT / "static" / "workspace.js").read_text(encoding="utf-8")
+    todos_start = src.find("function _loadWorkspacePanelTodos()")
+    artifacts_start = src.find("const ARTIFACT_IGNORE_RE")
+
+    assert todos_start != -1
+    assert artifacts_start != -1
+    assert "renderSessionArtifacts()" in src[:todos_start]
+    assert "const ARTIFACT_MUTATION_TOOLS = new Set" in src[artifacts_start:]
+    assert "function _normalizeArtifactPath(path)" in src[artifacts_start:]
+
+
+@pytest.mark.skipif(shutil.which("node") is None, reason="node is required for shared todo renderer behavior test")
+def test_shared_todo_renderer_outputs_consistent_status_markup(tmp_path):
+    ui = (REPO_ROOT / "static" / "ui.js").read_text(encoding="utf-8")
+    start = ui.find("const TODO_STATUS_RENDERING=Object.freeze({")
+    end = ui.find("function _todosPanelIsActive()", start)
+
+    assert start != -1
+    assert end != -1
+    helper = ui[start:end]
+    script = f"""
+const helper = {helper!r};
+const liCalls = [];
+function li(name, size) {{
+  liCalls.push([name, size]);
+  return `<svg data-icon="${{name}}" data-size="${{size}}"></svg>`;
+}}
+function esc(value) {{
+  return String(value ?? '').replace(/[&<>"']/g, c => ({{'&':'&amp;','<':'&lt;','>':'&gt;','"':'&quot;',"'":'&#39;'}}[c]));
+}}
+function t(key) {{ return key === 'todos_no_active' ? 'No active task list in this session.' : key; }}
+const api = new Function('li', 'esc', 't', helper + '; return {{TODO_STATUS_RENDERING, todoStatusKey, renderTodoRow, renderTodoRows, renderTodoEmptyState}};')(li, esc, t);
+function assert(cond, msg) {{ if(!cond) throw new Error(msg); }}
+const expected = {{
+  pending: ['square', 'var(--muted)'],
+  in_progress: ['loader', 'var(--blue)'],
+  completed: ['check', 'rgba(100,200,100,.8)'],
+  cancelled: ['x', 'rgba(200,100,100,.5)'],
+}};
+for (const [status, [icon, color]] of Object.entries(expected)) {{
+  assert(api.TODO_STATUS_RENDERING[status].icon === icon, status + ' icon mismatch');
+  assert(api.TODO_STATUS_RENDERING[status].color === color, status + ' color mismatch');
+  const row = api.renderTodoRow({{id: status + '-id', content: status + ' content', status}}, {{metadata: true}});
+  assert(row.includes(`data-icon="${{icon}}"`), status + ' row icon mismatch');
+  assert(row.includes(color), status + ' row color mismatch');
+  assert(row.includes(`${{status}}-id · ${{status}}`), status + ' metadata mismatch');
+}}
+assert(api.todoStatusKey('unknown') === 'pending', 'unknown statuses fall back to pending');
+assert(api.renderTodoRows([{{id:'a', content:'A', status:'pending'}}, {{id:'b', text:'B', status:'completed'}}], {{metadata:true}}).includes('b · completed'), 'shared rows include metadata');
+assert(api.renderTodoEmptyState({{centered:true}}).includes('No active task list in this session.'), 'empty state uses i18n text');
+"""
+    script_path = tmp_path / "shared_todo_renderer_test.js"
+    script_path.write_text(script, encoding="utf-8")
+    result = subprocess.run(
+        ["node", str(script_path)],
+        cwd=REPO_ROOT,
+        capture_output=True,
+        text=True,
+        check=False,
+    )
+
+    assert result.returncode == 0, result.stderr or result.stdout