From 9b84634ffe6c6a227cdaf63b57a2e34733f2cec7 Mon Sep 17 00:00:00 2001 From: mchwang Date: Wed, 23 Sep 2026 12:59:13 -0700 Subject: [PATCH 01/23] Attach selected code lines to review feedback --- docs/implementation/read-only-review.md | 10 +++ runner/review.ts | 20 ++++- runner/store.ts | 7 +- test/browser/review.spec.ts | 29 +++++++ test/review.test.ts | 16 ++++ web/public/app.js | 108 +++++++++++++++++++++++- web/public/index.html | 2 + web/public/style.css | 11 +++ 8 files changed, 194 insertions(+), 9 deletions(-) diff --git a/docs/implementation/read-only-review.md b/docs/implementation/read-only-review.md index 269faae..9d318ec 100644 --- a/docs/implementation/read-only-review.md +++ b/docs/implementation/read-only-review.md @@ -50,3 +50,13 @@ Review round 5 found a symlinked-ancestor escape and raw-spelling collisions in Review round 6 reproduced destination-inside-source mutation; planting now resolves the existing destination ancestor and rejects canonical source descendants before creating files, including `.git` and symlink aliases. The Git-environment finding was declined: `gitRaw` already passes `{cwd, env, ...}` to `execFileSync`. A new end-to-end test with inherited `GIT_DIR`/`GIT_WORK_TREE` passed before any production change. The summary's snapshot-race claim supplied no concrete interleaving; the service already checks revision/snapshot/review version before publishing a view and writes use atomic store CAS, covered by concurrent-write tests. Review round 7 reproduced oversized-dimension raster previews and dangling SQLite sidecar symlinks. Preview metadata is now parsed from the bounded buffer with image-size: at most 8,192 pixels per side, 4 million pixels per image and 16 million pixels across unique preview blobs. Unknown dimensions omit the preview; existing compressed/response byte limits remain. Sidecars use lstat so dangling links are rejected. Both new regressions failed before the fixes. The summary mentioned rename-scope/literal-path concerns without specific findings; no additional behavior was inferred from that shorthand. + +## Feedback on selected code + +Click a changed line number, Shift-click to extend a range, or highlight code within one changed block. The selection toolbar offers Ask and Request change; each attaches the full selected lines to that item's composer. Question/change-request drafts retain separate attachments. Remove snippet returns to item-level feedback. + +The server validates the segment and range against the current review token and constructs the saved reference itself: path, old/new side, exact text, line range, base and head commits. Limits are 200 lines and 16,000 characters. References on unassigned changes require assigning them to a plan item first. Existing item-level notes remain compatible without a database migration. + +Saved references navigate to the selected lines while the reviewed commits and assignment still match. Otherwise they are labeled Outdated and open the original captured snippet, never silently pointing at new code. This deliberately marks references outdated on any base/head change, even if the snippet is unchanged. Refresh invalidates transient selections; outdated draft attachments must be removed and reselected before submission. No agent or GitHub comment is invoked. + +Snippet-feedback validation: 176 unit/integration tests and 15 browser tests pass (baseline 174 and 13), plus typecheck and diff checks. Coverage includes clicked lines, Shift-click ranges, DOM text highlighting, independent question/change drafts, removal, persistence, navigation, removed-side references, forged/invalid references and outdated snapshots. The actual PR #597 walkthrough was also opened in Chromium and the selected-line/composer layout visually inspected without saving test notes to its database. diff --git a/runner/review.ts b/runner/review.ts index 38ce52b..61974c2 100644 --- a/runner/review.ts +++ b/runner/review.ts @@ -1,7 +1,7 @@ import { createHash } from 'node:crypto'; import { basename } from 'node:path'; import { isDeepStrictEqual } from 'node:util'; -import { Store, type ReviewState } from './store.ts'; +import { Store, type ReviewState, type SnippetReference } from './store.ts'; import type { PlanIdentity } from '../core/identity.ts'; import { readHistory } from '../git/history.ts'; import { linkHistory } from '../core/linking.ts'; @@ -52,7 +52,7 @@ export class ReviewService { if (states[item.id] === 'approved' && (segments.some(segment => segment.row === 'Ambiguous' && segment.owners.includes(item.id)) || item.depends_on.some(id => states[id] === 'stale'))) states[item.id] = 'stale'; } const expected: ReviewState = { revision: plan.revision, snapshotId: snapshot.id, reviewVersion }; - const notes = this.store.getReviewNotes(identity); + const notes = this.store.getReviewNotes(identity).map(note => ({ ...note, outdated: !!note.reference && (note.reference.head !== history.head || note.reference.base !== history.base || !segments.some(segment => segment.key === note.reference!.key && segment.row === note.item)) })); if (this.store.reviewVersion(identity) !== reviewVersion || this.store.getPlan(identity).revision !== plan.revision || this.store.getSnapshot(identity).id !== snapshot.id) throw new Error('Stale review state. Reload before writing.'); const items = plan.items.map(item => { const owned = segments.filter(segment => segment.row === item.id); @@ -92,7 +92,21 @@ export class ReviewService { const storedKey = choiceKeys(view.segments, identity)[view.segments.indexOf(segment)]!; this.store.saveReview(identity, view.expected, [], [{ key: storedKey, action: command.action, item }]); } else if (command.action === 'note' && typeof command.item === 'string' && typeof command.text === 'string' && (command.kind === 'question' || command.kind === 'change')) { - this.store.addReviewNote(identity, view.expected, command.item, command.kind, command.text); + let reference: SnippetReference | undefined; + if (command.reference !== undefined) { + if (!command.reference || typeof command.reference !== 'object') throw new Error('Invalid snippet reference.'); + const input = command.reference as Record; + const segment = view.segments.find(s => s.key === input.key && s.row === command.item && s.kind !== 'file'); + const start = input.start as number, end = input.end as number; + if (!segment || !Number.isSafeInteger(start) || !Number.isSafeInteger(end)) throw new Error('Invalid snippet reference.'); + const first = segment.operation === '+' ? segment.newLine : segment.oldLine; + const lines = segment.content.split('\n'); if (lines.at(-1) === '') lines.pop(); + if (first === null || start < first || end < start || end >= first + lines.length || end - start >= 200) throw new Error('Select up to 200 lines within one changed block.'); + const text = lines.slice(start-first, end-first+1).join('\n'); + if (text.length > 16000) throw new Error('Selected snippet exceeds 16000 characters.'); + reference = { key: segment.key, path: segment.operation === '-' ? segment.oldPath ?? segment.path : segment.path, side: segment.operation === '+' ? 'new' : 'old', start, end, text, head: view.snapshot.head, base: view.snapshot.base }; + } + this.store.addReviewNote(identity, view.expected, command.item, command.kind, command.text, reference); } else throw new Error('Unknown review command.'); return this.load(); } diff --git a/runner/store.ts b/runner/store.ts index cc96c5e..020f79c 100644 --- a/runner/store.ts +++ b/runner/store.ts @@ -12,7 +12,8 @@ export function requireSupportedNode(version = process.versions.node): void { } export interface Snapshot { id: string; base: string; head: string } export interface ReviewState { revision: number; snapshotId: string; reviewVersion?: number } -export interface ReviewNote { id: string; item: string; kind: 'question' | 'change'; text: string; createdAt: string; revision: number; snapshotId: string } +export interface SnippetReference { key: string; path: string; side: 'old' | 'new'; start: number; end: number; text: string; head: string; base: string } +export interface ReviewNote { id: string; item: string; kind: 'question' | 'change'; text: string; reference?: SnippetReference; createdAt: string; revision: number; snapshotId: string } export interface LedgerEntry { sha: string; owner: string | null; origin: 'owned' | 'foreign'; sourceSha: string | null } export interface Checkpoint { id: string; revision: number; snapshotId: string; item: string; @@ -246,12 +247,12 @@ export class Store { }); } reviewVersion(identity: PlanIdentity): number { return this.#current(identityKey(identity)).review_version as number; } - addReviewNote(identity: PlanIdentity, expected: ReviewState, item: string, kind: ReviewNote['kind'], text: string): ReviewNote { + addReviewNote(identity: PlanIdentity, expected: ReviewState, item: string, kind: ReviewNote['kind'], text: string, reference?: SnippetReference): ReviewNote { const key = identityKey(identity); return this.#transaction(() => { this.#expect(key, expected); if (!this.getPlan(identity).items.some(entry => entry.id === item) || !['question', 'change'].includes(kind) || typeof text !== 'string' || !text.trim() || text.length > 4000) throw new Error('Invalid review note.'); - const note = { id: randomUUID(), item, kind, text: text.trim(), createdAt: new Date().toISOString(), revision: expected.revision, snapshotId: expected.snapshotId }; + const note = { id: randomUUID(), item, kind, text: text.trim(), ...(reference ? { reference } : {}), createdAt: new Date().toISOString(), revision: expected.revision, snapshotId: expected.snapshotId }; this.#run('INSERT INTO review_notes VALUES (?,?,?)', key, note.id, encode(note)); this.#run('UPDATE plans SET review_version=review_version+1 WHERE key=?', key); return note; diff --git a/test/browser/review.spec.ts b/test/browser/review.spec.ts index 9118065..427c274 100644 --- a/test/browser/review.spec.ts +++ b/test/browser/review.spec.ts @@ -77,3 +77,32 @@ test('routes mixed owned and ambiguous items to attribution resolution',async({p test('keeps review shortcuts active while a toolbar button has focus',async({page})=>{ await page.goto(app.url);await expect(page.getByRole('heading',{name:'Bound exponential retries'})).toBeVisible();await page.getByRole('button',{name:'Refresh',exact:true}).focus();await page.keyboard.press('n');await expect(page.getByRole('heading',{name:'Document retry behavior'})).toBeVisible(); }); +test('attaches clicked lines to a question, persists and navigates the reference, and shows archived evidence',async({page})=>{ + await page.goto(app.url); + const line=page.locator('.added [data-line]').first();await line.click(); + await page.getByRole('button',{name:'Ask about selection',exact:true}).click(); + await expect(page.locator('#attachment')).toContainText('retry.ts'); + await page.getByLabel('Question about this item').fill('Why this exact line?');await page.getByRole('button',{name:'Save question',exact:true}).click(); + await page.reload();await page.locator('.note-reference').click();await expect(page.locator('.selected-line').first()).toBeVisible(); + execFileSync('git',['-c','core.hooksPath=/dev/null','commit','--allow-empty','-m','another revision'],{cwd:app.service.config.repository,stdio:'pipe'}); + await page.getByRole('button',{name:'Refresh',exact:true}).click();await expect(page.locator('.note-reference')).toContainText('Outdated');await page.locator('.note-reference').click();await expect(page.getByRole('heading',{name:'! Outdated code reference'})).toBeVisible();await expect(page.locator('#dialog-body pre')).toContainText('Math.min'); +}); +test('supports shift ranges, highlighted lines, and independent snippet drafts',async({page})=>{ + const repository=app.service.config.repository; + writeFileSync(join(repository,'retry.ts'),'export const cap = 5000;\nexport const base = 100;\nexport const delay = (n: number) => Math.min(cap, base * 2 ** n);\n'); + execFileSync('git',['-c','core.hooksPath=/dev/null','commit','-am','rewrite retry'],{cwd:repository,stdio:'pipe'}); + let view=app.service.load(); + for(const key of view.segments.filter(s=>s.path==='retry.ts'&&['Unplanned','Ambiguous'].includes(s.row)).map(s=>s.key)) view=app.service.act({action:'assign',key,item:'P1',token:view.token}); + await page.goto(app.url); + const block=page.locator('.added[data-segment]').filter({hasText:'export const cap'}); + await block.locator('[data-line]').nth(0).click();await block.locator('[data-line]').nth(2).click({modifiers:['Shift']}); + await expect(page.locator('#selection-label')).toContainText('L1–3'); + await page.getByRole('button',{name:'Request change to selection',exact:true}).click();await expect(page.locator('#attachment pre')).toContainText('export const delay'); + await page.getByLabel('Change to request').fill('Please explain these constants.'); + await page.getByRole('button',{name:'Ask',exact:true}).click();await expect(page.locator('#attachment')).toBeHidden(); + await block.locator('.code-lines').evaluate(element=>{const lines=element.querySelectorAll('[data-code-line]');const range=document.createRange();range.setStart(lines[0]!.firstChild!,0);range.setEnd(lines[1]!.firstChild!,6);const selection=window.getSelection()!;selection.removeAllRanges();selection.addRange(range);element.dispatchEvent(new MouseEvent('mouseup',{bubbles:true}));}); + await expect(page.locator('#selection-label')).toContainText('L1–2');await page.getByRole('button',{name:'Ask about selection',exact:true}).click();await expect(page.locator('#attachment pre')).not.toContainText('export const delay'); + await page.getByRole('button',{name:'Remove snippet',exact:true}).click();await expect(page.locator('#attachment')).toBeHidden(); + await page.getByRole('button',{name:'Request change',exact:true}).click();await expect(page.locator('#attachment pre')).toContainText('export const delay'); + await page.getByRole('button',{name:'Save change request',exact:true}).click();await expect(page.locator('.note-reference')).toContainText('L1–3'); +}); diff --git a/test/review.test.ts b/test/review.test.ts index d882be9..718c05d 100644 --- a/test/review.test.ts +++ b/test/review.test.ts @@ -42,3 +42,19 @@ it('refuses no-change confirmation while the item still owns ambiguous changes', expect(view.items[1]!.count).toBeGreaterThan(0); expect(()=>service.act({action:'approve',item:'P2',token:view.token})).toThrow(/ambiguous/i); }); +it('anchors notes to server-owned code and rejects invalid ranges and cross-item references',()=>{ + const {service,config}=fixture();let view=service.load();const segment=view.segments.find(s=>s.row==='P1'&&s.kind!=='file'&&s.operation==='+')!; + const start=segment.newLine!; + expect(()=>service.act({action:'note',item:'P2',kind:'question',text:'Why?',reference:{key:segment.key,start,end:start},token:view.token})).toThrow(/Invalid snippet/); + expect(()=>service.act({action:'note',item:'P1',kind:'question',text:'Why?',reference:{key:segment.key,start:0,end:start},token:view.token})).toThrow(/changed block/); + view=service.act({action:'note',item:'P1',kind:'question',text:'Why?',reference:{key:segment.key,start,end:start,text:'forged',path:'forged'},token:view.token}); + const ref=view.notes[0]!.reference!;expect(ref.text).toBe(segment.content.split('\n')[0]);expect(ref.path).toBe(segment.path);expect(ref.head).toBe(view.snapshot.head);expect(view.notes[0]!.outdated).toBe(false); + const reopened=new ReviewService(config);services.push(reopened);expect(reopened.load().notes[0]!.reference).toEqual(ref); +}); +it('preserves removed-side snippets and marks their references outdated after HEAD changes',async()=>{ + const {service,config}=fixture();let view=service.load();const segment=view.segments.find(s=>s.row==='P1'&&s.kind!=='file'&&s.operation==='-')!; + view=service.act({action:'note',item:'P1',kind:'change',text:'Keep this?',reference:{key:segment.key,start:segment.oldLine,end:segment.oldLine},token:view.token}); + const ref=view.notes[0]!.reference!;expect(ref.side).toBe('old'); + const {execFileSync}=await import('node:child_process');execFileSync('git',['-c','core.hooksPath=/dev/null','commit','--allow-empty','-m','new revision'],{cwd:config.repository,stdio:'pipe'}); + view=service.load();expect(view.notes[0]!.outdated).toBe(true);expect(view.notes[0]!.reference).toEqual(ref); +}); diff --git a/web/public/app.js b/web/public/app.js index 92dfbf4..6998134 100644 --- a/web/public/app.js +++ b/web/public/app.js @@ -20,6 +20,8 @@ let data, since = false, busy = false; const drafts = new Map(); +const attachments = new Map(); +let snippetSelection = null; const statusClass = (text) => text.startsWith("✓") ? "good" @@ -46,6 +48,9 @@ function rememberDraft() { } function showFailure(message) { data = null; + snippetSelection = null; + $("selection-actions").hidden = true; + $("attachment").hidden = true; $("banner").textContent = message; $("progress").textContent = "Review unavailable"; $("item-title").textContent = "Review unavailable"; @@ -74,6 +79,7 @@ async function refresh() { try { rememberDraft(); data = await api("/api/review"); + snippetSelection = null; selected ??= data.items[0]?.id || "Unplanned"; since = data.items.find((item) => item.id === selected)?.state === "stale"; render(); @@ -103,6 +109,7 @@ async function act(command) { function select(id) { rememberDraft(); selected = id; + snippetSelection = null; change = 0; since = data.items.find((item) => item.id === id)?.state === "stale"; render(); @@ -201,7 +208,7 @@ function render() { .filter((note) => note.item === selected) .map( (note) => - `
You · ${note.kind === "change" ? "Change requested" : "Question"}

${esc(note.text)}

r${note.revision} · ${esc(new Date(note.createdAt).toLocaleString())}${note.kind === "change" ? " · Pending" : " · Awaiting discussion"}
`, + `
You · ${note.kind === "change" ? "Change requested" : "Question"}

${esc(note.text)}

${note.reference ? `
${esc(note.reference.text)}
` : ""}r${note.revision} · ${esc(new Date(note.createdAt).toLocaleString())}${note.kind === "change" ? " · Pending" : " · Awaiting discussion"}
`, ) .join("") || '

No conversation yet. Keep questions and requested changes beside the evidence.

' @@ -210,6 +217,19 @@ function render() { $("save-note").disabled = !item; $("message").value = drafts.get(`${selected}:${mode}`) || ""; renderCode(); + renderAttachment(); + document.querySelectorAll("[data-note]").forEach(button => button.onclick = () => { + const note = data.notes.find(note => note.id === button.dataset.note); + const ref = note.reference; + if (note.outdated) { + showDialog(`

! Outdated code reference

${esc(ref.path)} · ${ref.side} L${ref.start}–${ref.end}

Reviewed commit ${esc(ref.head)} · base ${esc(ref.base)}

${esc(ref.text)}
`); + return; + } + select(note.item); + const segment = data.segments.find(s => s.key === ref.key); + chooseSnippet(segment, ref.start, ref.end); + document.querySelector(`[data-segment="${ref.key}"]`)?.scrollIntoView({block:"center"}); + }); } function renderCode() { const item = data.items.find((item) => item.id === selected), @@ -263,7 +283,7 @@ function renderCode() { if (lines.at(-1) === "") lines.pop(); const start = segment.operation === "+" ? segment.newLine : segment.oldLine; - content = `
${esc(provenance)}
${lines.map((_, i) => (start === null ? "" : start + i)).join("\n")}
${esc(segment.operation)}
${esc(segment.content)}
`; + content = `
${esc(provenance)}
${lines.map((_, i) => ``).join("")}
${esc(segment.operation)}
${lines.map((line,i) => `${esc(line) || "​"}`).join("\n")}
`; } const choices = ["Unplanned", "Ambiguous"].includes(segment.row) ? `

Assigning this change makes the selected item’s approval stale.

` @@ -274,6 +294,14 @@ function renderCode() { (data.segments.length ? '

No changes in this row

There are no current segments here. An item with no changes requires explicit confirmation before approval.

' : '

No code changes yet

This branch has no changes against the selected base. Open task shows the compared commits.

')); + paintSelection(); + document.querySelectorAll("[data-line]").forEach(button => button.onclick = event => { + const key = button.closest("[data-segment]").dataset.segment; + const segment = data.segments.find(s => s.key === key); + const line = Number(button.dataset.line); + const anchor = event.shiftKey && snippetSelection?.key === key ? snippetSelection.anchor : line; + chooseSnippet(segment, Math.min(anchor,line), Math.max(anchor,line), anchor); + }); document.querySelectorAll("[data-assign]").forEach((button) => button.addEventListener("click", () => { const index = Number(button.dataset.assign); @@ -317,6 +345,7 @@ function setMode(value) { $("save-note").textContent = mode === "change" ? "Save change request" : "Save question"; $("message").value = drafts.get(`${selected}:${mode}`) || ""; + renderAttachment(); } function showDialog(html) { $("dialog-body").innerHTML = html; @@ -348,8 +377,12 @@ $("composer").onsubmit = async (event) => { event.preventDefault(); const item = selected, kind = mode; - if (await act({ action: "note", item, kind, text: $("message").value })) { + const attached = attachments.get(`${item}:${kind}`); + if (attached && (attached.head !== data.snapshot.head || attached.base !== data.snapshot.base)) return; + if (await act({ action: "note", item, kind, text: $("message").value, ...(attached ? {reference:{key:attached.key,start:attached.start,end:attached.end}} : {}) })) { drafts.delete(`${item}:${kind}`); + attachments.delete(`${item}:${kind}`); + renderAttachment(); $("message").value = ""; $("saved").textContent = kind === "change" @@ -431,3 +464,72 @@ document.addEventListener("keydown", (event) => { if (event.key === "?") $("help").click(); }); await refresh(); + +function chooseSnippet(segment, start, end, anchor = start) { + if (!data.items.some(item => item.id === selected)) { + $("saved").textContent = "Assign this change to a plan item before adding feedback."; + return; + } + const first = segment.operation === "+" ? segment.newLine : segment.oldLine; + const text = segment.content.split("\n").slice(start-first,end-first+1).join("\n"); + if (end-start >= 200 || text.length > 16000) { + $("saved").textContent = "Select at most 200 lines and 16000 characters."; + return; + } + snippetSelection = {key:segment.key,start,end,anchor,text,path:segment.operation === "-" ? segment.oldPath || segment.path : segment.path,side:segment.operation === "+" ? "new" : "old",head:data.snapshot.head,base:data.snapshot.base}; + paintSelection(); +} +function referenceLabel(ref) { + return `${ref.path} · ${ref.side === "new" ? "Added" : "Removed"} L${ref.start}–${ref.end}`; +} +function paintSelection() { + const ref = snippetSelection; + $("selection-actions").hidden = !ref; + $("selection-label").textContent = ref ? referenceLabel(ref) : ""; + document.querySelectorAll("[data-code-line], [data-line]").forEach(element => { + const line = Number(element.dataset.codeLine ?? element.dataset.line); + const active = !!ref && element.closest("[data-segment]").dataset.segment === ref.key && line >= ref.start && line <= ref.end; + element.classList.toggle("selected-line",active); + if (element.matches("button")) element.setAttribute("aria-pressed",String(active)); + }); +} +function renderAttachment() { + const ref = attachments.get(`${selected}:${mode}`); + $("attachment").hidden = !ref; + const stale = ref && (!data || ref.head !== data.snapshot.head || ref.base !== data.snapshot.base || !data.segments.some(s => s.key === ref.key && s.row === selected)); + $("attachment").innerHTML = ref ? `${esc(referenceLabel(ref))}Commit ${esc(ref.head.slice(0,8))}${stale ? " · ! Outdated — remove and select again" : ""}
${esc(ref.text)}
` : ""; + $("save-note").disabled = !!stale || !data?.items.some(item=>item.id === selected); + if (ref) $("remove-reference").onclick = () => {attachments.delete(`${selected}:${mode}`);renderAttachment();}; +} +function attachSelection(kind) { + if (!snippetSelection) return; + setMode(kind); + attachments.set(`${selected}:${mode}`, {...snippetSelection}); + document.body.classList.add("conversation-open"); + document.body.classList.remove("conversation-closed"); + renderAttachment(); + $("message").focus(); +} +$("snippet-ask").onclick = () => attachSelection("question"); +$("snippet-request").onclick = () => attachSelection("change"); +$("selection-clear").onclick = () => {snippetSelection=null;paintSelection();}; +function captureHighlightedLines() { + const selection = window.getSelection(); + if (!selection || selection.isCollapsed || !selection.rangeCount) return; + const range = selection.getRangeAt(0); + const element = node => node.nodeType === Node.ELEMENT_NODE ? node : node.parentElement; + const first = element(range.startContainer)?.closest("[data-code-line]"); + const last = element(range.endContainer)?.closest("[data-code-line]"); + if (!first || !last) return; + const block = first.closest("[data-segment]"); + if (block !== last.closest("[data-segment]")) { + snippetSelection=null;paintSelection(); + $("saved").textContent="Select lines within one changed block and diff side."; + return; + } + let end=Number(last.dataset.codeLine); + if (range.endOffset===0 && last!==first) end--; + chooseSnippet(data.segments.find(s=>s.key===block.dataset.segment),Number(first.dataset.codeLine),end); +} +$("code").addEventListener("mouseup",captureHighlightedLines); +$("code").addEventListener("keyup",captureHighlightedLines); diff --git a/web/public/index.html b/web/public/index.html index b530d7e..031514c 100644 --- a/web/public/index.html +++ b/web/public/index.html @@ -81,6 +81,7 @@

Linking changes to plan items…

> +
@@ -103,6 +104,7 @@

Linking changes to plan items…

+
diff --git a/web/public/style.css b/web/public/style.css index d9267fc..0ffb9e6 100644 --- a/web/public/style.css +++ b/web/public/style.css @@ -677,3 +677,6 @@ dialog::backdrop { #attachment small { display: block; color: var(--muted); } .snippet-preview { white-space: pre-wrap; overflow-wrap: anywhere; max-height: 180px; overflow: auto; font: 12px/1.5 "IBM Plex Mono", monospace; } .note-reference { max-width: 100%; height: auto; text-align: left; overflow-wrap: anywhere; } +.agent-answer { margin-top: 12px; padding-top: 8px; border-top: 1px solid var(--line); } +.agent-answer p { white-space: pre-wrap; overflow-wrap: anywhere; } +#question-provider { display: block; width: 100%; margin: 8px 0; } diff --git a/web/server.ts b/web/server.ts index d086f54..aebb3c0 100644 --- a/web/server.ts +++ b/web/server.ts @@ -3,9 +3,11 @@ import { readFileSync } from 'node:fs'; import { fileURLToPath } from 'node:url'; import { randomBytes, timingSafeEqual } from 'node:crypto'; import { ReviewService, type ReviewConfig } from '../runner/review.ts'; +import { Questions, type QuestionAgent } from '../runner/questions.ts'; const publicRoot = new URL('./public/', import.meta.url); -export async function startServer(config: ReviewConfig, port = 4318) { +export async function startServer(config: ReviewConfig, port = 4318, questionAgent?: QuestionAgent) { const service = new ReviewService(config), token = randomBytes(32).toString('hex'); + const questions=new Questions(service,questionAgent); const server = createServer(async (req, res) => { const address = server.address(); const actualPort = address && typeof address !== 'string' ? address.port : port; const origin = `http://127.0.0.1:${actualPort}`; @@ -18,12 +20,26 @@ export async function startServer(config: ReviewConfig, port = 4318) { if (path.startsWith('/api/')) { const supplied = req.headers['x-codeboost-token']; if (typeof supplied !== 'string' || !/^[a-f0-9]{64}$/.test(supplied) || !timingSafeEqual(Buffer.from(supplied), Buffer.from(token))) { json(403, { error: 'Open the private local URL printed by the CLI.' }); return; } + if (req.method === 'GET' && path === '/api/settings') { json(200,{questionProvider:service.store.questionProvider()});return; } + if (req.method === 'GET' && path === '/api/questions') { json(200,{notes:service.load().notes});return; } if (req.method === 'GET' && path === '/api/review') { json(200, service.load()); return; } - if (req.method !== 'POST' || path !== '/api/action' || req.headers['content-type'] !== 'application/json') { json(405, { error: 'Unsupported request.' }); return; } + if (req.method !== 'POST' || !['/api/action','/api/settings'].includes(path) || req.headers['content-type'] !== 'application/json') { json(405, { error: 'Unsupported request.' }); return; } const chunks: Buffer[] = []; let size = 0; for await (const chunk of req) { size += chunk.length; if (size > 16384) { json(413, { error: 'Request too large.' }); return; } chunks.push(chunk); } const body = new TextDecoder('utf-8', { fatal: true }).decode(Buffer.concat(chunks)); - json(200, service.act(JSON.parse(body))); return; + const input=JSON.parse(body); + if(path==='/api/settings') {service.store.setQuestionProvider(input.questionProvider);json(200,{questionProvider:service.store.questionProvider()});return;} + if(input.action==='retry-question') { + const view=service.load();if(input.token!==view.token)throw new Error('Stale review state. Refresh and retry.'); + questions.start(input.id,view);json(200,service.load());return; + } + const view=service.act(input); + if(view.createdNoteId && input.kind==='question') { + try {questions.start(view.createdNoteId,view);} catch(error) { + // The saved question remains visible and retryable when capacity is reached. + } + } + json(200,service.load());return; } if (req.method !== 'GET') { json(405, { error: 'Method not allowed.' }); return; } const files: Record = { '/': ['index.html', 'text/html'], '/app.js': ['app.js', 'text/javascript'], '/style.css': ['style.css', 'text/css'] }; @@ -40,5 +56,5 @@ export async function startServer(config: ReviewConfig, port = 4318) { server.requestTimeout = 15000; await new Promise((resolve, reject) => { server.once('error', reject); server.listen(port, '127.0.0.1', () => { server.removeListener('error', reject); resolve(); }); }).catch(error => { service.close(); throw error; }); const address = server.address(); if (!address || typeof address === 'string') throw new Error('Cannot determine local address.'); - return { server, service, token, url: `http://127.0.0.1:${address.port}/#${token}`, close: () => new Promise((resolve, reject) => server.close(error => { service.close(); error ? reject(error) : resolve(); })) }; + return { server, service, token, url: `http://127.0.0.1:${address.port}/#${token}`, close: async () => { await questions.close(); await new Promise((resolve, reject) => server.close(error => { service.close(); error ? reject(error) : resolve(); })); } }; } From fabe0cd60f543854886b9fefb388f0b74a24e432 Mon Sep 17 00:00:00 2001 From: mchwang Date: Wed, 23 Sep 2026 13:40:11 -0700 Subject: [PATCH 03/23] Show immediate feedback when submitting review questions --- docs/implementation/read-only-review.md | 2 ++ test/browser/review.spec.ts | 17 +++++++++++++++++ web/public/app.js | 11 ++++++++++- 3 files changed, 29 insertions(+), 1 deletion(-) diff --git a/docs/implementation/read-only-review.md b/docs/implementation/read-only-review.md index 3ab9ebe..1e63430 100644 --- a/docs/implementation/read-only-review.md +++ b/docs/implementation/read-only-review.md @@ -74,3 +74,5 @@ The CLI adapter runs without a shell in a fresh temporary directory. Claude uses Codex options were checked against the installed CLI help and the official [non-interactive documentation](https://learn.chatgpt.com/docs/non-interactive-mode) and [configuration reference](https://learn.chatgpt.com/docs/config-file/config-reference). Both installed providers passed live connection checks. A separate copy of PR #597's review database passed a real Settings → Ask → saved Claude answer browser test; the user's review state and source checkout were unchanged. Native Node startup is covered by enabling TypeScript's erasableSyntaxOnly check after the live test caught an unsupported parameter-property declaration. Validation: 181 unit/integration tests, 19 browser tests, and typecheck pass. A delayed-initial-review regression reproduced an unresponsive Settings button; binding controls before awaiting the first load fixes it. The restarted PR #597 instance also passed an immediate Settings-open check with both provider options visible. + +Single-click submission follow-up: a held `/api/action` browser request reproduced the lack of immediate feedback (the submit button stayed enabled with no saving state). Question/change submission now shows Saving immediately, disables submission while a review action or refresh is in progress, and scrolls the saved note into view. Failures explicitly retain the draft for refresh/retry. The delayed-request regression checks one click produces exactly one stored question and one agent invocation; a lost first click was not independently reproduced. diff --git a/test/browser/review.spec.ts b/test/browser/review.spec.ts index 5e81871..ed29988 100644 --- a/test/browser/review.spec.ts +++ b/test/browser/review.spec.ts @@ -134,3 +134,20 @@ test('opens Settings while the initial review is still loading',async({page})=>{ await expect(page.getByLabel('Question agent',{exact:true})).toBeVisible(); } finally {release();} }); +test('acknowledges the first Ask agent click immediately and prevents duplicate submissions',async({page})=>{ + const config=app.service.config;await app.close();let calls=0; + app=await startServer(config,0,async()=>{calls++;return 'Single-click answer';}); + await page.goto(app.url);await expect(page.getByRole('heading',{name:'Bound exponential retries'})).toBeVisible(); + let release!:()=>void;const pending=new Promise(resolve=>{release=resolve;});let submissions=0; + await page.route('**/api/action',async route=>{submissions++;await pending;await route.continue();}); + try { + await page.getByLabel('Question about this item').fill('Does one click submit this?'); + await page.getByRole('button',{name:'Ask agent',exact:true}).click(); + await expect(page.locator('#save-note')).toBeDisabled(); + await expect(page.locator('#saved')).toHaveText('Saving question…'); + } finally {release();} + await expect(page.getByText('Does one click submit this?',{exact:true})).toBeVisible(); + await expect(page.getByText('Single-click answer',{exact:true})).toBeVisible({timeout:10000}); + await expect(page.getByRole('button',{name:'Ask agent',exact:true})).toBeEnabled(); + expect(submissions).toBe(1);expect(calls).toBe(1);expect(app.service.store.getReviewNotes(config.identity)).toHaveLength(1); +}); diff --git a/web/public/app.js b/web/public/app.js index 90748c2..60b811c 100644 --- a/web/public/app.js +++ b/web/public/app.js @@ -75,6 +75,7 @@ function showFailure(message) { async function refresh() { if (busy) return; busy = true; + renderAttachment(); $("banner").textContent = "Linking changes to plan items…"; try { rememberDraft(); @@ -89,11 +90,13 @@ async function refresh() { ); } finally { busy = false; + renderAttachment(); } } async function act(command) { if (busy || !data) return false; busy = true; + renderAttachment(); try { rememberDraft(); data = await api("/api/action", { ...command, token: data.token }); @@ -104,6 +107,7 @@ async function act(command) { return false; } finally { busy = false; + renderAttachment(); } } function select(id) { @@ -355,11 +359,14 @@ $("ask").onclick = () => setMode("question"); $("request").onclick = () => setMode("change"); $("composer").onsubmit = async (event) => { event.preventDefault(); + if (busy || !data) return; const item = selected, kind = mode; const attached = attachments.get(`${item}:${kind}`); if (attached && (attached.head !== data.snapshot.head || attached.base !== data.snapshot.base)) return; + $("saved").textContent = kind === "change" ? "Saving change request…" : "Saving question…"; if (await act({ action: "note", item, kind, text: $("message").value, ...(attached ? {reference:{key:attached.key,start:attached.start,end:attached.end}} : {}) })) { + $("notes").lastElementChild?.scrollIntoView({ block: "nearest" }); drafts.delete(`${item}:${kind}`); attachments.delete(`${item}:${kind}`); renderAttachment(); @@ -368,6 +375,8 @@ $("composer").onsubmit = async (event) => { kind === "change" ? "Saved for the next revision." : "Question saved. See agent status in Conversation."; + } else { + $("saved").textContent = "Could not save. Your draft is preserved; refresh and try again."; } }; $("conversation-toggle").onclick = () => { @@ -477,7 +486,7 @@ function renderAttachment() { $("attachment").hidden = !ref; const stale = ref && (!data || ref.head !== data.snapshot.head || ref.base !== data.snapshot.base || !data.segments.some(s => s.key === ref.key && s.row === selected)); $("attachment").innerHTML = ref ? `${esc(referenceLabel(ref))}Commit ${esc(ref.head.slice(0,8))}${stale ? " · ! Outdated — remove and select again" : ""}
${esc(ref.text)}
` : ""; - $("save-note").disabled = !!stale || !data?.items.some(item=>item.id === selected); + $("save-note").disabled = busy || !!stale || !data?.items.some(item=>item.id === selected); if (ref) $("remove-reference").onclick = () => {attachments.delete(`${selected}:${mode}`);renderAttachment();}; } function attachSelection(kind) { From 3b3cfbf9862ff44003950a52c35caa09b4fcea90 Mon Sep 17 00:00:00 2001 From: mchwang Date: Wed, 23 Sep 2026 13:53:47 -0700 Subject: [PATCH 04/23] Follow new agent answers in the conversation scroll pane --- docs/implementation/read-only-review.md | 2 ++ test/browser/review.spec.ts | 14 ++++++++++++++ web/public/app.js | 10 +++++++--- 3 files changed, 23 insertions(+), 3 deletions(-) diff --git a/docs/implementation/read-only-review.md b/docs/implementation/read-only-review.md index 1e63430..a69908e 100644 --- a/docs/implementation/read-only-review.md +++ b/docs/implementation/read-only-review.md @@ -76,3 +76,5 @@ Codex options were checked against the installed CLI help and the official [non- Validation: 181 unit/integration tests, 19 browser tests, and typecheck pass. A delayed-initial-review regression reproduced an unresponsive Settings button; binding controls before awaiting the first load fixes it. The restarted PR #597 instance also passed an immediate Settings-open check with both provider options visible. Single-click submission follow-up: a held `/api/action` browser request reproduced the lack of immediate feedback (the submit button stayed enabled with no saving state). Question/change submission now shows Saving immediately, disables submission while a review action or refresh is in progress, and scrolls the saved note into view. Failures explicitly retain the draft for refresh/retry. The delayed-request regression checks one click produces exactly one stored question and one agent invocation; a lost first click was not independently reproduced. + +Answer scrolling: when an asynchronous answer or failure arrives, Conversation follows the bottom if the reader was already within 32 pixels of it. Scrolling up keeps the current position through polling updates. Only the conversation list scrolls; the code pane and composer stay in place. Browser regressions cover both arrival at the bottom and arrival while reading earlier messages; the bottom-follow test failed before the fix with a 1,302-pixel gap. diff --git a/test/browser/review.spec.ts b/test/browser/review.spec.ts index ed29988..06a2859 100644 --- a/test/browser/review.spec.ts +++ b/test/browser/review.spec.ts @@ -151,3 +151,17 @@ test('acknowledges the first Ask agent click immediately and prevents duplicate await expect(page.getByRole('button',{name:'Ask agent',exact:true})).toBeEnabled(); expect(submissions).toBe(1);expect(calls).toBe(1);expect(app.service.store.getReviewNotes(config.identity)).toHaveLength(1); }); +for (const readingEarlier of [false,true]) test(`answer arrival ${readingEarlier ? 'preserves earlier reading position' : 'follows the conversation bottom'}`,async({page})=>{ + const config=app.service.config;await app.close();let answer!:(text:string)=>void; + app=await startServer(config,0,()=>new Promise(resolve=>{answer=resolve;})); + for(let i=0;i<3;i++) app.service.act({action:'note',item:'P1',kind:'change',text:`Earlier message ${i}\n`+'Earlier context.\n'.repeat(20),token:app.service.load().token}); + await page.goto(app.url);await page.getByLabel('Question about this item').fill('Explain the cap');await page.getByRole('button',{name:'Ask agent',exact:true}).click(); + await expect(page.getByText('Agent · Answering…',{exact:true})).toBeVisible(); + const notes=page.locator('#notes'); + await notes.evaluate((element,earlier)=>{element.scrollTop=earlier ? 80 : element.scrollHeight;},readingEarlier); + const before=await notes.evaluate(element=>element.scrollTop); + answer('Detailed answer.\n'.repeat(60)+'Answer end.'); + await expect(page.locator('.agent-answer')).toHaveCount(1,{timeout:10000}); + if(readingEarlier) expect(await notes.evaluate(element=>element.scrollTop)).toBeCloseTo(before,0); + else await expect.poll(()=>notes.evaluate(element=>element.scrollHeight-element.clientHeight-element.scrollTop)).toBeLessThan(2); +}); diff --git a/web/public/app.js b/web/public/app.js index 60b811c..021e406 100644 --- a/web/public/app.js +++ b/web/public/app.js @@ -530,7 +530,10 @@ function answerMarkup(note) { const error=answer?.status==="failed"?answer.error:answer?.status==="pending"?"Agent was interrupted or timed out.":"Answer not started. Choose an agent in Settings or retry when capacity is available."; return `

! ${esc(error)}

`; } -function renderNotes() { +function renderNotes({ follow = false } = {}) { + const notes = $("notes"); + const scrollTop = notes.scrollTop; + const atBottom = notes.scrollHeight - notes.clientHeight - scrollTop <= 32; const item=data.items.find(item=>item.id===selected); $("notes").innerHTML = item ? data.notes @@ -542,6 +545,7 @@ function renderNotes() { .join("") || '

No conversation yet. Keep questions and requested changes beside the evidence.

' : '

Select a plan item to add a question or request a change.

'; + if (follow) notes.scrollTop = atBottom ? notes.scrollHeight : scrollTop; document.querySelectorAll("[data-note]").forEach(button => button.onclick = () => { const note = data.notes.find(note => note.id === button.dataset.note); const ref = note.reference; @@ -561,10 +565,10 @@ setInterval(async()=>{ if(pollingQuestions || busy || !data || !data.notes.some(n=>n.answer?.status==="pending")) return; if(data.notes.every(n=>n.answer?.status!=="pending" || n.answer.expiresAt<=Date.now())) { data.notes=data.notes.map(n=>n.answer?.status==="pending"?{...n,answer:{...n.answer,status:"failed",error:"Agent was interrupted or timed out. Retry the question."}}:n); - renderNotes();return; + renderNotes({ follow: true });return; } pollingQuestions=true; - try {const response=await api("/api/questions");if(data){data.notes=response.notes;renderNotes();}} + try {const response=await api("/api/questions");if(data){data.notes=response.notes;renderNotes({ follow: true });}} catch { $("saved").textContent="Could not refresh agent answers. Use Refresh to reconnect."; } finally {pollingQuestions=false;} },2000); From 8131f0cdfdfcdd4f636929c5fab4f919ba9910cc Mon Sep 17 00:00:00 2001 From: mchwang Date: Wed, 23 Sep 2026 13:57:24 -0700 Subject: [PATCH 05/23] Add a draggable conversation pane divider --- docs/implementation/read-only-review.md | 2 ++ test/browser/review.spec.ts | 12 ++++++++ web/public/app.js | 39 +++++++++++++++++++++++++ web/public/index.html | 3 +- web/public/style.css | 17 ++++++++++- 5 files changed, 71 insertions(+), 2 deletions(-) diff --git a/docs/implementation/read-only-review.md b/docs/implementation/read-only-review.md index a69908e..0b7390e 100644 --- a/docs/implementation/read-only-review.md +++ b/docs/implementation/read-only-review.md @@ -78,3 +78,5 @@ Validation: 181 unit/integration tests, 19 browser tests, and typecheck pass. A Single-click submission follow-up: a held `/api/action` browser request reproduced the lack of immediate feedback (the submit button stayed enabled with no saving state). Question/change submission now shows Saving immediately, disables submission while a review action or refresh is in progress, and scrolls the saved note into view. Failures explicitly retain the draft for refresh/retry. The delayed-request regression checks one click produces exactly one stored question and one agent invocation; a lost first click was not independently reproduced. Answer scrolling: when an asynchronous answer or failure arrives, Conversation follows the bottom if the reader was already within 32 pixels of it. Scrolling up keeps the current position through polling updates. Only the conversation list scrolls; the code pane and composer stay in place. Browser regressions cover both arrival at the bottom and arrival while reading earlier messages; the bottom-follow test failed before the fix with a 1,302-pixel gap. + +Conversation width: drag the divider on its left edge to resize between 280 and 480 pixels. Focus the divider and use Left/Right for 20-pixel steps or Home/End for the limits. The width survives collapsing/reopening the pane within the page; reloading uses the layout default. This replaces the hard-to-discover native corner resize grip. The browser regression covers pointer dragging, keyboard limits, code-pane space, and collapse/reopen. diff --git a/test/browser/review.spec.ts b/test/browser/review.spec.ts index 06a2859..0e3d74f 100644 --- a/test/browser/review.spec.ts +++ b/test/browser/review.spec.ts @@ -165,3 +165,15 @@ for (const readingEarlier of [false,true]) test(`answer arrival ${readingEarlier if(readingEarlier) expect(await notes.evaluate(element=>element.scrollTop)).toBeCloseTo(before,0); else await expect.poll(()=>notes.evaluate(element=>element.scrollHeight-element.clientHeight-element.scrollTop)).toBeLessThan(2); }); +test('resizes Conversation using its divider and keyboard',async({page})=>{ + await page.goto(app.url);const pane=page.locator('#conversation-pane');const divider=page.getByRole('separator',{name:'Resize conversation'}); + await expect(divider).toBeVisible();const initial=(await pane.boundingBox())!.width;const handle=(await divider.boundingBox())!; + await page.mouse.move(handle.x+handle.width/2,handle.y+100);await page.mouse.down();await page.mouse.move(handle.x-100,handle.y+100);await page.mouse.up(); + expect((await pane.boundingBox())!.width).toBeGreaterThan(initial+90); + await divider.press('Home');await expect(divider).toHaveAttribute('aria-valuenow','280'); + await divider.press('ArrowLeft');await expect(divider).toHaveAttribute('aria-valuenow','300'); + await divider.press('End');await expect(divider).toHaveAttribute('aria-valuenow','480'); + expect((await page.locator('.code-pane').boundingBox())?.width ?? 0).toBeGreaterThan(400); + await page.getByRole('button',{name:'Collapse conversation',exact:true}).click();await expect(divider).toBeHidden(); + await page.getByRole('button',{name:'Conversation',exact:true}).click();await expect(divider).toBeVisible();expect((await pane.boundingBox())!.width).toBe(480); +}); diff --git a/web/public/app.js b/web/public/app.js index 021e406..4ef105c 100644 --- a/web/public/app.js +++ b/web/public/app.js @@ -582,4 +582,43 @@ $("settings").onclick=async()=>{ } catch(error){$("dialog-body").textContent=error.message;} }; +const conversationResize = $("conversation-resize"); +const conversationPane = $("conversation-pane"); +function resizeConversation(width) { + const bounded = Math.round(Math.max(280, Math.min(480, width))); + conversationPane.style.width = `${bounded}px`; + conversationResize.setAttribute("aria-valuenow", String(bounded)); +} +let conversationDrag; +conversationResize.onpointerdown = event => { + if (event.button !== 0) return; + event.preventDefault(); + conversationDrag = { x: event.clientX, width: conversationPane.getBoundingClientRect().width }; + conversationResize.setPointerCapture(event.pointerId); + conversationResize.focus(); + document.body.classList.add("resizing-conversation"); +}; +conversationResize.onpointermove = event => { + if (conversationDrag) resizeConversation(conversationDrag.width + conversationDrag.x - event.clientX); +}; +function endConversationResize() { + conversationDrag = null; + document.body.classList.remove("resizing-conversation"); +} +conversationResize.onpointerup = event => { + if (conversationResize.hasPointerCapture(event.pointerId)) conversationResize.releasePointerCapture(event.pointerId); + endConversationResize(); +}; +conversationResize.onpointercancel = endConversationResize; +conversationResize.onlostpointercapture = endConversationResize; +conversationResize.onkeydown = event => { + if (!["ArrowLeft", "ArrowRight", "Home", "End"].includes(event.key)) return; + event.preventDefault(); + resizeConversation(event.key === "Home" ? 280 : event.key === "End" ? 480 : conversationPane.getBoundingClientRect().width + (event.key === "ArrowLeft" ? 20 : -20)); +}; +new ResizeObserver(() => { + const width = conversationPane.getBoundingClientRect().width; + if (width) conversationResize.setAttribute("aria-valuenow", String(Math.round(width))); +}).observe(conversationPane); + await refresh(); diff --git a/web/public/index.html b/web/public/index.html index d8dd6b3..701c7e6 100644 --- a/web/public/index.html +++ b/web/public/index.html @@ -85,7 +85,8 @@

Linking changes to plan items…

-