Merge pull request 'Carry mobile review feedback into the authenticated Gitea handoff' (#130) from timmy/129-mobile-review-feedback-handoff into main

2026-08-06 18:57:14 +00:00 · 2026-08-06 18:57:14 +00:00 · e2a4b60143
commit e2a4b60143
parent bd0ae619db 331eb99c1e
3 changed files with 266 additions and 0 deletions
--- a/frontend/index.html
+++ b/frontend/index.html
@ -77,6 +77,13 @@ textarea { resize: vertical; min-height: 120px; }
 .review-file.reviewed .review-file-toggle { border-color:#22c55e; }
 .review-mark { min-height:44px; width:100%; margin-top:8px; }
 .review-mark[aria-pressed="true"] { border-color:#22c55e; background:#123c2a; }
+.review-note { min-height:88px; margin-top:6px; }
+.review-feedback { display:grid; gap:8px; }
+.review-feedback label { display:grid; gap:6px; }
+.review-feedback select { min-height:44px; padding:8px; border-radius:8px; border:1px solid #1f3a5f; background:#0b1526; color:#e5e7eb; }
+.review-handoff { position:sticky; bottom:0; z-index:3; display:grid; gap:8px; padding:10px 4px; background:rgba(11,21,38,.98); border-top:1px solid #2a496e; }
+.review-handoff button { min-height:44px; }
+.review-copy-fallback { min-height:140px; }
 .review-progress-actions { position:sticky; bottom:0; z-index:2; display:flex; align-items:center; justify-content:space-between; gap:10px; margin:8px -4px 0; padding:10px 4px; background:rgba(11,21,38,.96); border-top:1px solid #2a496e; }
 .review-progress-actions button { min-height:44px; max-width:100%; }
 .review-diff { overflow-x:auto; max-width:100%; margin-top:8px; white-space:pre; }
@ -232,6 +239,24 @@ textarea { resize: vertical; min-height: 120px; }
      <span class="small" id="review-progress" aria-live="polite">0 of 0 files reviewed</span>
      <button id="next-unreviewed-review" disabled>Next unreviewed</button>
    </div>
+    <section class="review-feedback" aria-labelledby="review-feedback-title">
+      <h2 id="review-feedback-title">Draft feedback</h2>
+      <label>Intended decision
+        <select id="review-decision">
+          <option value="comment">Comment</option>
+          <option value="approve">Approve</option>
+          <option value="request_changes">Request changes</option>
+        </select>
+      </label>
+      <label>Overall summary
+        <textarea id="review-summary" placeholder="Summarize your review"></textarea>
+      </label>
+      <div class="review-handoff">
+        <button id="copy-review-feedback">Copy feedback &amp; continue in Gitea</button>
+        <span class="small" id="review-handoff-status" aria-live="polite"></span>
+        <textarea class="review-copy-fallback" id="review-copy-fallback" readonly hidden aria-label="Feedback to copy manually"></textarea>
+      </div>
+    </section>
    <h2>Review history</h2>
    <div id="review-history" class="muted"></div>
  </section>
@ -284,6 +309,8 @@ textarea { resize: vertical; min-height: 120px; }
  let selectedReview = null;
  let reviewTrigger = null;
  let progress = null;
+  let draft = null;
+  let reviewFiles = [];

  async function fetchReviewJson(url, options) {
    const response = await fetch(url, options);
@ -424,6 +451,13 @@ textarea { resize: vertical; min-height: 120px; }
    qs('#next-unreviewed-review').disabled = true;
    qs('#review-history').textContent = '';
    qs('#open-review-gitea').href = item.url;
+    qs('#review-decision').value = 'comment';
+    qs('#review-summary').value = '';
+    qs('#review-copy-fallback').hidden = true;
+    qs('#review-copy-fallback').value = '';
+    qs('#review-handoff-status').textContent = '';
+    draft = null;
+    reviewFiles = [];
    qs('#close-review-sheet').focus();
    try {
      const detail = await reviewController.load(selectedReview);
@ -446,6 +480,21 @@ textarea { resize: vertical; min-height: 120px; }
        headSha: detail.head_sha || 'unknown',
        files: detail.files || [],
      });
+      reviewFiles = detail.files || [];
+      draft = createReviewController.createDraft({
+        storage: localStorage,
+        repository: item.repository,
+        number: item.number,
+        headSha: detail.head_sha || 'unknown',
+        files: reviewFiles,
+      });
+      const draftSnapshot = draft.snapshot();
+      qs('#review-decision').value = draftSnapshot.decision;
+      qs('#review-summary').value = draftSnapshot.summary;
+      document.querySelectorAll('.review-note').forEach(note => {
+        note.value = draftSnapshot.notes[note.dataset.reviewFilename] || '';
+        note.addEventListener('input', () => draft.setNote(note.dataset.reviewFilename, note.value));
+      });
      document.querySelectorAll('.review-mark').forEach(button => {
        button.addEventListener('click', () => {
          const snapshot = progress.markReviewed(button.dataset.reviewFilename);
@ -473,6 +522,8 @@ textarea { resize: vertical; min-height: 120px; }
    qs('#review-sheet').classList.remove('open');
    selectedReview = null;
    progress = null;
+    draft = null;
+    reviewFiles = [];
    if (reviewTrigger?.isConnected) reviewTrigger.focus();
  }

@ -589,6 +640,28 @@ textarea { resize: vertical; min-height: 120px; }
  qs('#next-unreviewed-review').addEventListener('click', () => {
    if (progress) openNextUnreviewed(progress.snapshot());
  });
+  qs('#review-summary').addEventListener('input', event => draft?.setSummary(event.target.value));
+  qs('#review-decision').addEventListener('change', event => draft?.setDecision(event.target.value));
+  qs('#copy-review-feedback').addEventListener('click', async () => {
+    if (!draft || !selectedReview) return;
+    const fallback = qs('#review-copy-fallback');
+    fallback.hidden = true;
+    const copied = await createReviewController.copyAndContinue({
+      text: createReviewController.formatFeedback(draft.snapshot(), reviewFiles),
+      url: selectedReview.url,
+      copy: text => navigator.clipboard.writeText(text),
+      open: url => window.open(url, '_blank', 'noopener,noreferrer'),
+      fallback: text => {
+        fallback.value = text;
+        fallback.hidden = false;
+        fallback.focus();
+        fallback.select();
+      },
+    });
+    qs('#review-handoff-status').textContent = copied ?
+      'Feedback copied. Opening Gitea…' :
+      'Clipboard unavailable. Copy the selected feedback, then use Open in Gitea.';
+  });

  const contextPoller = createContextPoller({
    fetchContext: fetchContextSnapshot,
--- a/frontend/review-sheet.js
+++ b/frontend/review-sheet.js
@ -43,6 +43,8 @@ function renderDiffFile(file, index, escapeHtml) {
    '<strong>' + filename + '</strong><span class="small">' +
    escapeHtml(file.status || 'changed') + ' · +' + Number(file.additions || 0) + ' / −' +
    Number(file.deletions || 0) + '</span></button>' + preview +
+    '<label class="small" for="review-note-' + index + '">Note for ' + filename + '</label>' +
+    '<textarea class="review-note" id="review-note-' + index + '" data-review-filename="' + filename + '" placeholder="Capture feedback for this file"></textarea>' +
    '<button class="review-mark" data-review-filename="' + filename + '" aria-pressed="false">Mark reviewed</button></div>';
 }

@ -91,9 +93,95 @@ function createProgress({ storage, repository, number, headSha, files }) {
  return { snapshot, markReviewed };
 }

+function createDraft({ storage, repository, number, headSha, files }) {
+  const filenames = (files || []).map(file => file && file.filename).filter(Boolean);
+  const key = 'stackchain.review-draft.v1:' + repository + '#' + number + '@' + headSha;
+  let draft = { notes: {}, summary: '', decision: 'comment' };
+  try {
+    const saved = JSON.parse(storage.getItem(key) || '{}');
+    if (saved && typeof saved === 'object') {
+      draft.notes = Object.fromEntries(filenames
+        .filter(filename => typeof saved.notes?.[filename] === 'string' && saved.notes[filename])
+        .map(filename => [filename, saved.notes[filename]]));
+      if (typeof saved.summary === 'string') draft.summary = saved.summary;
+      if (['comment', 'approve', 'request_changes'].includes(saved.decision)) {
+        draft.decision = saved.decision;
+      }
+    }
+  } catch (error) { /* start with an empty in-memory draft */ }
+
+  function persist() {
+    try { storage.setItem(key, JSON.stringify(draft)); } catch (error) { /* draft remains usable */ }
+  }
+
+  function snapshot() {
+    return { notes: { ...draft.notes }, summary: draft.summary, decision: draft.decision };
+  }
+
+  function setNote(filename, note) {
+    if (!filenames.includes(filename)) return snapshot();
+    if (String(note)) draft.notes[filename] = String(note);
+    else delete draft.notes[filename];
+    persist();
+    return snapshot();
+  }
+
+  function setSummary(summary) {
+    draft.summary = String(summary || '');
+    persist();
+    return snapshot();
+  }
+
+  function setDecision(decision) {
+    if (['comment', 'approve', 'request_changes'].includes(decision)) {
+      draft.decision = decision;
+      persist();
+    }
+    return snapshot();
+  }
+
+  return { snapshot, setNote, setSummary, setDecision };
+}
+
+function formatFeedback(draft, files) {
+  const decisions = {
+    comment: 'Comment',
+    approve: 'Approve',
+    request_changes: 'Request changes',
+  };
+  const sections = [
+    '## Intended decision\n' + (decisions[draft.decision] || decisions.comment),
+  ];
+  if (String(draft.summary || '').trim()) {
+    sections.push('## Summary\n' + String(draft.summary).trim());
+  }
+  const fileNotes = (files || []).flatMap(file => {
+    const filename = file && file.filename;
+    const note = filename && String(draft.notes?.[filename] || '').trim();
+    if (!note) return [];
+    return ['### `' + String(filename).replaceAll('`', '\\`') + '`\n' + note];
+  });
+  if (fileNotes.length) sections.push('## File notes\n' + fileNotes.join('\n\n'));
+  return sections.join('\n\n');
+}
+
+async function copyAndContinue({ text, url, copy, open, fallback }) {
+  try {
+    await copy(text);
+  } catch (error) {
+    fallback(text);
+    return false;
+  }
+  open(url);
+  return true;
+}
+
 createReviewController.renderDiffFile = renderDiffFile;
 createReviewController.toggleDiff = toggleDiff;
 createReviewController.createProgress = createProgress;
+createReviewController.createDraft = createDraft;
+createReviewController.formatFeedback = formatFeedback;
+createReviewController.copyAndContinue = copyAndContinue;

 if (typeof module !== 'undefined' && module.exports) {
  module.exports = createReviewController;
--- a/tests/test_my_work.py
+++ b/tests/test_my_work.py
@ -260,6 +260,89 @@ process.stdout.write(JSON.stringify(changed));
    }


+def test_review_feedback_draft_restores_notes_summary_and_decision_for_same_head():
+    script = f"""
+const reviewSheet = require({json.dumps(str(REVIEW_SHEET))});
+const values = new Map();
+const storage = {{
+  getItem(key) {{ return values.has(key) ? values.get(key) : null; }},
+  setItem(key, value) {{ values.set(key, value); }}
+}};
+const options = {{
+  storage, repository: 'stackchain/api', number: 7, headSha: 'abc123',
+  files: [{{filename:'src/a.py'}}, {{filename:'src/b.py'}}]
+}};
+const first = reviewSheet.createDraft(options);
+first.setNote('src/a.py', 'Handle the empty state.');
+first.setSummary('One blocker remains.');
+first.setDecision('request_changes');
+const restored = reviewSheet.createDraft(options).snapshot();
+process.stdout.write(JSON.stringify(restored));
+"""
+    result = subprocess.run(
+        ["node", "-e", script], check=True, capture_output=True, text=True
+    )
+
+    assert json.loads(result.stdout) == {
+        "notes": {"src/a.py": "Handle the empty state."},
+        "summary": "One blocker remains.",
+        "decision": "request_changes",
+    }
+
+
+def test_review_feedback_formats_non_empty_file_notes_in_changed_file_order():
+    script = f"""
+const reviewSheet = require({json.dumps(str(REVIEW_SHEET))});
+const markdown = reviewSheet.formatFeedback({{
+  decision: 'request_changes', summary: 'One blocker remains.',
+  notes: {{'src/b.py':'Second note', 'src/a.py':'First note', 'README.md':''}}
+}}, [{{filename:'src/a.py'}}, {{filename:'README.md'}}, {{filename:'src/b.py'}}]);
+process.stdout.write(markdown);
+"""
+    result = subprocess.run(
+        ["node", "-e", script], check=True, capture_output=True, text=True
+    )
+
+    assert result.stdout == (
+        "## Intended decision\nRequest changes\n\n"
+        "## Summary\nOne blocker remains.\n\n"
+        "## File notes\n### `src/a.py`\nFirst note\n\n"
+        "### `src/b.py`\nSecond note"
+    )
+
+
+def test_review_handoff_opens_gitea_only_after_copy_and_preserves_failed_copy():
+    script = f"""
+const reviewSheet = require({json.dumps(str(REVIEW_SHEET))});
+const events = [];
+const success = reviewSheet.copyAndContinue({{
+  text: 'feedback', url: 'https://forge.example/pulls/7',
+  copy: async text => events.push('copy:' + text),
+  open: url => events.push('open:' + url),
+  fallback: text => events.push('fallback:' + text),
+}});
+const failedEvents = [];
+const failure = reviewSheet.copyAndContinue({{
+  text: 'keep me', url: 'https://forge.example/pulls/8',
+  copy: async () => {{ throw new Error('denied'); }},
+  open: url => failedEvents.push('open:' + url),
+  fallback: text => failedEvents.push('fallback:' + text),
+}});
+Promise.all([success, failure]).then(results => process.stdout.write(JSON.stringify({{
+  events, failedEvents, results
+}})));
+"""
+    result = subprocess.run(
+        ["node", "-e", script], check=True, capture_output=True, text=True
+    )
+
+    assert json.loads(result.stdout) == {
+        "events": ["copy:feedback", "open:https://forge.example/pulls/7"],
+        "failedEvents": ["fallback:keep me"],
+        "results": [True, False],
+    }
+
+
@pytest.mark.anyio
 async def test_review_requests_open_an_accessible_mobile_detail_sheet():
    html = await dashboard()
@ -295,6 +378,28 @@ async def test_review_sheet_loads_details_and_preserves_safe_gitea_handoff():
    assert "reviewController.submit" not in html


+@pytest.mark.anyio
+async def test_mobile_review_sheet_captures_and_safely_hands_off_feedback():
+    html = await dashboard()
+    review_script = REVIEW_SHEET.read_text()
+
+    assert 'class="review-note"' in review_script
+    assert 'id="review-decision"' in html
+    assert 'id="review-summary"' in html
+    assert 'id="copy-review-feedback"' in html
+    assert 'id="review-copy-fallback"' in html
+    assert '.review-note' in html and 'min-height:88px' in html
+    assert '.review-handoff' in html and 'position:sticky' in html
+    assert "createReviewController.createDraft" in html
+    assert "draft.setNote" in html
+    assert "draft?.setSummary" in html
+    assert "draft?.setDecision" in html
+    assert "createReviewController.formatFeedback" in html
+    assert "createReviewController.copyAndContinue" in html
+    assert "navigator.clipboard.writeText" in html
+    assert "window.open(url, '_blank', 'noopener,noreferrer')" in html
+
+
@pytest.mark.anyio
 async def test_mobile_review_failure_offers_an_in_place_retry_for_the_same_item():
    html = await dashboard()