diff --git a/frontend/index.html b/frontend/index.html index 03ad5ae..bf42ff9 100644 --- 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; } 0 of 0 files reviewed +
+

Draft feedback

+ + +
+ + + +
+

Review history

@@ -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, diff --git a/frontend/review-sheet.js b/frontend/review-sheet.js index 292e08b..a090251 100644 --- a/frontend/review-sheet.js +++ b/frontend/review-sheet.js @@ -43,6 +43,8 @@ function renderDiffFile(file, index, escapeHtml) { '' + filename + '' + escapeHtml(file.status || 'changed') + ' · +' + Number(file.additions || 0) + ' / −' + Number(file.deletions || 0) + '' + preview + + '' + + '' + ''; } @@ -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; diff --git a/tests/test_my_work.py b/tests/test_my_work.py index 4415ee1..fefa0b0 100644 --- 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()