Merge pull request 'Load comment actions only when a conversation opens' (#840) from timmy/839-lazy-comment-actions into main

frontend/conversation-action-hydrator.js

@@ -0,0 +1,49 @@
+function createConversationActionHydrator({ load, activate }) {
+  let actions = null;
+  let pending = null;
+  const wiredRoots = new WeakSet();
+
+  function ensure() {
+    if (actions) return Promise.resolve(actions);
+    if (!pending) {
+      pending = load().then(() => {
+        actions = activate();
+        return actions;
+      }).catch(error => {
+        pending = null;
+        throw error;
+      });
+    }
+    return pending;
+  }
+
+  function show({ root, state, paint, wire, retry }) {
+    if (actions) {
+      retry.hidden = true;
+      if (!wiredRoots.has(root)) {
+        wire(actions);
+        wiredRoots.add(root);
+      }
+      paint(state, actions);
+      return Promise.resolve(true);
+    }
+
+    paint(state, null);
+    retry.hidden = true;
+    return ensure().then(controller => {
+      if (!wiredRoots.has(root)) {
+        wire(controller);
+        wiredRoots.add(root);
+      }
+      paint(state, controller);
+      return true;
+    }).catch(() => {
+      retry.hidden = false;
+      return false;
+    });
+  }
+
+  return { show, ready: () => Boolean(actions) };
+}
+
+if (typeof module !== 'undefined' && module.exports) module.exports = createConversationActionHydrator;

frontend/dashboard.css

@@ -696,6 +696,7 @@ textarea { resize: vertical; min-height: 120px; }
 .pull-sheet-content { overflow-wrap:anywhere; white-space:pre-wrap; }
 .pull-file, .pull-comment-card { margin:8px 0; padding:10px; border:1px solid #203a5c; border-radius:10px; }
 .conversation-more { min-height:44px; width:100%; margin:8px 0; }
+.conversation-actions-retry { min-height:44px; max-width:100%; margin:8px 0; }
 .pull-file-toggle, .pull-review-file { min-height:44px; width:100%; }
 .pull-file-toggle { display:flex; justify-content:space-between; align-items:center; gap:8px; text-align:left; }
 .pull-review-file { margin-top:8px; }

frontend/dashboard.js

@@ -614,14 +614,16 @@
       'comment-actions': document.querySelector('meta[name="stackchain-feature-comment-actions"]')?.content || '',
     },
   });
-  await commentActionFeatures.run('comment-actions', {
+  const commentActionHydrator = createConversationActionHydrator({
-    status: qs('#my-work-action-status'), retryLabel:'Reload to retry comment actions.',
+    load: () => commentActionFeatures.load('comment-actions'),
-  }, () => {
+    activate: () => {
-    commentActions = createCommentActions({
+      commentActions = createCommentActions({
-      fetchJson: fetchReviewJson,
+        fetchJson: fetchReviewJson,
-      getLogin: () => confirmedOwnerLogin,
+        getLogin: () => confirmedOwnerLogin,
-      confirmDelete: message => window.confirm(message),
+        confirmDelete: message => window.confirm(message),
-    });
+      });
+      return commentActions;
+    },
   });
   const issueCaptureFeatures = createFeatureLoader({
     document,
@@ -3100,8 +3102,8 @@
     toggle?.focus();
   }
 
-  function renderIssueComment(comment) {
+  function renderIssueComment(comment, controller = commentActions) {
-    const actions = commentActions.actionHtml?.(comment) || '';
+    const actions = controller?.actionHtml?.(comment) || '';
     return '<div class="issue-comment" data-comment-id="' + Number(comment.id || 0) + '"><div class="small">' +
       escapeHtml(comment.author || 'Unknown author') +
       (comment.created_at ? ' · ' + escapeHtml(fmt(comment.created_at)) : '') +
@@ -3109,19 +3111,19 @@
       renderMarkdown(comment.body || 'No comment body provided.') + '</div></div>';
   }
 
-  function renderIssueConversation(state) {
+  function paintIssueConversation(state, controller) {
     const comments = state?.comments || [];
     qs('#issue-comments').innerHTML = comments.length ?
-      comments.map(renderIssueComment).join('') : '<div class="muted">No comments yet.</div>';
+      comments.map(comment => renderIssueComment(comment, controller)).join('') : '<div class="muted">No comments yet.</div>';
     qs('#load-older-issue-comments').hidden = !Number.isInteger(state?.older_page);
     qs('#issue-conversation-status').textContent = comments.length ?
       comments.length + ' of ' + Math.max(state.total || 0, comments.length) + ' messages loaded.' : 'No comments yet.';
   }
 
-  function renderUpdateConversation(state) {
+  function paintUpdateConversation(state, controller) {
     const comments = state?.comments || [];
     qs('#update-comments').innerHTML = comments.length ?
-      comments.map(renderIssueComment).join('') : '<div class="muted">No comments yet.</div>';
+      comments.map(comment => renderIssueComment(comment, controller)).join('') : '<div class="muted">No comments yet.</div>';
     qs('#load-older-update-comments').hidden = !Number.isInteger(state?.older_page);
     qs('#update-conversation-status').textContent = comments.length ?
       comments.length + ' of ' + Math.max(state.total || 0, comments.length) + ' messages loaded.' : 'No comments yet.';
@@ -3129,10 +3131,10 @@
     updateReadPosition.ready(String(selectedUpdate?.notification_id || ''), newest);
   }
 
-  function renderPullConversation(state) {
+  function paintPullConversation(state, controller) {
     const comments = state?.comments || [];
     qs('#pull-comments').innerHTML = comments.length ? comments.map(comment =>
-      '<div class="pull-comment-card">' + renderIssueComment(comment) + '</div>'
+      '<div class="pull-comment-card">' + renderIssueComment(comment, controller) + '</div>'
     ).join('') : '<div class="muted">No comments yet.</div>';
     qs('#load-older-pull-comments').hidden = !Number.isInteger(state?.older_page);
     qs('#pull-conversation-status').textContent = comments.length ?
@@ -3154,16 +3156,47 @@
     };
   }
 
-  function wireCommentActions(selector) {
+  function wireCommentActions(selector, controller = commentActions) {
-    commentActions.wire({
+    controller.wire({
       root:qs(selector), getSurface:()=>commentSurface(selector),
       isOffline:()=>offlineWorkMode || navigator.onLine === false, escapeHtml,
     });
   }
 
-  wireCommentActions('#issue-comments');
+  const conversationActionSurfaces = {
-  wireCommentActions('#pull-comments');
+    issue: { selector:'#issue-comments', paint:paintIssueConversation },
-  wireCommentActions('#update-comments');
+    pull: { selector:'#pull-comments', paint:paintPullConversation },
+    update: { selector:'#update-comments', paint:paintUpdateConversation },
+  };
+  const latestConversationStates = {};
+
+  function wireConversationActions(kind) {
+    if (kind === 'issue') wireCommentActions('#issue-comments');
+    if (kind === 'pull') wireCommentActions('#pull-comments');
+    if (kind === 'update') wireCommentActions('#update-comments');
+  }
+
+  function showConversationWithActions(kind, state) {
+    const surface = conversationActionSurfaces[kind];
+    latestConversationStates[kind] = state;
+    return commentActionHydrator.show({
+      root:qs(surface.selector), state, paint:surface.paint,
+      retry:qs('#retry-' + kind + '-comment-actions'),
+      wire:controller => wireConversationActions(kind, controller),
+    });
+  }
+
+  function renderIssueConversation(state) { void showConversationWithActions('issue', state); }
+  function renderPullConversation(state) { void showConversationWithActions('pull', state); }
+  function renderUpdateConversation(state) { void showConversationWithActions('update', state); }
+
+  function retryConversationActions(kind) {
+    const state = latestConversationStates[kind];
+    if (state) void showConversationWithActions(kind, state);
+  }
+  qs('#retry-issue-comment-actions').addEventListener('click', () => retryConversationActions('issue'));
+  qs('#retry-pull-comment-actions').addEventListener('click', () => retryConversationActions('pull'));
+  qs('#retry-update-comment-actions').addEventListener('click', () => retryConversationActions('update'));
 
   function renderIssueLabelEditor(item, confirmedNames, labels) {
     const list = qs('#issue-label-list');

frontend/index.html

@@ -595,6 +595,7 @@
     <div id="issue-comments"></div>
     <button class="conversation-more" id="load-older-issue-comments" type="button" hidden>Load older messages</button>
     <div id="issue-conversation-status" class="small" aria-live="assertive"></div>
+    <button class="conversation-actions-retry" id="retry-issue-comment-actions" type="button" hidden>Retry comment tools</button>
     <section class="issue-comment-composer" aria-labelledby="issue-comment-title">
       <h2 id="issue-comment-title">Add comment</h2>
       <textarea id="issue-comment" maxlength="10000" placeholder="Write a comment"></textarea>
@@ -921,6 +922,7 @@
     <button class="conversation-more" id="load-older-update-comments" type="button" hidden>Load older messages</button>
     <div id="update-conversation-status" class="small" aria-live="assertive"></div>
     <button class="update-retry" id="retry-update-conversation" type="button" hidden>Retry conversation</button>
+    <button class="conversation-actions-retry" id="retry-update-comment-actions" type="button" hidden>Retry comment tools</button>
     <details>
       <summary><h2>Subject context</h2></summary>
       <div class="update-sheet-content muted markdown-content" id="update-subject-body"></div>
@@ -995,6 +997,7 @@
     <h2>Full conversation</h2><div id="pull-comments"></div>
     <button class="conversation-more" id="load-older-pull-comments" type="button" hidden>Load older messages</button>
     <div id="pull-conversation-status" class="small" aria-live="assertive"></div>
+    <button class="conversation-actions-retry" id="retry-pull-comment-actions" type="button" hidden>Retry comment tools</button>
     <section class="pull-comment-composer" aria-labelledby="pull-comment-title">
       <h2 id="pull-comment-title">Add comment</h2>
       <textarea id="pull-comment" maxlength="10000" placeholder="Write a comment"></textarea>
@@ -1182,6 +1185,7 @@
 
 <script src="static/session.js"></script>
 <script src="static/feature-loader.js"></script>
+<script src="static/conversation-action-hydrator.js"></script>
 <script src="static/security-center.js"></script>
 <script src="static/markdown.js"></script>
 <script src="static/commands.js"></script>

frontend/service-worker.js

@@ -14,6 +14,7 @@ const SHELL = [
   BASE + 'static/icons/stackchain-512.png',
   BASE + 'static/session.js',
   BASE + 'static/feature-loader.js',
+  BASE + 'static/conversation-action-hydrator.js',
   BASE + 'static/security-center.js',
   BASE + 'static/markdown.js',
   BASE + 'static/commands.js',

tests/test_conversation_action_hydrator.py

@@ -0,0 +1,120 @@
+import json
+import subprocess
+from pathlib import Path
+
+
+FRONTEND = Path(__file__).parents[1] / "frontend"
+HYDRATOR = FRONTEND / "conversation-action-hydrator.js"
+
+
+def run_node(script: str):
+    result = subprocess.run(["node", "-e", script], check=True, capture_output=True, text=True)
+    return json.loads(result.stdout)
+
+
+def test_conversation_paints_before_optional_actions_load_and_hydrates_once():
+    script = f"""
+const createConversationActionHydrator = require({json.dumps(str(HYDRATOR))});
+const events = [];
+let finishLoad;
+const hydrator = createConversationActionHydrator({{
+  load: () => {{
+    events.push('load');
+    return new Promise(resolve => {{ finishLoad = resolve; }});
+  }},
+  activate: () => ({{name:'actions'}}),
+}});
+const root = {{}};
+const retry = {{hidden:false}};
+const status = {{textContent:'2 messages loaded.'}};
+const state = {{comments:[1,2]}};
+const pending = hydrator.show({{
+  root, state, retry, status,
+  paint: (_state, actions) => events.push(actions ? 'paint-actions' : 'paint-core'),
+  wire: () => events.push('wire'),
+}});
+events.push('returned');
+finishLoad();
+pending.then(() => {{
+  hydrator.show({{
+    root, state, retry, status,
+    paint: (_state, actions) => events.push(actions ? 'paint-actions-again' : 'paint-core-again'),
+    wire: () => events.push('wire-again'),
+  }}).then(() => process.stdout.write(JSON.stringify({{events,retry,status:status.textContent}})));
+}});
+"""
+    output = run_node(script)
+
+    assert output == {
+        "events": [
+            "paint-core", "load", "returned", "wire", "paint-actions",
+            "paint-actions-again",
+        ],
+        "retry": {"hidden": True},
+        "status": "2 messages loaded.",
+    }
+
+
+def test_failed_comment_actions_keep_conversation_usable_and_retry_in_place():
+    script = f"""
+const createConversationActionHydrator = require({json.dumps(str(HYDRATOR))});
+const events = [];
+let attempt = 0;
+const hydrator = createConversationActionHydrator({{
+  load: async () => {{
+    attempt += 1;
+    if (attempt === 1) throw new Error('chunk unavailable');
+  }},
+  activate: () => ({{name:'actions'}}),
+}});
+const root = {{}};
+const retry = {{hidden:true}};
+const status = {{textContent:'2 messages loaded.'}};
+const options = {{
+  root, retry, status, state:{{comments:[1,2]}},
+  paint: (_state, actions) => events.push(actions ? 'actions' : 'core'),
+  wire: () => events.push('wire'),
+}};
+(async () => {{
+  const failed = await hydrator.show(options);
+  events.push('reply-still-usable');
+  const recovered = await hydrator.show(options);
+  process.stdout.write(JSON.stringify({{
+    failed, recovered, attempt, events, retry, status:status.textContent,
+  }}));
+}})();
+"""
+    output = run_node(script)
+
+    assert output == {
+        "failed": False,
+        "recovered": True,
+        "attempt": 2,
+        "events": ["core", "reply-still-usable", "core", "wire", "actions"],
+        "retry": {"hidden": True},
+        "status": "2 messages loaded.",
+    }
+
+
+def test_issue_pull_and_update_conversations_trigger_optional_actions_not_startup():
+    html = (FRONTEND / "index.html").read_text()
+    javascript = (FRONTEND / "dashboard.js").read_text()
+
+    assert '<script src="static/conversation-action-hydrator.js"></script>' in html
+    assert html.index("static/conversation-action-hydrator.js") < html.index("static/dashboard.js")
+    assert "await commentActionFeatures.run('comment-actions'" not in javascript
+    assert "load: () => commentActionFeatures.load('comment-actions')" in javascript
+    assert "createConversationActionHydrator" in javascript
+    for kind in ("issue", "pull", "update"):
+        assert f"retry-{kind}-comment-actions" in html
+        assert f"showConversationWithActions('{kind}'" in javascript
+        assert f"qs('#retry-{kind}-comment-actions').addEventListener" in javascript
+
+
+def test_comment_action_retry_is_a_phone_sized_inline_control():
+    css = (FRONTEND / "dashboard.css").read_text()
+
+    assert ".conversation-actions-retry" in css
+    rule = css.split(".conversation-actions-retry", 1)[1].split("}", 1)[0]
+    assert "min-height:44px" in rule
+    assert "max-width:100%" in rule

tests/test_service_worker.py

@@ -787,6 +787,7 @@ def test_install_precaches_complete_subpath_scoped_app_shell():
         "/dashboard/static/icons/stackchain-512.png",
         "/dashboard/static/session.js",
         "/dashboard/static/feature-loader.js",
+        "/dashboard/static/conversation-action-hydrator.js",
         "/dashboard/static/security-center.js",
         "/dashboard/static/markdown.js",
         "/dashboard/static/commands.js",