diff --git a/test/browser/first-load.spec.ts b/test/browser/first-load.spec.ts new file mode 100644 index 00000000..af380001 --- /dev/null +++ b/test/browser/first-load.spec.ts @@ -0,0 +1,234 @@ +import { test, expect, type Page } from '@playwright/test'; +import { mkdtempSync, rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { createDemo } from '../../scripts/demo.ts'; +import { startServer } from '../../web/server.ts'; + +// These tests act while the page's first review load is still pending, so they must not use openReview/reloadReview, +// which wait for that load to finish. +const LOADING = 'Linking changes to plan items…'; + +let root: string, app: Awaited>; +test.beforeEach(async () => { + root = mkdtempSync(join(tmpdir(), 'codeboost-first-load-browser-')); + app = await startServer(createDemo(join(root, 'demo')), 0); +}); +test.afterEach(async () => { + await app.close(); + rmSync(root, { recursive: true, force: true }); +}); + +/** Opens the page with its first /api/review request held in the browser until the test calls `release`. */ +async function openWithHeldReview(page: Page, url: string, answer: 'pass' | 'fail' = 'pass') { + let release!: () => void, arrived!: () => void; + const held = new Promise(resolve => { release = resolve; }); + const reached = new Promise(resolve => { arrived = resolve; }); + await page.route('**/api/review', async route => { + arrived(); + await held; + if (answer === 'fail') await route.fulfill({ status: 500, contentType: 'application/json', body: JSON.stringify({ error: 'History unavailable.' }) }); + else await route.continue(); + }, { times: 1 }); + await page.goto(url, { waitUntil: 'commit' }); + await reached; + await expect(page.locator('#banner')).toHaveText(LOADING); + return release; +} + +// app.url carries the session token in its hash; the view goes in the query before it. +function plansUrl() { + const url = new URL(app.url); + url.search = '?view=plans'; + return url.href; +} + +function countRequests(page: Page, path: string) { + const seen: string[] = []; + page.on('request', request => { if (new URL(request.url()).pathname === path) seen.push(request.method()); }); + return seen; +} + +const nextPlan = { + schema_version: 1, + issue: 3, + revision: 2, + summary: 'Make retry limits visible to operators', + items: [{ + id: 'P1', + title: 'Expose the retry ceiling', + intent: 'Keep the bounded delay and make its ceiling explicit.', + files: [{ path: 'retry.ts', kind: 'edit', renamed_from: null, change: 'Name the retry ceiling.' }], + acceptance: [{ type: 'check', text: 'The delay never exceeds five seconds.' }], + depends_on: [], + }], + questions: [], +}; + +test('keeps question and change-request text typed before the first review load and gives it to the first item', async ({ page }) => { + const errors: string[] = []; + page.on('pageerror', error => errors.push(error.message)); + const actions = countRequests(page, '/api/action'); + const release = await openWithHeldReview(page, app.url); + + await page.getByLabel('Question about this item').fill('Typed before the review loaded.'); + await page.getByRole('button', { name: 'Request change', exact: true }).click(); + await page.getByLabel('Change to request').fill('Change typed before the review loaded.'); + await page.getByRole('button', { name: 'Ask', exact: true }).click(); + await expect(page.getByLabel('Question about this item')).toHaveValue('Typed before the review loaded.'); + // Typed after the last mode switch: only the page's handling of the response can save this part. + await page.getByLabel('Question about this item').press('End'); + await page.getByLabel('Question about this item').pressSequentially(' Still typing.'); + + release(); + await expect(page.locator('#banner')).not.toHaveText(LOADING, { timeout: 30_000 }); + await expect(page.locator('[data-select="P1"]')).toHaveAttribute('aria-current', 'true'); + await expect(page.getByLabel('Question about this item')).toHaveValue('Typed before the review loaded. Still typing.'); + await expect(page.getByLabel('Question about this item')).toBeFocused(); + await expect(page.getByText('Retained draft')).toHaveCount(0); + await page.getByRole('button', { name: 'Request change', exact: true }).click(); + await expect(page.getByLabel('Change to request')).toHaveValue('Change typed before the review loaded.'); + await page.getByRole('button', { name: 'Ask', exact: true }).click(); + + // The text now belongs to P1: another item starts empty, and P1 still has it on return. + await page.locator('[data-select="P2"]').click(); + await expect(page.getByLabel('Question about this item')).toHaveValue(''); + await page.locator('[data-select="P1"]').click(); + await expect(page.getByLabel('Question about this item')).toHaveValue('Typed before the review loaded. Still typing.'); + + expect(actions).toEqual([]); + expect(app.service.store.getReviewNotes(app.service.config.identity)).toEqual([]); + expect(errors).toEqual([]); +}); + +test('keeps text typed before a failed first load and gives it to the first item when Refresh succeeds', async ({ page }) => { + const errors: string[] = []; + page.on('pageerror', error => errors.push(error.message)); + const release = await openWithHeldReview(page, app.url, 'fail'); + await page.getByLabel('Question about this item').fill('Typed before the failed load.'); + await page.getByRole('button', { name: 'Request change', exact: true }).click(); + await page.getByLabel('Change to request').fill('Change typed before the failed load.'); + await page.getByRole('button', { name: 'Ask', exact: true }).click(); + // Typed after the last mode switch, so no mode switch saved this part. + await page.getByLabel('Question about this item').press('End'); + await page.getByLabel('Question about this item').pressSequentially(' Still typing.'); + + release(); + await expect(page.locator('#banner')).toContainText('Could not read this branch’s history.'); + await expect(page.getByLabel('Question about this item')).toBeDisabled(); + await expect(page.getByLabel('Question about this item')).toHaveValue('Typed before the failed load. Still typing.'); + + await page.getByRole('button', { name: 'Refresh', exact: true }).click(); + await expect(page.locator('[data-select="P1"]')).toHaveAttribute('aria-current', 'true', { timeout: 30_000 }); + await expect(page.getByLabel('Question about this item')).toBeEnabled(); + await expect(page.getByLabel('Question about this item')).toHaveValue('Typed before the failed load. Still typing.'); + await expect(page.getByText('Retained draft')).toHaveCount(0); + await page.getByRole('button', { name: 'Request change', exact: true }).click(); + await expect(page.getByLabel('Change to request')).toHaveValue('Change typed before the failed load.'); + expect(app.service.store.getReviewNotes(app.service.config.identity)).toEqual([]); + expect(errors).toEqual([]); +}); + +test('refuses a plan import made before the first review load, says why, and imports once the plan appears', async ({ page }) => { + const errors: string[] = []; + page.on('pageerror', error => errors.push(error.message)); + const imports = countRequests(page, '/api/plan/import'); + const release = await openWithHeldReview(page, plansUrl()); + + await expect(page.locator('#plans-summary')).toHaveText('Loading plan…'); + await expect(page.locator('#plans-status')).toHaveText('Loading the current plan…'); + const importButton = page.getByRole('button', { name: 'Import next revision' }); + await expect(importButton).toHaveAttribute('aria-disabled', 'true'); + // aria-disabled keeps the control reachable: keyboard activation must explain the refusal, not do nothing. With no + // file chosen yet, the browser's own file check must not answer instead. + await importButton.focus(); + await page.keyboard.press('Enter'); + await expect(page.locator('#plans-status')).toHaveText('! The plan is still loading. Nothing was imported. Import after it appears.'); + await expect(page.locator('#plans-status')).toHaveClass('warn'); + await expect(importButton).toBeFocused(); + await page.locator('#plans-status').evaluate(element => { element.textContent = ''; }); + await page.getByLabel('Plan file').setInputFiles({ name: 'plan.json', mimeType: 'application/json', buffer: Buffer.from(JSON.stringify(nextPlan)) }); + await importButton.focus(); + await page.keyboard.press('Enter'); + await expect(page.locator('#plans-status')).toHaveText('! The plan is still loading. Nothing was imported. Import after it appears.'); + await expect(importButton).toBeFocused(); + + release(); + await expect(page.locator('#plans-status')).toHaveText('✓ Revision r1 is current.', { timeout: 30_000 }); + await expect(page.locator('#plans-revision')).toHaveText('r1'); + await expect(importButton).not.toHaveAttribute('aria-disabled', 'true'); + expect(imports).toEqual([]); + expect(app.service.store.getPlan(app.service.config.identity).revision).toBe(1); + + // The file chosen during the load is still selected and imports normally now. + await expect(page.locator('#plan-file-details')).toHaveText('plan.json · JSON'); + await importButton.click(); + await expect(page.locator('#plans-status')).toHaveText('✓ Revision r2 is current.', { timeout: 30_000 }); + expect(imports).toEqual(['POST']); + expect(app.service.store.getPlan(app.service.config.identity).revision).toBe(2); + expect(errors).toEqual([]); +}); + +test('refuses a plan import after the first review load failed, and says why', async ({ page }) => { + const errors: string[] = []; + page.on('pageerror', error => errors.push(error.message)); + const imports = countRequests(page, '/api/plan/import'); + const release = await openWithHeldReview(page, plansUrl(), 'fail'); + release(); + await expect(page.locator('#plans-status')).toContainText('✕ Could not read this branch’s history. History unavailable.'); + await expect(page.locator('#plans-summary')).toHaveText('Plan unavailable'); + await expect(page.locator('#plan-author-status')).toHaveText('Load a plan before asking the agent.'); + + const importButton = page.getByRole('button', { name: 'Import next revision' }); + await expect(importButton).toHaveAttribute('aria-disabled', 'true'); + await importButton.focus(); + await page.keyboard.press('Enter'); + await expect(page.locator('#plans-status')).toHaveText('✕ No plan is loaded, so nothing was imported. Use Refresh plan, then import.'); + await expect(page.locator('#plans-status')).toHaveClass('bad'); + await expect(importButton).toBeFocused(); + await page.locator('#plans-status').evaluate(element => { element.textContent = ''; }); + await page.getByLabel('Plan file').setInputFiles({ name: 'plan.json', mimeType: 'application/json', buffer: Buffer.from(JSON.stringify(nextPlan)) }); + await importButton.focus(); + await page.keyboard.press('Enter'); + await expect(page.locator('#plans-status')).toHaveText('✕ No plan is loaded, so nothing was imported. Use Refresh plan, then import.'); + expect(imports).toEqual([]); + expect(app.service.store.getPlan(app.service.config.identity).revision).toBe(1); + expect(errors).toEqual([]); +}); + +test('refuses a plan import during Refresh plan after a failed first load, and keeps the refresh result', async ({ page }) => { + const errors: string[] = []; + page.on('pageerror', error => errors.push(error.message)); + const imports = countRequests(page, '/api/plan/import'); + const release = await openWithHeldReview(page, plansUrl(), 'fail'); + release(); + await expect(page.locator('#plans-status')).toContainText('✕ Could not read this branch’s history.'); + + let releaseRefresh!: () => void, refreshArrived!: () => void; + const heldRefresh = new Promise(resolve => { releaseRefresh = resolve; }); + const refreshReached = new Promise(resolve => { refreshArrived = resolve; }); + await page.route('**/api/review', async route => { + refreshArrived(); + await heldRefresh; + await route.continue(); + }, { times: 1 }); + await page.getByRole('button', { name: 'Refresh plan' }).click(); + await refreshReached; + await expect(page.locator('#plans-summary')).toHaveText('Loading plan…'); + await expect(page.locator('#plan-author-status')).toHaveText('The plan is still loading. Ask the agent after it appears.'); + + const importButton = page.getByRole('button', { name: 'Import next revision' }); + await expect(importButton).toHaveAttribute('aria-disabled', 'true'); + await page.getByLabel('Plan file').setInputFiles({ name: 'plan.json', mimeType: 'application/json', buffer: Buffer.from(JSON.stringify(nextPlan)) }); + await importButton.focus(); + await page.keyboard.press('Enter'); + await expect(page.locator('#plans-status')).toHaveText('! The plan is still loading. Nothing was imported. Import after it appears.'); + + releaseRefresh(); + await expect(page.locator('#plans-status')).toHaveText('✓ Revision r1 is current.', { timeout: 30_000 }); + await expect(page.locator('#plans-revision')).toHaveText('r1'); + await expect(importButton).not.toHaveAttribute('aria-disabled', 'true'); + expect(imports).toEqual([]); + expect(app.service.store.getPlan(app.service.config.identity).revision).toBe(1); + expect(errors).toEqual([]); +}); diff --git a/test/browser/plans-authoring.spec.ts b/test/browser/plans-authoring.spec.ts index 17da263b..7aa1d5c9 100644 --- a/test/browser/plans-authoring.spec.ts +++ b/test/browser/plans-authoring.spec.ts @@ -387,6 +387,55 @@ test('replays an ambiguous suggestion Apply, stales siblings, and refreshes befo expect(app.service.store.getPlan(app.service.config.identity).revision).toBe(3); }); +test('refuses an import during a Retry Apply made after the review failed, and keeps the Apply result', async ({ page }) => { + const { deps, invocations } = planning(); + await openPlans(page, deps); + await page.getByLabel('Guidance').fill('Improve the retry item.'); + await page.getByRole('button', { name: 'Suggest edits' }).click(); + await expect.poll(() => invocations.length).toBe(1); + invocations[0]!.resolve(JSON.stringify(suggestions())); + await expect(page.getByRole('button', { name: 'Apply this edit' })).toHaveCount(1); + + // The first Apply commits but its answer is lost, so the result stays Apply-uncertain. + let applies = 0, releaseRetry!: () => void, retryArrived!: () => void; + const heldRetry = new Promise(resolve => { releaseRetry = resolve; }); + const retryReached = new Promise(resolve => { retryArrived = resolve; }); + await page.route('**/api/plan/suggestions/*/apply', async route => { + applies++; + if (applies === 1) { await route.fetch(); await route.abort('failed'); return; } + retryArrived(); + await heldRetry; + await route.continue(); + }); + await page.getByRole('button', { name: 'Apply this edit' }).click(); + await expect(page.getByRole('button', { name: 'Retry Apply', exact: true })).toBeVisible(); + + // A failed review refresh unloads the plan while Retry Apply stays available. + await page.route('**/api/review', route => route.fulfill({ status: 500, contentType: 'application/json', body: JSON.stringify({ error: 'History unavailable.' }) }), { times: 1 }); + await page.getByRole('link', { name: 'Review', exact: true }).click(); + await page.getByRole('button', { name: 'Refresh', exact: true }).click(); + await expect(page.locator('#banner')).toContainText('Could not read this branch’s history.'); + await page.getByRole('link', { name: 'Plans', exact: true }).click(); + await expect(page.locator('#plans-summary')).toHaveText('Plan unavailable'); + + const imports: string[] = []; + page.on('request', request => { if (new URL(request.url()).pathname === '/api/plan/import') imports.push(request.method()); }); + await page.getByRole('button', { name: 'Retry Apply', exact: true }).click(); + await retryReached; + await page.getByLabel('Plan file').setInputFiles({ name: 'plan.json', mimeType: 'application/json', buffer: Buffer.from('{}') }); + const importButton = page.getByRole('button', { name: 'Import next revision' }); + await importButton.focus(); + await page.keyboard.press('Enter'); + await expect(page.locator('#plans-status')).toHaveText('! Wait for the current review action before importing a plan.'); + + releaseRetry(); + await expect(page.locator('#plans-status')).toHaveText('✓ Revision r2 is current.'); + await expect(page.locator('#plans-revision')).toHaveText('r2'); + expect(imports).toEqual([]); + expect(applies).toBe(2); + expect(app.service.store.getPlan(app.service.config.identity).revision).toBe(2); +}); + test('retries an ambiguous suggestion start with the exact request', async ({ page }) => { const { deps, invocations } = planning(); await openPlans(page, deps); diff --git a/test/browser/plans.spec.ts b/test/browser/plans.spec.ts index 4583334b..5a0a3ecd 100644 --- a/test/browser/plans.spec.ts +++ b/test/browser/plans.spec.ts @@ -120,6 +120,21 @@ for (const fixture of [ }); } +test('explains an import pressed with no file chosen instead of leaving it to the browser', async ({ page }) => { + const imports: string[] = []; + page.on('request', request => { if (new URL(request.url()).pathname === '/api/plan/import') imports.push(request.method()); }); + await openReview(page, app.url); + await page.getByRole('link', { name: 'Plans', exact: true }).click(); + await expect(page.locator('#plans-revision')).toHaveText('r1'); + const importButton = page.getByRole('button', { name: 'Import next revision' }); + await importButton.click(); + await expect(page.locator('#plans-status')).toHaveText('✕ Choose a JSON or YAML plan file.'); + await expect(page.locator('#plans-status')).toHaveClass('bad'); + await expect(importButton).toBeFocused(); + expect(imports).toEqual([]); + expect(app.service.store.getPlan(app.service.config.identity).revision).toBe(1); +}); + test('shows a rejected import without replacing the current plan', async ({ page }) => { await openReview(page, app.url); await page.getByRole('link', { name: 'Plans', exact: true }).click(); diff --git a/web/public/app.js b/web/public/app.js index a79f8c54..eee8c730 100644 --- a/web/public/app.js +++ b/web/public/app.js @@ -68,8 +68,16 @@ async function api(path, body) { if (!response.ok) throw Object.assign(new Error(value.error || "Request failed."), { status: response.status, outcomeUnknown: value.outcomeUnknown === true }); return value; } +// Before the first review load no item is selected; text typed then is kept under "" until render() selects an item. function rememberDraft() { - if (selected) drafts.set(`${selected}:${mode}`, $("message").value); + drafts.set(`${selected ?? ""}:${mode}`, $("message").value); +} +function adoptUnselectedDrafts() { + for (const kind of ["question", "change"]) { + const text = drafts.get(`:${kind}`); + drafts.delete(`:${kind}`); + if (text) drafts.set(`${selected}:${kind}`, text); + } } function showFailure(message, planStatusOwner) { mergeGeneration++; @@ -123,6 +131,10 @@ async function refresh() { mergePollDelay = 2000; renderAttachment(); $("banner").textContent = "Linking changes to plan items…"; + if (!data) { + setPlanStatus(planStatusOwner, "neutral", "Loading the current plan…"); + renderPlans(); + } try { rememberDraft(); const updated = await api("/api/review"); @@ -136,7 +148,6 @@ async function refresh() { const newerQueue = newerMergeQueue(updated, mergeObservationOwner); data = preserveSettledQuestionAnswers(newerQueue ? withMergeQueue(updated, newerQueue) : updated); snippetSelection = null; - selected ??= data.items[0]?.id || "Unplanned"; render(); if (newerQueue) showMergeQueueStatus(newerQueue); setPlanStatus(planStatusOwner, "good", `✓ Revision r${data.plan.revision} is current.`); @@ -149,6 +160,8 @@ async function refresh() { ); } finally { endBusy(busyOwner); + // showFailure rendered the Plans screen while this read still counted as loading. + if (!data) renderPlans(); } } function renderMerge() { @@ -281,6 +294,10 @@ function select(id) { render(); } function render() { + if (selected === undefined) { + selected = data.items[0]?.id || "Unplanned"; + adoptUnselectedDrafts(); + } const item = data.items.find((item) => item.id === selected); const retainedItems = [...new Set([ ...[...drafts].filter(([, text]) => text.length).map(([key]) => key.split(":")[0]), @@ -499,7 +516,7 @@ function setMode(value) { mode === "change" ? "Change to request" : "Question about this item"; $("save-note").textContent = mode === "change" ? "Save change request" : "Ask agent"; - $("message").value = drafts.get(`${selected}:${mode}`) || ""; + $("message").value = drafts.get(`${selected ?? ""}:${mode}`) || ""; renderAttachment(); } function showDialog(html) { @@ -1095,7 +1112,8 @@ function renderPlanAuthor() { } if (!request) { $("plan-author-status").className = "neutral"; - $("plan-author-status").textContent = unavailable ? "Load a plan before asking the agent." : ""; + $("plan-author-status").textContent = !unavailable ? "" : reviewLoading() + ? "The plan is still loading. Ask the agent after it appears." : "Load a plan before asking the agent."; $("plan-author-result").innerHTML = ""; return; } @@ -1125,8 +1143,9 @@ function renderPlanAuthor() { else if (restoreApplyFocus) ($(restoreApplyFocus) ?? $("plan-author-refresh-suggestions") ?? $("plans-refresh"))?.focus(); } function renderPlans() { + renderPlanImport(); if (!data?.plan) { - $("plans-summary").textContent = "Plan unavailable"; + $("plans-summary").textContent = reviewLoading() ? "Loading plan…" : "Plan unavailable"; $("plans-revision").textContent = ""; $("plans-items").innerHTML = ""; $("plans-questions").innerHTML = ""; @@ -1152,6 +1171,15 @@ function renderPlans() { `).join(""); renderPlanAuthor(); } +// A plan or review is being read: the page's review read, or Refresh plan, which reads without taking the busy flag. +const reviewLoading = () => busyKind === "read" || planRefreshPending; +// aria-disabled, not disabled: focus stays on the button, and a click with no plan loaded still reaches importPlan, +// which says why nothing was imported. The form is novalidate so the browser's file check cannot answer first; +// importPlan reports a missing file itself. (During an import, the button's "Importing…" label shows the state.) +function renderPlanImport() { + if (planImportPending || !data?.plan) $("plan-import").setAttribute("aria-disabled", "true"); + else $("plan-import").removeAttribute("aria-disabled"); +} function planFileFormat(name) { return /\.ya?ml$/i.test(name) ? "yaml" : /\.json$/i.test(name) ? "json" : null; } @@ -1164,9 +1192,18 @@ function renderPlanFile() { } async function importPlan(event) { event.preventDefault(); - if (planImportPending || !data?.plan) return; + if (planImportPending) return; + // The busy refusal and the "still loading" refusal keep the current status ownership, so a running Apply, review read + // or Refresh plan replaces them with its result instead of being hidden by them. The "no plan" refusal claims the line + // only when nothing is running. The file refusal below always claims it, like the import itself: feedback on the + // file just chosen is newer than any read already running, so that read's result must not replace it. if (busy && busyKind !== "read") { - claimPlanStatus("warn", "! Wait for the current review action before importing a plan."); + setPlanStatus(planStatusGeneration, "warn", "! Wait for the current review action before importing a plan."); + return; + } + if (!data?.plan) { + if (reviewLoading()) setPlanStatus(planStatusGeneration, "warn", "! The plan is still loading. Nothing was imported. Import after it appears."); + else claimPlanStatus("bad", "✕ No plan is loaded, so nothing was imported. Use Refresh plan, then import."); return; } const file = $("plan-file").files[0], format = file && planFileFormat(file.name); @@ -1193,7 +1230,7 @@ async function importPlan(event) { } catch (error) { planImportPending = false; endBusy(busyOwner); - $("plan-import").removeAttribute("aria-disabled"); + renderPlanImport(); $("plan-import").textContent = "Import next revision"; setPlanStatus(statusOwner, "bad", `✕ Could not read the plan file. ${error.message}`); renderMerge(); @@ -1246,7 +1283,7 @@ async function importPlan(event) { } finally { planImportPending = false; endBusy(busyOwner); - $("plan-import").removeAttribute("aria-disabled"); + renderPlanImport(); $("plan-import").textContent = "Import next revision"; } } @@ -1661,6 +1698,7 @@ $("plans-refresh").onclick = async () => { $("plans-refresh").setAttribute("aria-disabled", "true"); $("plans-refresh").textContent = "Refreshing…"; const statusOwner = claimPlanStatus("neutral", "Refreshing the current plan…"); + if (!data) renderPlans(); try { rememberDraft(); const updated = await api("/api/review"); @@ -1689,6 +1727,7 @@ $("plans-refresh").onclick = async () => { planRefreshPending = false; $("plans-refresh").removeAttribute("aria-disabled"); $("plans-refresh").textContent = "Refresh plan"; + if (!data) renderPlans(); } } }; diff --git a/web/public/index.html b/web/public/index.html index 9e366630..3d04ddaa 100644 --- a/web/public/index.html +++ b/web/public/index.html @@ -96,7 +96,7 @@

Loading plan…

IMPORT PLAN
-
+

Import a complete JSON or YAML plan as the next revision. The selected issue cannot change.