Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions docs/implementation/read-only-review.md
Original file line number Diff line number Diff line change
Expand Up @@ -110,3 +110,13 @@ The documentation-only review after adding `AGENTS.md` exposed one more concrete
The follow-up review exposed the same active-job marker missing from the initial review response. Reloading during provider settlement could therefore expose Retry and stop polling. Initial loads and action responses now include the marker used by question polling; the browser regression reloads during settlement and confirms Retry remains hidden until the invocation finishes.

The next review made assignment drift concrete for item-level answers without snippet references. Each answer now records a hash of the changed segments supplied for its item. Moving code into or out of that item preserves the answer but labels it as earlier review context and rejects retrying the old question; a regression assigns new code without changing the snapshot or plan revision, checks the historical marker, and proves no second agent invocation starts.

## Refresh draft preservation (#13)

Held-response browser regressions reproduced Refresh replacing newer question/change text with the pre-request draft, losing text entered after switching items and modes, and hiding a draft when its item was removed. Five cases failed before the fix; the failed-refresh recovery case already passed. All six focused cases pass after capturing the current draft immediately before applying the response and on failure.

The browser owns unsent text, mode, navigation, and attachments; the refreshed response owns the persisted plan, approvals, notes, and snapshot. Refresh remains serialized with review actions and invalidates older question polls through the existing generation counter. A valid attached snippet survives; changed snapshot or assignment context keeps the text and marks the attachment outdated, blocking submission until it is removed or reselected. Removed items with drafts remain accessible as retained-draft rows, with submission disabled and instructions to copy the text to a current item. These drafts remain page-local and do not survive a browser reload.

The regression gate is `npm run test:browser -- --grep 'during refresh'`. It checks visible drafts and refreshed durable approval/snapshot/plan state, and confirms no draft was accidentally saved as a note. Issues #10 (review edge cases), #12 (polling efficiency), and #3 (human go/no-go experiment) remain separate work.

PR #14 review round 1 found that retained-draft rows omitted the regular rows' `aria-current` state. A browser assertion reproduced the missing attribute. Retained rows now expose their selected state; the regression checks selection, navigation away, and selection again. No findings were declined.
63 changes: 63 additions & 0 deletions test/browser/review.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -236,6 +236,69 @@ test('outdated questions explain how to continue without offering a broken retry
await expect(page.getByText('This question refers to an earlier review. Ask again against the current code.',{exact:true})).toBeVisible();
await expect(page.getByRole('heading',{name:'Bound exponential retries'})).toBeVisible();
});
for(const kind of ['question','change']) test(`preserves ${kind} edits and snippets during refresh`,async({page})=>{
await page.goto(app.url);await expect(page.getByRole('heading',{name:'Bound exponential retries'})).toBeVisible();
await page.locator('.added [data-line]').first().click();
await page.getByRole('button',{name:kind==='question'?'Ask about selection':'Request change to selection',exact:true}).click();
const composer=page.locator('#message');await composer.fill('Before refresh');
let release!:()=>void,arrived!:()=>void;const held=new Promise<void>(resolve=>release=resolve),started=new Promise<void>(resolve=>arrived=resolve);
await page.route('**/api/review',async route=>{arrived();await held;await route.continue();});
await page.getByRole('button',{name:'Refresh',exact:true}).click();await started;
await composer.fill('Edited during refresh');
app.service.act({action:'approve',item:'P3',confirmNoChange:true,token:app.service.load().token});
release();await expect(page.getByText('1 of 3 approved')).toBeVisible();
await expect(composer).toHaveValue('Edited during refresh');
await expect(page.locator(kind==='question'?'#ask':'#request')).toHaveAttribute('aria-pressed','true');
await expect(page.locator('#attachment')).toContainText('retry.ts');await expect(page.locator('#attachment')).not.toContainText('Outdated');
await expect(page.locator('#save-note')).toBeEnabled();
expect(app.service.load().approved).toBe(1);expect(app.service.load().notes).toHaveLength(0);
});
for(const scenario of ['navigation','snapshot','removed item','failure']) test(`preserves drafts during refresh with ${scenario}`,async({page})=>{
await page.goto(app.url);await expect(page.getByRole('heading',{name:'Bound exponential retries'})).toBeVisible();
await page.locator('.added [data-line]').first().click();await page.getByRole('button',{name:'Ask about selection',exact:true}).click();
await page.locator('#message').fill('Before refresh');
let release!:()=>void,arrived!:()=>void;const held=new Promise<void>(resolve=>release=resolve),started=new Promise<void>(resolve=>arrived=resolve);
await page.route('**/api/review',async route=>{arrived();await held;if(scenario==='failure')await route.fulfill({status:500,json:{error:'Temporary read failure'}});else await route.continue();});
await page.getByRole('button',{name:'Refresh',exact:true}).click();await started;
await page.locator('#message').fill('Latest question');
if(scenario==='navigation'){
await page.getByRole('button',{name:/P2 Document retry behavior/}).click();
await page.getByRole('button',{name:'Request change',exact:true}).click();await page.locator('#message').fill('Latest change request');
}
if(scenario==='snapshot')execFileSync('git',['-c','core.hooksPath=/dev/null','commit','--allow-empty','-m','New snapshot'],{cwd:app.service.config.repository,stdio:'pipe'});
if(scenario==='removed item'){
const service=app.service,identity=service.config.identity,plan=service.store.getPlan(identity);
service.store.importRevision(JSON.stringify({...plan,items:plan.items.filter(item=>item.id!=='P1').map(item=>({...item,depends_on:[]}))}),'json',{identity,issue:plan.issue,baseEntries:['retry.ts','README.md','run.sh'].map(path=>({path,kind:'file' as const})),pathKey:path=>path,allowedCommands:[]},plan.revision);
}
release();
if(scenario==='failure'){
await expect(page.locator('#banner')).toContainText('Temporary read failure');await page.unroute('**/api/review');
await page.getByRole('button',{name:'Refresh',exact:true}).click();
}
await expect(page.locator('#banner')).not.toContainText('Linking changes');await expect(page.locator('#banner')).not.toContainText('Temporary read failure');
if(scenario==='navigation'){
await expect(page.getByRole('heading',{name:'Document retry behavior'})).toBeVisible();
await expect(page.locator('#message')).toHaveValue('Latest change request');await expect(page.locator('#request')).toHaveAttribute('aria-pressed','true');
await page.getByRole('button',{name:/P1 Bound exponential retries/}).click();await page.getByRole('button',{name:'Ask',exact:true}).click();
}
await expect(page.locator('#message')).toHaveValue('Latest question');
if(scenario==='snapshot'){
await expect(page.locator('#attachment')).toContainText('Outdated');await expect(page.locator('#save-note')).toBeDisabled();
expect(app.service.load().snapshot.head).toBe(execFileSync('git',['rev-parse','HEAD'],{cwd:app.service.config.repository,encoding:'utf8'}).trim());
}
if(scenario==='removed item'){
await expect(page.locator('#item-details')).toContainText('no longer in the plan');await expect(page.locator('#save-note')).toBeDisabled();
await expect(page.getByRole('button',{name:'! P1 · Retained draft',exact:true})).toHaveAttribute('aria-current','true');
await expect(page.getByRole('button',{name:/P1 Bound exponential retries/})).toHaveCount(0);
await page.getByRole('button',{name:/P2 Document retry behavior/}).click();
await expect(page.getByRole('button',{name:'! P1 · Retained draft',exact:true})).toHaveAttribute('aria-current','false');
await page.getByRole('button',{name:'! P1 · Retained draft',exact:true}).click();
await expect(page.getByRole('button',{name:'! P1 · Retained draft',exact:true})).toHaveAttribute('aria-current','true');
await expect(page.locator('#message')).toHaveValue('Latest question');await expect(page.locator('#message')).toBeEnabled();
expect(app.service.load().items.map(item=>item.id)).toEqual(['P2','P3']);
}
expect(app.service.load().notes).toHaveLength(0);
});
for(const switchItem of [false,true]) test(`preserves edits made while a question submission is in flight (switch item: ${switchItem})`,async({page})=>{
await page.goto(app.url);await expect(page.getByRole('heading',{name:'Bound exponential retries'})).toBeVisible();
let release!:()=>void;const held=new Promise<void>(resolve=>release=resolve);
Expand Down
20 changes: 15 additions & 5 deletions web/public/app.js
Original file line number Diff line number Diff line change
Expand Up @@ -81,12 +81,15 @@ async function refresh() {
$("banner").textContent = "Linking changes to plan items…";
try {
rememberDraft();
data = await api("/api/review");
const updated = await api("/api/review");
rememberDraft();
data = updated;
snippetSelection = null;
selected ??= data.items[0]?.id || "Unplanned";
since = data.items.find((item) => item.id === selected)?.state === "stale";
render();
} catch (error) {
rememberDraft();
showFailure(
`Could not read this branch’s history. ${error.message} Use Refresh to retry.`,
);
Expand Down Expand Up @@ -126,7 +129,12 @@ function select(id) {
}
function render() {
const item = data.items.find((item) => item.id === selected);
if (!item && !["Unplanned", "Ambiguous", "Accepted"].includes(selected)) {
const retainedItems = [...new Set([
...[...drafts].filter(([, text]) => text.length).map(([key]) => key.split(":")[0]),
...[...attachments.keys()].map((key) => key.split(":")[0]),
])].filter((id) => !data.items.some((entry) => entry.id === id) && !["Unplanned", "Ambiguous", "Accepted"].includes(id));
const retained = retainedItems.includes(selected);
if (!item && !retained && !["Unplanned", "Ambiguous", "Accepted"].includes(selected)) {
selected = data.items[0]?.id || "Unplanned";
return render();
}
Expand Down Expand Up @@ -164,7 +172,9 @@ function render() {
(row) =>
`<button class="plan-row ${row === selected ? "selected" : ""}" data-select="${row}"><span class="row-title ${row === "Unplanned" ? "bad" : row === "Ambiguous" ? "warn" : "muted"}">${row === "Unplanned" ? "✕" : row === "Ambiguous" ? "!" : "✓"} ${row}${row === "Unplanned" ? " changes" : ""}<span class="count">${data.segments.filter((s) => s.row === row).length}</span></span></button>`,
)
.join("");
.join("") + retainedItems.map((id) =>
`<button class="plan-row ${id === selected ? "selected" : ""}" data-select="${esc(id)}" aria-current="${id === selected ? "true" : "false"}"><span class="warn">! ${esc(id)} · Retained draft</span></button>`,
).join("");
document
.querySelectorAll("[data-select]")
.forEach((button) =>
Expand All @@ -174,7 +184,7 @@ function render() {
$("item-title").textContent = item?.title || selected;
$("item-details").innerHTML = item
? `<p>${esc(item.intent)}</p>${item.reasons.map((reason) => `<p class="warn">! Stale: ${esc(reason)}</p>`).join("")}`
: "";
: retained ? '<p class="warn">This item is no longer in the plan. Your draft is retained in this page. Copy the text, select a current plan item, and reselect any code before submitting.</p>' : "";
$("approve").hidden = !item;
$("approve").textContent = item?.ambiguousCount
? "Resolve ambiguous changes"
Expand Down Expand Up @@ -214,7 +224,7 @@ function render() {
}),
);
renderNotes();
$("message").disabled = !item;
$("message").disabled = !item && !retained;
$("save-note").disabled = !item;
$("message").value = drafts.get(`${selected}:${mode}`) || "";
renderCode();
Expand Down
Loading