diff --git a/src/chrome-client.js b/src/chrome-client.js index 4143b8f..be7dc1a 100644 --- a/src/chrome-client.js +++ b/src/chrome-client.js @@ -5,7 +5,7 @@ * hostname: a separate origin that can never reach this page or its token. */ import { tidy } from "./anchor-text.js"; -import { pageUrl, replacePage } from "./chrome-session.js"; +import { newestComments, pageUrl, replacePage } from "./chrome-session.js"; import { framePolicy } from "./frame-policy.js"; const $ = (id) => document.getElementById(id); @@ -139,7 +139,7 @@ function render() { const page = state.page; if (!page) return; - const comments = page.comments || []; + const comments = newestComments(page.comments); const edits = page.edits || []; $("count").textContent = String(comments.length); @@ -175,9 +175,21 @@ function render() { sep.textContent = "·"; const when = document.createElement("span"); when.className = "when"; - when.textContent = ago(comment.createdAt); + when.textContent = ago(comment.updatedAt || comment.createdAt); who.append(sep, when); + if (comment.correction) { + const badge = document.createElement("span"); + badge.className = "badge correction"; + badge.textContent = "correction"; + who.append(badge); + } else if (comment.updatedAt) { + const badge = document.createElement("span"); + badge.className = "badge"; + badge.textContent = "edited"; + who.append(badge); + } + if (state.orphans.has(comment.id)) { const badge = document.createElement("span"); badge.className = "badge"; @@ -194,6 +206,15 @@ function render() { setActive(comment.id, true); }); + const edit = document.createElement("button"); + edit.type = "button"; + edit.className = "jump edit-comment"; + edit.textContent = "Edit"; + edit.addEventListener("click", (event) => { + event.stopPropagation(); + editComment(card, body, comment); + }); + const remove = document.createElement("button"); remove.type = "button"; remove.className = "remove"; @@ -207,8 +228,6 @@ function render() { render(); }); - head.append(who, jump, remove); - const quote = document.createElement("p"); quote.className = "quote"; quote.textContent = tidy(comment.quote, 140); @@ -222,6 +241,8 @@ function render() { editComment(card, body, comment); }); + head.append(who, jump, edit, remove); + card.append(head, quote, body); card.addEventListener("click", () => setActive(comment.id, false)); list.append(card); @@ -379,11 +400,21 @@ function editComment(card, body, comment) { const feedback = input.value.trim(); if (!feedback || feedback === comment.feedback) return render(); try { - state.page = (await api(`/api/page/${state.key}/comment/${comment.id}`, { + const result = await api(`/api/page/${state.key}/comment/${comment.id}`, { method: "PATCH", body: JSON.stringify({ feedback }), - })).page; - state.sent = false; + }); + state.page = result.page; + if (result.delivery === "updated-pending") { + toast("Updated the feedback waiting for your agent"); + } else if (result.delivery === "correction") { + state.sent = false; + toFrame({ type: "eh:remove", id: comment.id }); + toFrame({ type: "eh:anchors", comments: state.page.comments }); + toast("Saved as a correction — send it after the current batch is acknowledged"); + } else { + state.sent = false; + } } catch (err) { toast(err.message); } diff --git a/src/chrome-session.js b/src/chrome-session.js index 5b00040..7c55fc9 100644 --- a/src/chrome-session.js +++ b/src/chrome-session.js @@ -6,3 +6,10 @@ export function replacePage(state, page) { state.page = page; state.others = page.others || []; } + +/** New and newly revised feedback belongs where the reviewer can see it. */ +export function newestComments(comments) { + return [...(comments || [])].sort( + (a, b) => (b.updatedAt || b.createdAt || 0) - (a.updatedAt || a.createdAt || 0) + ); +} diff --git a/src/chrome.css b/src/chrome.css index 6440d14..89a5f26 100644 --- a/src/chrome.css +++ b/src/chrome.css @@ -254,6 +254,7 @@ body.collapsed .handle { right: 0; } cursor: pointer; } .jump:hover { color: var(--strong-txt); } +.edit-comment { margin-left: 0; } .remove { width: 18px; height: 18px; @@ -321,6 +322,7 @@ body.collapsed .handle { right: 0; } font-size: 10px; font-weight: 600; } +.badge.correction { background: var(--danger-soft); color: var(--danger); } .empty { padding: 16px 14px; diff --git a/src/server.js b/src/server.js index 7c9667a..65ff0e0 100644 --- a/src/server.js +++ b/src/server.js @@ -222,6 +222,7 @@ export function createServer() { quote: c.quote, anchor: c.anchor, feedback: c.feedback, + ...(c.correction ? { correction: true, correction_of: c.correctionOf } : {}), })), edits: page.edits.map((e) => ({ label: e.label, @@ -257,6 +258,7 @@ export function createServer() { const hasMarkdown = pages.some((p) => p.kind === "file" && isMarkdown(p.file)); const hasUrl = pages.some((p) => p.kind === "url"); + const hasCorrections = pages.some((p) => p.comments.some((c) => c.correction)); const batch = { status: "feedback", pages: pages.map(({ kind, file, url, comments, edits }) => ({ kind, file, ...(url ? { url } : {}), comments, edits })), @@ -277,6 +279,9 @@ export function createServer() { "When an edit includes `staged_assets`, copy each local image into the app's appropriate asset folder, replace its " + "temporary preview URL in `after_html`, and preserve the image at the user's insertion point. " : "") + + (hasCorrections + ? "Comments marked `correction` replace their `correction_of` instruction; follow the correction and do not apply the older wording. " + : "") + "When every page is updated, run the same poll command again with --ack to clear this " + "batch and wait for more.", }; @@ -673,8 +678,37 @@ export function createServer() { const body = await readBody(req); const feedback = String(body.feedback || "").trim(); if (!feedback) return json(res, 400, { error: "empty feedback" }); - if (!store.updateComment(key, tail, feedback)) return json(res, 404, { error: "unknown comment" }); - return json(res, 200, { page: pageState(key) }); + const existing = store.page(key).comments.find((comment) => comment.id === tail); + if (!existing) return json(res, 404, { error: "unknown comment" }); + + const matchingCleanup = (record) => + record.cleanup.some((item) => item.key === key && item.ids.includes(tail)); + const wasDelivered = [...batches.values()].some((record) => record.delivered && matchingCleanup(record)); + + if (wasDelivered) { + // The agent already received the old wording, so the revision must + // survive that batch's ack and explicitly supersede it next time. + store.updateComment(key, tail, feedback, { + replacementId: uid("c"), + correctionOf: existing.feedback, + }); + return json(res, 200, { delivery: "correction", page: pageState(key) }); + } + + store.updateComment(key, tail, feedback); + let updatedPending = false; + const target = store.page(key); + const targetName = target.kind === "url" ? target.url : target.file; + for (const [entryKey, record] of batches) { + if (record.delivered || !matchingCleanup(record)) continue; + const batchPage = record.batch.pages.find((page) => page.file === targetName || page.url === targetName); + const batchComment = batchPage?.comments.find((comment) => comment.id === tail); + if (!batchComment) continue; + batchComment.feedback = feedback; + store.setBatch(entryKey, record); + updatedPending = true; + } + return json(res, 200, { delivery: updatedPending ? "updated-pending" : "unsent", page: pageState(key) }); } if (action === "edit" && req.method === "POST") { diff --git a/src/state.js b/src/state.js index 4fec323..7556480 100644 --- a/src/state.js +++ b/src/state.js @@ -189,13 +189,21 @@ export class Store { }); } - /** Rewording feedback before it is sent. Returns null for an unknown id. */ - updateComment(key, id, feedback) { + /** Reword feedback, optionally turning a delivered instruction into a correction. */ + updateComment(key, id, feedback, { replacementId = "", correctionOf = "" } = {}) { let found = false; const page = this.update(key, (p) => { - const comment = p.comments.find((c) => c.id === id); + const index = p.comments.findIndex((c) => c.id === id); + const comment = p.comments[index]; if (comment) { - comment.feedback = feedback; + const updated = { + ...comment, + ...(replacementId ? { id: replacementId } : {}), + feedback, + updatedAt: Date.now(), + ...(correctionOf ? { correction: true, correctionOf } : {}), + }; + p.comments[index] = updated; found = true; } }); diff --git a/test/chrome-session.test.js b/test/chrome-session.test.js index 78284d6..b30e7ed 100644 --- a/test/chrome-session.test.js +++ b/test/chrome-session.test.js @@ -1,7 +1,7 @@ import test from "node:test"; import assert from "node:assert/strict"; -import { pageUrl, replacePage } from "../src/chrome-session.js"; +import { newestComments, pageUrl, replacePage } from "../src/chrome-session.js"; test("page refreshes keep session context and clear stale cross-page counts", () => { assert.equal(pageUrl("abc123", "session with spaces"), "/api/page/abc123?session=session%20with%20spaces"); @@ -17,3 +17,14 @@ test("page refreshes keep session context and clear stale cross-page counts", () assert.equal(state.page, refreshed); assert.deepEqual(state.others, []); }); + +test("comments are newest-first and an edited comment moves to the top", () => { + const comments = [ + { id: "old", createdAt: 100 }, + { id: "new", createdAt: 300 }, + { id: "edited", createdAt: 50, updatedAt: 400 }, + ]; + + assert.deepEqual(newestComments(comments).map((comment) => comment.id), ["edited", "new", "old"]); + assert.deepEqual(comments.map((comment) => comment.id), ["old", "new", "edited"], "stored order is not mutated"); +}); diff --git a/test/feedback-safety.test.js b/test/feedback-safety.test.js index 29ab92b..7a60348 100644 --- a/test/feedback-safety.test.js +++ b/test/feedback-safety.test.js @@ -152,6 +152,7 @@ test("a comment can be reworded before it is sent", async (t) => { }); assert.equal(reworded.status, 200); assert.deepEqual(j(reworded).page.comments.map((c) => c.feedback), ["Sharper thoughts"]); + assert.equal(typeof j(reworded).page.comments[0].updatedAt, "number"); const missing = await request(port, token, { method: "PATCH", route: `/api/page/${key}/comment/c_nope`, body: { feedback: "x" } }); assert.equal(missing.status, 404); @@ -165,6 +166,56 @@ test("a comment can be reworded before it is sent", async (t) => { await ackAndAbandon(port, token, file); }); +test("rewording replaces a waiting batch and becomes a correction after delivery", async (t) => { + const file = path.join(tmp, "reword-after-send.html"); + fs.writeFileSync(file, "
Draft
"); + const { port, token, dispose } = await start(0); + t.after(() => dispose()); + + const opened = j(await request(port, token, { method: "POST", route: "/api/session", body: { file } })); + const added = j(await request(port, token, { + method: "POST", + route: `/api/page/${opened.key}/comment`, + body: { kind: "selection", quote: "Draft", feedback: "Delete this" }, + })); + + // Nothing is polling yet, so editing should replace the stranded batch in place. + await request(port, token, { method: "POST", route: `/api/page/${opened.key}/send`, body: { sessionId: opened.sessionId, note: "" } }); + const waitingEdit = j(await request(port, token, { + method: "PATCH", + route: `/api/page/${opened.key}/comment/${added.comment.id}`, + body: { feedback: "Shorten this" }, + })); + assert.equal(waitingEdit.delivery, "updated-pending"); + + const delivered = j(await request(port, token, { route: `/api/poll?target=${encodeURIComponent(file)}` })); + assert.equal(delivered.pages[0].comments[0].feedback, "Shorten this", "the agent never sees the superseded wording"); + + // Once delivered, a further revision must survive ack as an explicit correction. + const correction = j(await request(port, token, { + method: "PATCH", + route: `/api/page/${opened.key}/comment/${added.comment.id}`, + body: { feedback: "Keep it, but add an example" }, + })); + assert.equal(correction.delivery, "correction"); + assert.equal(correction.page.comments[0].correction, true); + assert.equal(correction.page.comments[0].correctionOf, "Shorten this"); + assert.notEqual(correction.page.comments[0].id, added.comment.id, "the delivered id is retired so ack cannot clear the correction"); + + await ackAndAbandon(port, token, file); + const afterAck = j(await request(port, token, { route: `/api/page/${opened.key}` })); + assert.equal(afterAck.comments.length, 1); + assert.equal(afterAck.comments[0].feedback, "Keep it, but add an example"); + + await request(port, token, { method: "POST", route: `/api/page/${opened.key}/send`, body: { sessionId: opened.sessionId, note: "" } }); + const correctedBatch = j(await request(port, token, { route: `/api/poll?target=${encodeURIComponent(file)}` })); + const shipped = correctedBatch.pages[0].comments[0]; + assert.equal(shipped.correction, true); + assert.equal(shipped.correction_of, "Shorten this"); + assert.match(correctedBatch.next_step, /replace their `correction_of` instruction/); + await ackAndAbandon(port, token, file); +}); + test("a save based on a stale version of the file is refused", async (t) => { const file = path.join(tmp, "save.html"); const v1 = "\nOne
\n";