diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 8e670f57..04dd9e00 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -60,6 +60,16 @@ jobs: - run: npm run i18n -- check - run: npm run i18n:check-app - run: npm run build + - name: Review batching and toolbar browser regressions + if: matrix.node == 22 + run: | + node --import tsx scripts/check-review-batching.mjs + node --import tsx scripts/check-review-application.mjs + node --import tsx scripts/check-review-flow.mjs + node --import tsx scripts/check-note-results.mjs + node --import tsx scripts/check-note-options-flow.mjs + env: + PAPILOT_BROWSER_CHANNEL: chromium # Checks the generated UI demos and local asset links before main. - run: npm run site - name: Verify the static playground without a backend diff --git a/CHANGELOG.md b/CHANGELOG.md index 0db1d89b..2baf1470 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,12 +13,13 @@ Notable changes to Papilot, newest first. The format follows [Keep a Changelog]( - **Shorter change locations.** A change shows its file's name and line span; hovering shows the full path, every line and the page. ([#112](https://github.com/PapilotAI/papilot/pull/112)) - **Note alternatives are off until chosen.** Pick a count in Review preferences to have edits prepare them; a saved choice is kept. ([#112](https://github.com/PapilotAI/papilot/pull/112)) - **Smoother review scrolling.** The review scrolls natively, and selectable PDF text is laid out once scrolling rests. ([#112](https://github.com/PapilotAI/papilot/pull/112)) - - **A much smaller desktop installer.** The desktop app no longer includes Codex: it uses the Codex installed on the computer, and **Settings → Runtime → Agents** says when none is found or none runs, so you can install it and select **Reconnect**. The review engine also no longer carries its own copy of dependencies the app already has. ([#110](https://github.com/PapilotAI/papilot/pull/110)) - **Quicker with a long history.** Each streamed agent message updates one task and one message instead of reading every task and the whole conversation again, and the app's state poll gets an empty answer when nothing changed instead of every task again. ([#110](https://github.com/PapilotAI/papilot/pull/110)) +- **Note review stays continuous.** The drawer stays open, advances to the next pending note after a confirmed decision, and leaves the final note available to inspect. Alternative previews reconnect once if the review engine restarts. ([#109](https://github.com/PapilotAI/papilot/pull/109)) ### Fixed +- **Apply reuses the final selection's compilation.** Intermediate decisions do not refresh the manuscript; the final selection is prepared in the background and reused when saved. Older tasks that write directly to the manuscript refresh only after their last pending decision is confirmed. ([#109](https://github.com/PapilotAI/papilot/pull/109)) - **Notes on an older PDF are kept.** A note written while a newer PDF compiles is saved and placed in the current source, and a new PDF arriving leaves an open note box as it is. ([#112](https://github.com/PapilotAI/papilot/pull/112)) - **Saving a PDF note no longer waits for SyncTeX.** The note appears at once and its source fills in afterwards. ([#112](https://github.com/PapilotAI/papilot/pull/112)) - **Saving is offered after rejecting changes.** Rejections no longer hide Apply and Commit; sending them to the agent stays optional. ([#112](https://github.com/PapilotAI/papilot/pull/112)) diff --git a/docs/images/review-building-toolbar.png b/docs/images/review-building-toolbar.png new file mode 100644 index 00000000..e485b9c6 Binary files /dev/null and b/docs/images/review-building-toolbar.png differ diff --git a/docs/images/review-note-drawer.png b/docs/images/review-note-drawer.png new file mode 100644 index 00000000..dc883cb7 Binary files /dev/null and b/docs/images/review-note-drawer.png differ diff --git a/docs/performance.md b/docs/performance.md index 35a7f9a4..264fd9e9 100644 --- a/docs/performance.md +++ b/docs/performance.md @@ -14,15 +14,17 @@ Papilot should compile about as fast as running `latexmk` (or `make`) in the pap ## When builds start -The manuscript compiles as soon as something may have changed its files, not when its view is next opened: after a task is committed or applied, a Git action, or a review decision. A build started meanwhile is followed right away by one of the newest files. The view learns that a build started or finished from the server's event stream and reloads at once; edits saved in another editor are noticed within five seconds while the manuscript is open, and immediately when Papilot's window regains focus. +The manuscript compiles after a task is committed or applied, or after a Git action changes the paper. Individual task decisions and notes do not trigger a manuscript refresh. Older tasks that write accepted changes directly to the manuscript refresh once the last pending change is decided. A build started meanwhile is followed right away by one of the newest files. The view learns that a build started or finished from the server's event stream and reloads at once; edits saved in another editor are noticed within five seconds while the manuscript is open, and immediately when Papilot's window regains focus. -A task that waits to be applied (the default) changes the manuscript only when it is committed or applied, so that build could only start then. Instead, whenever a round is published or a change is accepted or rejected, Papilot asks the review engine what applying the task would write if every pending change were accepted, merged with the files on disk exactly as applying merges them, and compiles that version in the background. When the task is applied, the manuscript's build has the same key: its PDF is already there, or the build already under way is the one it waits for. A rejected change makes a new prediction, which continues from the previous one. +A task that waits to be applied (the default) saves each acceptance, rejection and undo without refreshing the manuscript. Once every change is decided, Papilot predicts the manuscript with the final choices in the background. Applying the result starts one manuscript refresh, which reuses that prediction or joins it if it is still building. Generating a new comparison after an agent revision or an explicit source edit remains a separate operation. ## The review's full paper -A review can show its changes in the whole paper (**PDF** in the review's switch). A round compiles only the changed chapters, so the whole new version is a separate build. While the review engine publishes a round that compiles only some chapters, its job status names the round's new version (`revision`), and Papilot starts compiling the whole paper at once, in the background, alongside the round's own builds instead of after them. It is usually the same version that committing the task will produce, so the commit's prediction often finds it built. A round that compiles the whole paper names no version: its own build is the full PDF, and a build of it here would wait for that one, or hold up the round. +For a task that waits to be applied, the review's **PDF** view shows the paper with the decisions made so far. The engine compiles this preview in the background after each decision; opening the view reads its latest result. The previous pages stay visible while the next version loads, and a text selection keeps those pages in place until the author finishes it. This preview does not refresh the manuscript or apply the task. -When the review opens on the full paper before that PDF is complete, it does not wait for it. It first shows the PDF the round itself compiled (`build` in the round's details): the changed chapters, with each change exactly where its strips come from. Then it shows each TeX pass of the whole paper once latexmk follows it with more work, and finally the complete PDF (`passes` in the round's details: `?after=N` answers with the next version, and the PDF route serves it with `?pass=N`). Each version replaces the previous one keeping the change being reviewed at the same place on screen, and pages already painted stay until the new version's replace them. +Other review PDF views, including prepared static demos, show the published round. A round may compile only the changed chapters, so its whole new version is a separate build requested when that view is opened. A round that already compiled the whole paper can reuse that PDF. + +When a published round's full paper is opened before that PDF is complete, it does not wait for it. It first shows the PDF the round itself compiled (`build` in the round's details): the changed chapters, with each change exactly where its strips come from. Then it shows each TeX pass of the whole paper once latexmk follows it with more work, and finally the complete PDF (`passes` in the round's details: `?after=N` answers with the next version, and the PDF route serves it with `?pass=N`). Each version replaces the previous one keeping the change being reviewed at the same place on screen, and pages already painted stay until the new version's replace them. ## Priorities diff --git a/engine/src/web/review.tsx b/engine/src/web/review.tsx index e416ecb6..d92dc1c1 100644 --- a/engine/src/web/review.tsx +++ b/engine/src/web/review.tsx @@ -570,6 +570,10 @@ export function ReviewApp({ setOnline(true); setError(""); setSettling(false); + const unavailable = (value: PageState) => value.state === "building" || value.state === "build_failed" || Boolean(value.rendering); + // A new source round can precede its PDF, or retain a failed build. + // Clear only the previous build's success message, not action feedback. + if (unavailable(next)) setToast(message => message === t("Changes are ready") ? "" : message); const target = pinned || next.latest; if ( target && @@ -641,9 +645,16 @@ export function ReviewApp({ const shown = paper.current?.querySelector(`[data-stop="${index}"]`); if (shown) reveal(shown); }); - else if (previous.detail) + if (previous.detail && !loaded.rendering && !unavailable(loadedState) && (!same || previous.detail.rendering)) setToast(t("Changes are ready")); - } else setState(next); + } else { + setState(next); + // The PDF and job status are read separately. If the PDF arrived + // first, announce it when the publication's status catches up. + if (previous.detail && !previous.detail.rendering && !unavailable(next) && + (previous.state?.state === "building" || previous.state?.state === "build_failed")) + setToast(t("Changes are ready")); + } } catch (e) { if (!cancelled) { setOnline(false); diff --git a/i18n/state/de.json b/i18n/state/de.json index d5836786..a79bb0bc 100644 --- a/i18n/state/de.json +++ b/i18n/state/de.json @@ -3445,7 +3445,7 @@ "model": "gemini-4-argon-medium" }, "Request failed. Please try again.": { - "revision": "e275550822028b5cb54a0d5711b53b4ceef495de5e0d3679a8d51e5ed35d8738", + "revision": "f03f2760a79e714b8fda0931f31e39de4cf77c58417b79a61c19bdb362abce53", "textHash": "505409b0cde0e505f7ce673cd183cf32f91c5f927b2490f5b7de25d674f52c87", "model": "gemini-4-argon-medium" }, diff --git a/i18n/state/es.json b/i18n/state/es.json index 3622b211..3000598f 100644 --- a/i18n/state/es.json +++ b/i18n/state/es.json @@ -3445,7 +3445,7 @@ "model": "gemini-4-argon-medium" }, "Request failed. Please try again.": { - "revision": "05806d16823bc6933f689ef436b50e6fd2b8540cf92598595ee8be546862880e", + "revision": "d83e10fbb72d67d9a6f67bde7d8af74305566dfa608c24b971276a8db0f68feb", "textHash": "94733f294d13e7a2438b871ce5373382eb5192b7ac8b27bd0606c85cb3b9cda3", "model": "gemini-4-argon-medium" }, diff --git a/i18n/state/fr.json b/i18n/state/fr.json index 6b2af71c..3320530f 100644 --- a/i18n/state/fr.json +++ b/i18n/state/fr.json @@ -3445,7 +3445,7 @@ "model": "gemini-4-argon-medium" }, "Request failed. Please try again.": { - "revision": "0953e94f1a424052f933c862517a3edcd17998db19574f66c1eb676c22c047f5", + "revision": "68472a355c23f37c23127432fc3e472dccc4cfff60a49531fbf9c1fee8edbec0", "textHash": "f8236763d1736f9dc312a5067536d62d411a0958684d9dbb6050fb25846b44b3", "model": "gemini-4-argon-medium" }, diff --git a/i18n/state/it.json b/i18n/state/it.json index f93a02cc..6d728727 100644 --- a/i18n/state/it.json +++ b/i18n/state/it.json @@ -3445,7 +3445,7 @@ "model": "gemini-4-argon-medium" }, "Request failed. Please try again.": { - "revision": "0e9f1b1a25cb0e8c8e1f40661ae3c7b82287649097e20d93e61ecfee0d8327af", + "revision": "327db58350a6a10c6ed59ca53905a033053dae02de20396539ddac3cc3d103d9", "textHash": "117542b4dfccf02f8af21f6ccad9a4bc0e578c514acc623b67442d0e3e113235", "model": "gemini-4-argon-medium" }, diff --git a/i18n/state/ja.json b/i18n/state/ja.json index 058faff6..8aa331c8 100644 --- a/i18n/state/ja.json +++ b/i18n/state/ja.json @@ -3445,7 +3445,7 @@ "model": "gemini-4-argon-medium" }, "Request failed. Please try again.": { - "revision": "f9bd1529488d56e09a53b5b2a3d521c212cf6e9d3a03afcd5419608f0cdb8898", + "revision": "b3f2cb80e36357b2d6198aae5e8ae777d62a1bc5e677e23342cad172670f14e1", "textHash": "5b2c7704f726e1b81ed10f9b8bd47fa3055812884ffcdcdb8fd8b4f8170ee6b7", "model": "gemini-4-argon-medium" }, diff --git a/i18n/state/ko.json b/i18n/state/ko.json index e921b931..00179c35 100644 --- a/i18n/state/ko.json +++ b/i18n/state/ko.json @@ -3445,7 +3445,7 @@ "model": "gemini-4-argon-medium" }, "Request failed. Please try again.": { - "revision": "546e5f15f093f21abfd1f8916bbd2ad2c829bd3962794dd295a52af572b272c2", + "revision": "80ffc9305d45ac71bac9f30795f554032dfdc6246c32a46c66db20dfdee4a57c", "textHash": "cfc551f50dcaedb80afa4f981335447a4173ea56c8036ca7bc13f71e4ce72425", "model": "gemini-4-argon-medium" }, diff --git a/i18n/state/pt.json b/i18n/state/pt.json index c8246682..375fc0ff 100644 --- a/i18n/state/pt.json +++ b/i18n/state/pt.json @@ -3445,7 +3445,7 @@ "model": "gemini-4-argon-medium" }, "Request failed. Please try again.": { - "revision": "9f7ca132d2ba0061d9856fdc3a1a7331ea3e2da314955f437d1ef46ea2bd83a6", + "revision": "e8b2ce6f88599eec6a9fd9a9178eb57513fddfe689dbe3dc594ad726a7b36495", "textHash": "bd4ab0b8c990068df745b5747e69e254139998533089942da3978a56b23a998b", "model": "gemini-4-argon-medium" }, diff --git a/i18n/state/sv.json b/i18n/state/sv.json index c2a4c13c..8d0b292e 100644 --- a/i18n/state/sv.json +++ b/i18n/state/sv.json @@ -3445,7 +3445,7 @@ "model": "gemini-4-argon-medium" }, "Request failed. Please try again.": { - "revision": "0b54b70fa7198e466b1a2f2543bbc1981b6d51a195101d74b8168ae7554380ed", + "revision": "88fd9f188fe67de9e7686f9df2df0242020b2784e3b98544d56f0da5bab3282d", "textHash": "8884bdbd813f948cf479b6c62cdd73955d1738c4583adbe3c666a0d8c4139951", "model": "gemini-4-argon-medium" }, diff --git a/i18n/state/zh-Hant.json b/i18n/state/zh-Hant.json index ad84db98..d7a17374 100644 --- a/i18n/state/zh-Hant.json +++ b/i18n/state/zh-Hant.json @@ -3445,7 +3445,7 @@ "model": "gemini-4-argon-medium" }, "Request failed. Please try again.": { - "revision": "fae7b2ccd697914f9bc8164a8673caa57b8178a89c7414b3fcf75a8cfcd48f4e", + "revision": "a5ba07b061e7157c6c7f06453fd467ab48cedc296dee5f4b33df308f4ece814a", "textHash": "0ce0b5dc3cb5f7c4471b4e0738eb026c182543755d5557bad7b0b543720db80e", "model": "gemini-4-argon-medium" }, diff --git a/package.json b/package.json index 872dc8fb..a43cc821 100644 --- a/package.json +++ b/package.json @@ -67,12 +67,13 @@ "typecheck": "tsc --noEmit && tsc -p desktop/tsconfig.json --noEmit", "test": "tsx --test tests/*.test.ts", "test:source-edit": "npm run build && npm --prefix engine run build && node --import tsx scripts/check-source-edit.mjs", + "test:review-batching": "npm run build && npm --prefix engine run build && node --import tsx scripts/check-review-batching.mjs && node --import tsx scripts/check-review-application.mjs", "setup:review": "npm --prefix engine ci && npm --prefix engine run build", "demo": "tsx scripts/demo.ts", "doctor": "node bin/papilot.mjs doctor", "service:install": "tsx scripts/service.ts install", "service:stop": "tsx scripts/service.ts stop", - "test:ui": "npm run build && node scripts/check-system-requirements.mjs && node scripts/check-settings-layout.mjs && node scripts/check-git-workspace.mjs && node scripts/check-ui.mjs && node scripts/check-task-preparation.mjs && node scripts/check-git-panel.mjs && node scripts/check-task-model.mjs && node scripts/check-comparison-progress.mjs && node scripts/check-appearance.mjs && node --import tsx scripts/check-review-pdf-appearance.mjs && node scripts/check-updates.mjs && node scripts/check-papers.mjs && node scripts/check-workspace-switcher.mjs && node scripts/check-workspace-switcher-notes.mjs && node scripts/check-imports.mjs && node scripts/check-compilation.mjs && node scripts/check-build-problems.mjs && node scripts/check-review-notes.mjs && node --import tsx scripts/check-note-groups.mjs && node --import tsx scripts/check-pdf-tools.mjs && node scripts/check-note-selection.mjs && node scripts/check-note-new-pass.mjs && node scripts/check-ai-review.mjs && node --import tsx scripts/check-note-results.mjs && node --import tsx scripts/check-review-flow.mjs && node --import tsx scripts/check-fast-decisions.mjs && node --import tsx scripts/check-review-batching.mjs && node scripts/check-skills.mjs && node --import tsx scripts/check-skill-library.mjs && node scripts/check-attachments.mjs && node --import tsx scripts/check-places.mjs && node scripts/check-pdf-words.mjs && node scripts/check-reply-duplicates.mjs && node --import tsx scripts/check-task-permissions.mjs", + "test:ui": "npm run build && node scripts/check-system-requirements.mjs && node scripts/check-settings-layout.mjs && node scripts/check-git-workspace.mjs && node scripts/check-ui.mjs && node scripts/check-task-preparation.mjs && node scripts/check-git-panel.mjs && node scripts/check-task-model.mjs && node scripts/check-comparison-progress.mjs && node scripts/check-appearance.mjs && node --import tsx scripts/check-review-pdf-appearance.mjs && node scripts/check-updates.mjs && node scripts/check-papers.mjs && node scripts/check-workspace-switcher.mjs && node scripts/check-workspace-switcher-notes.mjs && node scripts/check-imports.mjs && node scripts/check-compilation.mjs && node scripts/check-build-problems.mjs && node scripts/check-review-notes.mjs && node --import tsx scripts/check-note-groups.mjs && node --import tsx scripts/check-pdf-tools.mjs && node scripts/check-note-selection.mjs && node scripts/check-note-new-pass.mjs && node scripts/check-ai-review.mjs && node --import tsx scripts/check-note-results.mjs && node --import tsx scripts/check-note-options-flow.mjs && node --import tsx scripts/check-review-flow.mjs && node --import tsx scripts/check-fast-decisions.mjs && node --import tsx scripts/check-review-batching.mjs && node --import tsx scripts/check-review-application.mjs && node scripts/check-skills.mjs && node --import tsx scripts/check-skill-library.mjs && node scripts/check-attachments.mjs && node --import tsx scripts/check-places.mjs && node scripts/check-pdf-words.mjs && node scripts/check-reply-duplicates.mjs && node --import tsx scripts/check-task-permissions.mjs", "site": "node scripts/build-site.mjs", "test:site": "npm run site && node scripts/check-site-analytics.mjs && node scripts/check-site-privacy.mjs && node scripts/check-site.mjs && node scripts/check-site-blog.mjs", "prepack": "npm run build && npm run setup:review", diff --git a/scripts/check-note-options-flow.mjs b/scripts/check-note-options-flow.mjs new file mode 100644 index 00000000..128b705e --- /dev/null +++ b/scripts/check-note-options-flow.mjs @@ -0,0 +1,168 @@ +import assert from 'node:assert/strict'; +import fs from 'node:fs'; +import path from 'node:path'; +import Fastify from 'fastify'; +import staticFiles from '@fastify/static'; +import { chromium, expect } from '../engine/node_modules/@playwright/test/index.mjs'; +import { rewriteReviewHtml } from '../src/server/review.ts'; +import { readyRequirements } from './required-tools-fixture.mjs'; + +const server = Fastify(); +await server.register(staticFiles, { root: path.resolve('dist') }); +await server.listen({ host: '127.0.0.1', port: 0 }); +const origin = `http://127.0.0.1:${server.server.address().port}`; +const browser = await chromium.launch({ channel: process.env.PAPILOT_BROWSER_CHANNEL || 'chrome', headless: true }); +fs.mkdirSync('test-results', { recursive: true }); +try { + for (const width of [1440, 390]) { + const page = await browser.newPage({ locale: 'en-US', viewport: { width, height: 900 } }); + page.setDefaultTimeout(15000); + const errors = [], writes = [], previews = [], undone = new Set(); + page.on('pageerror', error => errors.push(error.message)); + await page.addInitScript(() => { localStorage.setItem('papilot:language', 'en'); localStorage.setItem('papilot:task', 'task'); }); + const task = { id: 'task', projectId: 'paper', name: 'flow', title: 'Review note choices', status: 'reviewing', createdAt: 1, updatedAt: 1, review: { round: 1, state: 'reviewing', counts: { pending: 3 } } }; + const noteUnits = ['a', 'a', 'b', 'c']; + const annotations = noteUnits.map((unit, index) => ({ where: `main.tex line ${index + 1}`, quote: `Old ${unit}`, comment: `Improve note ${index + 1}.` })); + const items = [{ id: 'run', taskId: 'task', type: 'user', text: '', createdAt: 1, meta: { notes: annotations } }]; + const decisions = { a: 'pending', b: 'pending', c: 'pending' }, selected = { a: 'current', b: 'current', c: 'current' }; + const strip = (unit, text) => { + const src = 'data:image/svg+xml,' + encodeURIComponent(`${text}`); + return { src, clean: src, width: 600, height: 36, page: 1, label: '1', text: [{ text, chars: Array.from(text, (text, index) => ({ text, rect: [12 + index * 9, 8, 21 + index * 9, 28] })) }], marks: [{ unit, kind: 'added', rect: [12, 8, 220, 28] }] }; + }; + let version = 1, failPreview = false, latePreview = null, saveFails = false, pendingBuild = null; + const changed = () => { task.updatedAt++; task.review.counts.pending = Object.values(decisions).filter(state => state === 'pending').length; }; + const notes = () => noteUnits.map((unit, index) => ({ key: `run:${index}`, itemId: 'run', index, note: annotations[index], round: task.review.round, state: 'matched', changes: [{ round: task.review.round, stop: ['a', 'b', 'c'].indexOf(unit), units: [unit], state: decisions[unit], match: 'source' }] })); + const detail = (unit, option) => ({ task: 'flow', round: task.review.round, before: 'before', after: `${unit}-${option}-v${version}`, files: '', warnings: [], sections: [], + units: ['a', 'b', 'c'].map((id, index) => ({ id, file: 'main.tex', kind: 'text', old_start: index + 1, old_end: index + 1, new_start: index + 1, new_end: index + 1, old_text: `Old ${id}`, new_text: `${id === unit ? option : selected[id]} ${id}` })), + loose: ['a', 'b', 'c'].map(id => ({ kind: 'change', units: [id], before: [strip(id, `Old ${id}`)], after: [strip(id, `${id === unit ? option : selected[id]} ${id}`)] })) }); + const options = key => { + const unit = noteUnits[Number(key.split(':')[1])]; + return { key, round: task.review.round, comparisonKey: `v${version}`, selected: selected[unit], + ...(decisions[unit] !== 'pending' ? { locked: 'This note includes a reviewed change. Undo that decision before replacing it.' } : {}), + options: [{ id: 'current', label: `Original ${unit}`, reason: 'Keep the original proposal.', edits: [{ unit, text: `current ${unit}` }] }, + { id: 'alternative', label: `Alternative ${unit}`, reason: 'Use the alternative wording.', edits: [{ unit, text: `alternative ${unit}` }] }] }; + }; + await page.route('**/api/**', async route => { + const url = new URL(route.request().url()), json = value => route.fulfill({ json: value }); + if (url.pathname.startsWith('/review/')) return route.fallback(); + if (url.pathname === '/api/state') return json({ version: 'test', csrf: 'fixture', projects: [{ id: 'paper', name: 'Paper', path: '/fixture/paper' }], tasks: [task], requests: [], runtime: { connected: true, authenticated: true, models: [] } }); + if (url.pathname === '/api/runtime/requirements') return json(readyRequirements); + if (url.pathname === '/api/updates') return json({ currentVersion: 'test', phase: 'idle' }); + if (url.pathname === '/api/indirect-changes') return json({ indirectChanges: false }); + if (url.pathname === '/api/note-alternatives') return json({ noteAlternatives: true }); + if (url.pathname.endsWith('/items')) return json({ items }); + if (url.pathname.endsWith('/note-results')) return json({ notes: notes() }); + if (url.pathname.endsWith('/note-options/preview')) { + const unit = noteUnits[Number(url.searchParams.get('key').split(':')[1])], option = url.searchParams.get('option'); + const result = detail(unit, option); previews.push({ unit, option, version }); + if (unit === 'a' && option === 'alternative') { + if (latePreview) { const wait = latePreview; latePreview = null; await wait; return route.fulfill({ status: 503, json: { error: 'Late preview failure' } }); } + if (failPreview && url.searchParams.get('retry') !== '1') return route.fulfill({ status: 503, json: { error: 'fetch failed' } }); + } + return json(result); + } + if (url.pathname.endsWith('/note-options')) { + if (route.request().method() === 'GET') return json(options(url.searchParams.get('key'))); + const body = route.request().postDataJSON(), unit = noteUnits[Number(body.key.split(':')[1])]; writes.push(body); + if (undone.has(body.id)) return route.fulfill({ status: 400, json: { error: 'That decision was undone or failed. Make it again.' } }); + if (saveFails) return route.fulfill({ status: 400, json: { error: 'The source changed. Try again.' } }); + if (body.accept && body.option !== selected[unit]) { pendingBuild = { unit, option: body.option }; task.status = 'building'; changed(); return json({ building: true }); } + decisions[unit] = body.accept ? 'accepted' : 'rejected'; changed(); return json(options(body.key)); + } + if (url.pathname.endsWith('/skills')) return json({ skills: [] }); + if (url.pathname.endsWith('/git')) return json({ files: [], branch: 'main' }); + if (url.pathname === '/api/events') return route.fulfill({ contentType: 'text/event-stream', body: ': fixture\n\n' }); + throw new Error(`Unexpected request: ${url.pathname}`); + }); + const css = fs.readFileSync('engine/src/web/style.css', 'utf8'); + await page.route('**/review/**', async route => { + const url = new URL(route.request().url()); + if (url.pathname.includes('/rounds/')) return route.fulfill({ json: detail('a', selected.a) }); + const html = `
${['a', 'b', 'c'].map((unit, index) => `
Change ${index + 1}

Old ${unit}

${selected[unit]} ${unit}

`).join('')}
`; + return route.fulfill({ contentType: 'text/html', body: rewriteReviewHtml(html, '/review/paper') }); + }); + const frame = () => page.frameLocator('.review-frame'), drawer = page.locator('.review-note-controls[data-open="true"]'); + const sync = () => expect.poll(() => page.evaluate(() => { + // The review round can replace the iframe. Resolve its current window + // in the parent context and wait until the host has attached listeners. + const current = document.querySelector('.review-frame'); + const doc = current?.contentDocument, view = current?.contentWindow; + if (!view || !doc?.querySelector('#paper') || !doc.querySelector('#papilot-embed-style')) return false; + view.dispatchEvent(new view.Event('latex-review:decided')); + return true; + }), { timeout: 15000 }).toBe(true); + const tab = name => drawer.getByRole('tab', { name, exact: true }); + const comparison = revision => drawer.locator(`.note-option-comparison[data-revision="${revision}"]`); + try { + await page.goto(origin); + await frame().getByRole('button', { name: 'Show note 1: Improve note 1.', exact: true }).click(); + await expect(comparison('a-current-v1')).toBeVisible(); + failPreview = true; version++; changed(); await sync(); + await expect(comparison('a-current-v2')).toBeVisible(); + await tab('Alternative a').click(); + await expect(drawer.getByRole('alert')).toBeVisible(); + await expect(drawer.locator('.note-option-comparison')).toHaveCount(0); + await expect(drawer.getByRole('button', { name: 'Accept Alternative a', exact: true })).toBeDisabled(); + await drawer.getByRole('button', { name: 'Retry', exact: true }).click(); + await expect(comparison('a-alternative-v2')).toBeVisible(); + await expect(comparison('a-alternative-v2').locator('.note-comparison-side.after')).toContainText('alternative a'); + await expect(drawer.getByRole('alert')).toHaveCount(0); + assert.equal(writes.length, 0, 'switching and retrying previews never saves a choice'); + + failPreview = false; await tab('Original a').click(); + let releaseLate; latePreview = new Promise(resolve => { releaseLate = resolve; }); + version++; changed(); await sync(); + await expect(comparison('a-current-v3')).toBeVisible(); + await expect.poll(() => previews.some(request => request.unit === 'a' && request.option === 'alternative' && request.version === 3)).toBe(true); + await tab('Alternative a').click(); + await expect(drawer.locator('.note-option-comparison')).toHaveCount(0); + await tab('Original a').click(); releaseLate(); + await expect(comparison('a-current-v3')).toBeVisible(); + await expect(drawer.getByRole('alert')).toHaveCount(0); + await drawer.getByRole('button', { name: 'Accept Original a', exact: true }).click(); + await expect(drawer.locator('.review-note-position')).toHaveText('Note 3 / 4'); + await expect(comparison('b-current-v3')).toBeVisible(); + assert.equal(decisions.a, 'accepted', 'shared notes are skipped only after confirmed acceptance'); + + await tab('Alternative b').click(); + await expect(comparison('b-alternative-v3')).toBeVisible(); + await drawer.getByRole('button', { name: 'Accept Alternative b', exact: true }).click(); + await expect.poll(() => pendingBuild?.unit).toBe('b'); + await sync(); + await expect(drawer.locator('.review-note-position')).toHaveText('Note 3 / 4'); + selected.b = pendingBuild.option; decisions.b = 'accepted'; pendingBuild = null; + task.status = 'reviewing'; task.review.round++; version++; changed(); await sync(); + await expect(drawer.locator('.review-note-position')).toHaveText('Note 4 / 4'); + await expect(comparison('c-current-v4')).toBeVisible(); + + saveFails = true; + await drawer.getByRole('button', { name: 'Reject Original c', exact: true }).click(); + await expect(drawer.getByRole('alert')).toContainText('The source changed'); + await expect(drawer.locator('.review-note-position')).toHaveText('Note 4 / 4'); + assert.equal(decisions.c, 'pending'); + saveFails = false; + await drawer.getByRole('button', { name: 'Reject Original c', exact: true }).click(); + await expect.poll(() => decisions.c).toBe('rejected'); + await expect(drawer.locator('.review-note-position')).toHaveText('Note 4 / 4'); + await expect(drawer.getByRole('button', { name: 'Accept Original c', exact: true })).toBeDisabled(); + assert.equal(writes.at(-1).id, writes.at(-2).id, 'a failed decision keeps its retry identity'); + // The final drawer stays mounted. An undo elsewhere must permit a new + // decision, while an unconfirmed retry above keeps the original ID. + for (const action of ['reject', 'accept', 'accept']) { + const previous = writes.at(-1).id; + undone.add(previous); decisions.c = 'pending'; version++; changed(); await sync(); + const button = drawer.getByRole('button', { name: `${action === 'accept' ? 'Accept' : 'Reject'} Original c`, exact: true }); + await expect(button).toBeEnabled(); await button.click(); + await expect.poll(() => decisions.c).toBe(action === 'accept' ? 'accepted' : 'rejected'); + assert.notEqual(writes.at(-1).id, previous, 'a confirmed decision retires its request identity before undo'); + await expect(drawer.locator('.review-note-position')).toHaveText('Note 4 / 4'); + } + assert.deepEqual(errors, []); + await page.screenshot({ path: `test-results/note-options-flow-${width}.png` }); + console.log(`PASS ${width}px: preview failure/retry, stale response isolation, confirmed acceptance, shared-note skipping, asynchronous alternative save, final drawer retention and decisions after undo`); + } catch (error) { + await page.screenshot({ path: `test-results/note-options-flow-error-${width}.png` }); + throw error; + } finally { await page.close(); } + } +} finally { await browser.close(); await server.close(); } diff --git a/scripts/check-note-results.mjs b/scripts/check-note-results.mjs index 81a055cd..70cd3c40 100644 --- a/scripts/check-note-results.mjs +++ b/scripts/check-note-results.mjs @@ -251,8 +251,8 @@ try { assert.equal(decisions[2].c1, 'pending'); rejectFails = false; await controls().getByRole('button', { name: 'Reject Concise', exact: true }).click(); - await controls().waitFor({ state: 'hidden' }); - assert.equal(decisions[2].c1, 'rejected'); + await expect.poll(() => decisions[2].c1).toBe('rejected'); + await expect(controls()).toHaveAttribute('data-open', 'true'); await frame().locator('.papilot-note-options button').waitFor({ state: 'hidden' }); assert.equal(optionWrites.length, 2); assert.ok(optionWrites.every(body => body.reject === true && body.accept === undefined && body.option === 'brief')); diff --git a/scripts/check-playground.mjs b/scripts/check-playground.mjs index 20a9c64e..bf561e9f 100644 --- a/scripts/check-playground.mjs +++ b/scripts/check-playground.mjs @@ -149,8 +149,14 @@ try { await notePanel.getByRole('textbox', { name: 'Comment on Initial proposal', exact: true }).fill('Keep the author’s emphasis here.'); await page.screenshot({ path: 'test-results/demo-prepared-task.png' }); await notePanel.getByRole('button', { name: 'Reject Initial proposal', exact: true }).click(); - await expect(notePanel).toBeHidden(); + await expect(notePanel).toBeVisible(); + await expect(notePanel.getByRole('button', { name: 'Reject Initial proposal', exact: true })).toBeDisabled(); + await expect(notePanel.getByRole('textbox', { name: 'Comment on Initial proposal', exact: true })).toHaveValue('Keep the author’s emphasis here.'); const rejectedChange = page.frameLocator('iframe.review-frame').locator('.change[data-state="rejected"]'); + await expect(rejectedChange).toHaveAttribute('data-note-focus', 'true'); + // The final note remains inspectable until the author explicitly closes it. + await rejectedChange.locator('.change-where').click(); + await expect(notePanel).toBeHidden(); await expect(rejectedChange).toHaveAttribute('data-resolved', 'before'); await expect(rejectedChange.locator('.side.after')).toBeHidden(); await page.reload(); @@ -178,7 +184,9 @@ try { await expect(notePanel.locator('.note-comparison-change[data-related="true"]')).toHaveCount(1); await notePanel.getByRole('button', { name: 'Accept Initial proposal', exact: true }).click(); await expect(page.locator('[data-task="example-note-math"] .task-badge')).toHaveText('1'); - await expect(notePanel.getByRole('button', { name: 'Accept Initial proposal', exact: true })).toBeDisabled(); + await expect(notePanel.getByLabel('Note 1 of 2', { exact: true })).toBeVisible(); + await expect(notePanel.getByRole('button', { name: 'Accept Initial proposal', exact: true })).toBeEnabled(); + await expect(page.frameLocator('iframe.review-frame').locator('.papilot-linked-target[data-state="pending"]')).toHaveCount(1); await expect(page.frameLocator('iframe.review-frame').locator('.change[data-state="accepted"]')).toHaveCount(1); await expect(page.frameLocator('iframe.review-frame').locator('.change[data-state="pending"]')).toHaveCount(1); await openManuscript(); @@ -390,4 +398,9 @@ try { assert.deepEqual(failed, [], 'all static assets resolve under /demo/'); assert.deepEqual(errors, []); console.log('PASS: precise notes, prepared tasks and note-to-diff links, review decisions, model controls, local Git diff/stage/commit/sync/restore/history, demo reset, dismissible banner, download gates, mobile layout and zero API requests.'); -} catch (error) { await page.screenshot({ path: 'test-results/playground-failure.png' }); console.error({ errors, failed, apiRequests }); throw error; } finally { await browser.close(); await new Promise(resolve => server.close(resolve)); } +} catch (error) { + await page.screenshot({ path: 'test-results/playground-failure.png' }); + const changes = await page.frameLocator('iframe.review-frame').locator('.change').evaluateAll(elements => elements.map(el => ({ data: { ...el.dataset }, classes: el.className, round: el.ownerDocument.documentElement.dataset.papilotRound }))).catch(() => []); + console.error(JSON.stringify({ errors, failed, apiRequests, changes })); + throw error; +} finally { await browser.close(); await new Promise(resolve => server.close(resolve)); } diff --git a/scripts/check-review-application.mjs b/scripts/check-review-application.mjs new file mode 100644 index 00000000..0271a8a6 --- /dev/null +++ b/scripts/check-review-application.mjs @@ -0,0 +1,297 @@ +// Browser -> Papilot -> real review server -> real Git and LaTeX. +// Only the agent is inert. All repositories, state and browser profiles are disposable. +import assert from 'node:assert/strict'; +import fs from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; +import { execFileSync } from 'node:child_process'; +import { EventEmitter } from 'node:events'; +import { AsyncLocalStorage } from 'node:async_hooks'; +import { Builder } from '../engine/src/core/build.ts'; +import { chromium, expect } from '../engine/node_modules/@playwright/test/index.mjs'; + +const temp = fs.mkdtempSync(path.join(os.tmpdir(), 'papilot-review-application-')); +process.env.PAPILOT_DATA = path.join(temp, 'data'); +delete process.env.PAPILOT_DESKTOP; +// Exercise ordinary manuscript previews separately from the review's dated PDFs. +delete process.env.SOURCE_DATE_EPOCH; +const latexmk = execFileSync('which', ['latexmk'], { encoding: 'utf8' }).trim(); +const calls = path.join(temp, 'compiles.jsonl'), fail = path.join(temp, 'fail'), hold = path.join(temp, 'hold'); +const bin = path.join(temp, 'bin'); fs.mkdirSync(bin); +fs.writeFileSync(path.join(bin, 'latexmk'), `#!${process.execPath} +const fs = require('node:fs'); +const build = process.argv.includes('-outdir=build'); +const parts = process.cwd().split(require('node:path').sep); +if (build) fs.appendFileSync(${JSON.stringify(calls)}, JSON.stringify({ cwd: process.cwd(), key: parts[parts.lastIndexOf('builds') + 1], args: process.argv.slice(2) }) + String.fromCharCode(10)); +if (build && fs.existsSync(${JSON.stringify(fail)})) { console.error('Intentional compilation failure'); process.exit(1); } +const deadline = Date.now() + 30000; +while (build && fs.existsSync(${JSON.stringify(hold)}) && Date.now() < deadline) Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0, 30); +const result = require('node:child_process').spawnSync(${JSON.stringify(latexmk)}, process.argv.slice(2), { stdio: 'inherit' }); +process.exit(result.status ?? 1); +`, { mode: 0o755 }); +process.env.PATH = bin + path.delimiter + process.env.PATH; +const [{ Fixture }, { createServer }, { Store }, { Workspace }, { Manuscripts }, { buildApp, gitQueue }, { Updates }, { AGENT_IDS }] = await Promise.all([ + import('../engine/tests/ts/helpers.ts'), import('../engine/src/server.ts'), import('../src/server/store.ts'), + import('../src/server/workspace.ts'), import('../src/server/manuscript.ts'), import('../src/server/app.ts'), + import('../src/server/updates.ts'), import('../src/shared.ts'), +]); +class InertAgent extends EventEmitter { ready = false; close() {} } +const f = new Fixture(), store = new Store(path.join(temp, 'data/state.sqlite3')), manuscripts = new Manuscripts(); +let server, reviewServer, browser, page, workspace, updates; +const compiles = () => fs.existsSync(calls) ? fs.readFileSync(calls, 'utf8').trim().split('\n').filter(Boolean).map(line => { + const entry = JSON.parse(line); + assert.match(entry.key || '', /^[a-f0-9]{32}$/, 'every counted latexmk belongs to a review build key'); + return entry; +}) : []; +const count = () => compiles().length; +fs.mkdirSync('test-results', { recursive: true }); +const report = { decisions: 0, intermediateHostCompiles: 0, finalPredictionCompiles: 0, applicationCompiles: 0, appliedPreviewCompiles: 0, refreshes: 0, predictions: 0, errors: [] }; +// Observe real builds without replacing any compilation, snapshot or cache. +// A count of every latexmk would conflate the engine's per-decision PDF with +// the host's manuscript preview. Associate each invocation with its build key. +const owner = new AsyncLocalStorage(), observedBuilds = [], buildJobs = new Set(), predictionJobs = new Set(); +const originalBuild = Builder.prototype.build; +Builder.prototype.build = function (rev, targets, seed, options = {}) { + const entry = { owner: owner.getStore() || 'engine', rev, key: null, resultKey: null, status: 'building' }; + observedBuilds.push(entry); + const job = originalBuild.call(this, rev, targets, seed, { ...options, onKey(key) { entry.key = key; options.onKey?.(key); } }).then(result => { + entry.status = 'ready'; entry.resultKey = result.key; return result; + }, error => { entry.status = 'failed'; entry.error = error.message; throw error; }); + buildJobs.add(job); + void job.finally(() => buildJobs.delete(job)).catch(() => undefined); + return job; +}; +const state = manuscripts.state.bind(manuscripts), refresh = manuscripts.refresh.bind(manuscripts), predict = manuscripts.predict.bind(manuscripts); +manuscripts.state = (...args) => owner.run('host-state', () => state(...args)); +manuscripts.refresh = (...args) => { report.refreshes++; return owner.run('host-refresh', () => refresh(...args)); }; +manuscripts.predict = (...args) => { + report.predictions++; + const job = owner.run('prediction', () => predict(...args)); + predictionJobs.add(job); + void job.finally(() => predictionJobs.delete(job)).catch(() => undefined); + return job; +}; +const keysFor = kinds => new Set(observedBuilds.filter(build => kinds.includes(build.owner)).flatMap(build => [build.key, build.resultKey]).filter(Boolean)); +const countKeys = (records, keys) => records.filter(record => keys.has(record.key)).length; +const settledBuilds = async () => { + await expect.poll(() => predictionJobs.size, { timeout: 60000 }).toBe(0); + await expect.poll(() => buildJobs.size, { timeout: 60000 }).toBe(0); +}; +try { + f.write('paper/main.tex', '\\documentclass{article}\n\\begin{document}\n\\input{first}\n\\input{second}\n\\input{third}\n\\end{document}\n'); + for (const name of ['first', 'second', 'third']) f.write(`paper/${name}.tex`, `\\section{${name}}\nOriginal ${name} sentence.\n`); + f.repo.run(['add', '.']); f.repo.run(['commit', '-qm', 'Browser batching fixture']); + const engine = f.app(); await engine.ready; + const proposal = engine.tasks.create('batching', 'Review batching', [], ['paper'], true); + for (const name of ['first', 'second', 'third']) f.edit(proposal, `paper/${name}.tex`, `Original ${name}`, `Revised ${name}`); + engine.publisher.submit(proposal.name); await engine.publisher.join(); + assert.equal(engine.publisher.current(proposal.name)?.status, 'done', JSON.stringify(engine.publisher.current(proposal.name))); + reviewServer = createServer(engine); await reviewServer.listen({ host: '127.0.0.1', port: 0 }); + engine.url = `http://127.0.0.1:${reviewServer.server.address().port}`; + const call = async (route, body) => { + const response = await fetch(engine.url + route, { method: body === undefined ? 'GET' : 'POST', + headers: { 'Content-Type': 'application/json', 'X-Review-Token': engine.csrf }, ...(body === undefined ? {} : { body: JSON.stringify(body) }) }); + const result = await response.json(); + if (!response.ok) throw new Error(result.error || response.statusText); + return result; + }; + const client = { url: engine.url, token: engine.csrf, get: route => call(route), post: (route, body = {}) => call(route, body) }; + workspace = new Workspace(store, new InertAgent(), async () => client, Object.fromEntries(AGENT_IDS.filter(id => id !== 'codex').map(id => [id, new InertAgent()]))); + store.saveProject({ id: 'paper', name: 'Batching test paper', path: f.root, createdAt: Date.now() }); + store.saveTask({ id: 'batching', projectId: 'paper', name: proposal.name, title: 'Review batching', instructions: '', worktree: proposal.worktree, + deferred: true, threadId: null, turnId: null, model: null, status: 'reviewing', createdAt: Date.now(), updatedAt: Date.now(), feedbackCursor: 0, error: null, review: null }); + await workspace.refresh('batching'); + const settle = async () => { + let result; + await expect.poll(async () => { result = await manuscripts.state('paper', f.root); return result.build.status; }, { timeout: 60000 }).not.toBe('building'); + return result.build; + }; + assert.equal((await settle()).status, 'ready'); + const listener = { kind: 'local', port: 0, publicOrigin: '', owner: '' }; + updates = new Updates({ root: process.cwd(), data: process.env.PAPILOT_DATA, version: '0.9.2', port: 0, supported: false, busy: () => false, shutdown() {}, latest: async () => '0.9.2' }); + server = await buildApp({ store, workspace, manuscripts, listener, csrf: 'batching-test', serialGit: gitQueue(), dist: path.resolve('dist'), version: '0.9.2', connectReview: async () => client, updates }); + await server.listen({ host: '127.0.0.1', port: 0 }); listener.port = server.server.address().port; + const origin = `http://127.0.0.1:${listener.port}`; + console.log(`Isolated web test: ${origin}, real review service: ${engine.url}`); + browser = await chromium.launch({ channel: process.env.PAPILOT_BROWSER_CHANNEL || 'chrome', headless: true }); + page = await browser.newPage({ locale: 'en-US', viewport: { width: 1100, height: 900 } }); + page.setDefaultTimeout(15000); + page.on('pageerror', error => report.errors.push(error.message)); + page.on('response', response => { if (response.status() >= 500) report.errors.push(`${response.status()} ${response.url()}`); }); + await page.addInitScript(() => { localStorage.setItem('papilot:task', 'batching'); localStorage.setItem('papilot:project', 'paper'); localStorage.setItem('papilot:language', 'en'); }); + await page.goto(origin); + const viewer = () => page.frameLocator('.review-frame'); + await expect(viewer().locator('.change.current')).toBeVisible(); + assert.equal(engine.review.summary(proposal.name).counts.pending, 3); + await settledBuilds(); + const start = count(), refreshStart = report.refreshes, predictionStart = report.predictions, round = engine.review.latestRound(proposal.name).number; + const decisionResponse = route => page.waitForResponse(response => new URL(response.url()).pathname === `/review/paper/api/tasks/batching/${route}` && response.request().method() === 'POST'); + const choose = async (action, pending) => { + const response = decisionResponse('decide'); + await viewer().locator('.change.current').getByRole('button', { name: action, exact: true }).click(); + const confirmed = await response; assert.ok(confirmed.ok(), await confirmed.text()); await confirmed.finished(); + await expect.poll(() => engine.review.summary(proposal.name).counts.pending).toBe(pending); + if (pending) await expect(viewer().locator('.change.current').getByRole('button', { name: 'Accept', exact: true })).toBeEnabled(); + report.decisions++; + }; + await choose('Accept', 2); + await choose('Reject', 1); + // Undo through the real embedded viewer, then reload with persisted choices. + await viewer().locator('.change.current .change-number').click(); + const undoResponse = decisionResponse('undo'); + await page.keyboard.press('ControlOrMeta+z'); + const undone = await undoResponse; assert.ok(undone.ok(), await undone.text()); await undone.finished(); + await expect.poll(() => engine.review.summary(proposal.name).counts.pending).toBe(2); + await page.reload(); + await expect(viewer().locator('.bar .reject-all')).toBeEnabled(); + await choose('Accept', 1); + // Read the authoritative proxied state: its summary waits for the queued + // settlement. A pending choice must not start any host prediction/build. + await page.evaluate(async () => { + const response = await fetch('/review/paper/api/tasks/batching/state'); + if (!response.ok) throw new Error('Could not read settled review state'); + await response.json(); + }); + await engine.applied.join(); await settledBuilds(); + report.intermediateHostCompiles = countKeys(compiles().slice(start), keysFor(['host-state', 'host-refresh', 'prediction'])); + assert.equal(report.intermediateHostCompiles, 0, 'intermediate decisions never compile the manuscript'); + assert.equal(report.refreshes, refreshStart, 'intermediate decisions never refresh the manuscript'); + assert.equal(report.predictions, predictionStart, 'unfinished choices never predict a manuscript'); + + await choose('Accept', 0); + const apply = () => viewer().getByRole('button', { name: 'Apply to workspace', exact: true }); + await expect(apply()).toBeEnabled(); + await expect.poll(() => observedBuilds.some(build => build.owner === 'prediction' && build.status === 'ready'), { timeout: 60000 }).toBe(true); + await engine.applied.join(); await settledBuilds(); + const finalKeys = keysFor(['prediction']); + assert.equal(finalKeys.size, 1, 'all final-selection predictions share one build key'); + report.finalPredictionCompiles = countKeys(compiles().slice(start), finalKeys); + assert.equal(report.finalPredictionCompiles, 1, 'the final manuscript is compiled exactly once before Apply'); + assert.equal(report.refreshes, refreshStart, 'preparing the final selection does not rewrite or refresh the manuscript'); + for (const name of ['first', 'second', 'third']) assert.match(f.read(`paper/${name}.tex`), /Original/); + + // The engine's PDF of accepted decisions is a separate dated build. It may + // compile intermediate choices; its work must not be counted as host work. + const applied = await engine.applied.state(proposal.name); + assert.equal(applied.shown?.current, true); + assert.equal(applied.building, false); + assert.ok(!finalKeys.has(applied.shown.key), 'the applied-paper preview and manuscript prediction have independent keys'); + report.appliedPreviewCompiles = countKeys(compiles().slice(start), keysFor(['engine'])); + assert.ok(report.appliedPreviewCompiles > 0, 'real applied-paper previews were exercised'); + + // Reopening the Apply controls requests another prediction, but its real + // compiler result must be reused. Do not require exactly one HTTP request. + const predictionsBeforeReload = report.predictions, compilesBeforeReload = count(); + const requestedPrediction = page.waitForResponse(response => new URL(response.url()).pathname === '/api/tasks/batching/predict' && response.ok()); + await page.reload(); await expect(apply()).toBeEnabled(); await requestedPrediction; + await expect.poll(() => report.predictions).toBeGreaterThan(predictionsBeforeReload); + await engine.applied.join(); await settledBuilds(); + assert.deepEqual([...keysFor(['prediction'])], [...finalKeys]); + assert.equal(count(), compilesBeforeReload, 'reload reuses the final prediction and applied-paper caches'); + assert.equal(engine.review.latestRound(proposal.name).number, round); + await page.screenshot({ path: 'test-results/review-application-before-apply.png' }); + + const beforeApply = count(); + await apply().click(); + await expect.poll(() => store.task('batching').delivery?.mode).toBe('workspace'); + await expect.poll(() => report.refreshes - refreshStart).toBe(1); + assert.equal((await settle()).status, 'ready'); + await engine.applied.join(); await settledBuilds(); + report.applicationCompiles = count() - beforeApply; + assert.equal(report.applicationCompiles, 0, 'Apply reuses the final prediction without another latexmk invocation'); + assert.equal(report.refreshes - refreshStart, 1, 'the actual manuscript write refreshes its displayed state once'); + assert.equal(countKeys(compiles().slice(start), finalKeys), 1); + assert.match(f.read('paper/first.tex'), /Revised first/); + assert.match(f.read('paper/second.tex'), /Revised second/); + assert.match(f.read('paper/third.tex'), /Revised third/); + console.log('PASS: intermediate decisions avoid host builds; the final selection compiles once; reload and Apply reuse it; applied-paper previews are counted separately.'); + + // A new task exercises a real in-flight comparison, compiler failure and retry. + f.repo.run(['add', '.']); f.repo.run(['commit', '-qm', 'Applied selection']); + const retry = engine.tasks.create('retry', 'Compilation recovery', [], ['paper'], true); + f.edit(retry, 'paper/first.tex', 'Revised first', 'Recovery first'); + f.edit(retry, 'paper/third.tex', 'Revised third', 'Recovery third'); + store.saveTask({ ...store.task('batching'), id: 'retry', name: retry.name, title: 'Compilation recovery', worktree: retry.worktree, delivery: null, review: null }); + await workspace.publish('retry'); + await page.getByRole('button', { name: /^Compilation recovery/ }).click(); + await expect(viewer().locator('.bar .accept-all')).toBeEnabled(); + // Record transient false success messages too: waiting for a toast to + // disappear could otherwise pass only because its six-second timer expired. + await viewer().locator('body').evaluate(body => { + window.readyWhileUnavailable = []; + const record = () => { + const toast = [...body.querySelectorAll('.toast')].find(element => element.textContent.trim() === 'Changes are ready'); + const unavailable = body.querySelector('.comparison-waiting, .bar .status[data-state="building"], .bar .status[data-state="build_failed"]'); + if (toast && unavailable) window.readyWhileUnavailable.push(unavailable.textContent.trim()); + }; + new MutationObserver(record).observe(body, { childList: true, subtree: true, characterData: true, attributes: true, attributeFilter: ['data-state'] }); + record(); + }); + const readyToast = () => viewer().locator('.toast').filter({ hasText: /^Changes are ready$/ }); + const noFalseReady = async () => { + assert.equal(await readyToast().count(), 0, 'an unavailable comparison never claims that changes are ready'); + assert.deepEqual(await viewer().locator('body').evaluate(() => window.readyWhileUnavailable), [], 'no transient ready toast while comparisons build or fail'); + }; + f.edit(retry, 'paper/first.tex', 'Recovery first', 'Updated first'); + fs.writeFileSync(hold, ''); + await page.locator('.review-rebuild').click(); + await expect(page.locator('.review-rebuild')).toHaveAttribute('aria-busy', 'true'); + await expect(viewer().locator('.bar .status[data-state="building"]')).toHaveCSS('clip-path', 'inset(50%)'); + await expect(viewer().locator('.comparison-waiting[data-state="pending"]').first()).toBeVisible(); + await noFalseReady(); + assert.equal(await viewer().locator('.bar').evaluate(el => el.offsetHeight), 44); + await page.screenshot({ path: 'test-results/review-application-real-build.png' }); + fs.rmSync(hold); + await expect(page.locator('.review-rebuild')).toHaveAttribute('aria-busy', 'false', { timeout: 60000 }); + await expect(viewer().locator('.bar .accept-all')).toBeEnabled(); + await expect(readyToast()).toBeVisible(); + const goodRound = engine.review.latestRound(retry.name).number; + + f.edit(retry, 'paper/first.tex', 'Updated first', 'Broken first'); fs.writeFileSync(fail, ''); + await page.locator('.review-rebuild').click(); + await expect(page.locator('.review-rebuild')).toHaveAttribute('data-state', 'failed', { timeout: 60000 }); + await expect(viewer().locator('.bar .status[data-state="build_failed"]')).toBeVisible(); + // This fixture has one carried change and one missing comparison. The + // usable single change keeps its own controls; a batch requires two. + await expect(viewer().locator('.bar .accept-all')).toHaveCount(0); + const carried = viewer().locator('.change:has(.change-carried)'); + await expect(carried).toHaveCount(1); + await expect(carried.getByRole('button', { name: 'Accept', exact: true })).toBeEnabled(); + await expect(carried.getByRole('button', { name: 'Reject', exact: true })).toBeEnabled(); + assert.equal(engine.review.latestRound(retry.name).number, goodRound + 1, 'a failed build keeps its new source round'); + assert.equal(engine.review.latestRound(retry.name).visuals, 'failed'); + await expect(viewer().locator('.change-carried').first()).toContainText('PDF from'); + await expect(viewer().locator('.comparison-waiting').first()).toContainText('PDF comparison failed'); + await expect(viewer().locator('.change:has(.comparison-waiting) button.accept')).toHaveCount(0); + await expect(viewer().locator('.change.current')).toBeVisible(); + await noFalseReady(); + await page.screenshot({ path: 'test-results/review-application-failed.png' }); + fs.rmSync(fail); await page.locator('.review-rebuild').click(); + await expect(page.locator('.review-rebuild')).toHaveAttribute('data-state', 'ready', { timeout: 60000 }); + await expect(viewer().locator('.bar .accept-all')).toBeEnabled(); + await expect(viewer().locator('.bar .status')).toHaveCount(0); + assert.equal(engine.review.latestRound(retry.name).visuals, 'ready'); + await expect(viewer().locator('.comparison-waiting')).toHaveCount(0); + await expect(readyToast()).toBeVisible(); + assert.deepEqual(await viewer().locator('body').evaluate(() => window.readyWhileUnavailable), [], 'success is announced only when the comparison is ready'); + assert.deepEqual(report.errors, []); + console.log('PASS: real comparison progress, carried pages, source-first failure gating and retry recover without stuck controls'); + report.builds = observedBuilds; report.compiles = compiles(); + fs.writeFileSync('test-results/review-application.json', JSON.stringify(report, null, 2)); +} catch (error) { + console.error(error); + console.error('Task at failure:', JSON.stringify(store.tasks())); + fs.writeFileSync('test-results/review-application.json', JSON.stringify({ ...report, builds: observedBuilds, compilerLog: fs.existsSync(calls) ? fs.readFileSync(calls, 'utf8') : '' }, null, 2)); + if (page) { await page.screenshot({ path: 'test-results/review-application-error.png' }); fs.writeFileSync('test-results/review-application-error.txt', await page.locator('body').innerText()); } + throw error; +} finally { + fs.rmSync(hold, { force: true }); + if (browser) await browser.close(); + if (server) await server.close(); + workspace?.close(); updates?.close(); + await manuscripts.stop(); + if (reviewServer) await reviewServer.close(); + await f.close(); store.close(); + Builder.prototype.build = originalBuild; + fs.rmSync(temp, { recursive: true, force: true }); +} diff --git a/scripts/check-review-flow.mjs b/scripts/check-review-flow.mjs index 5a3dd3e3..6f960171 100644 --- a/scripts/check-review-flow.mjs +++ b/scripts/check-review-flow.mjs @@ -25,7 +25,7 @@ const actions = [], writes = [], messageRequests = [], commitRequests = [], appl let gitFiles = []; let pushError, conflictOnApply = false, combinedOnCommit = false; let timeline = []; -const state = round => ({ task: 'flow', title: task.title, csrf: 'fixture', closed: false, state: task.review.state, latest, history: Object.keys(rounds).map(number => ({ number: Number(number), created: 1793322000 + Number(number) * 120 })), round, revision: `after-${round}`, decisions: rounds[round], comments: {}, undo, job: null }); +const state = round => ({ task: 'flow', title: task.title, csrf: 'fixture', closed: false, state: task.review.state, latest, history: Object.keys(rounds).map(number => ({ number: Number(number), created: 1793322000 + Number(number) * 120 })), round, revision: `after-${round}`, decisions: rounds[round], comments: {}, undo, job: task.review.job || null }); function update() { const values = Object.values(rounds[latest]); task.review.counts = Object.fromEntries(['pending', 'accepted', 'rejected', 'commented'].map(value => [value, values.filter(v => v === value).length])); @@ -372,6 +372,44 @@ try { await expect(viewer().getByRole('progressbar')).toBeInViewport(); await expect.poll(() => viewer().locator('.change.current .dock').evaluate(el => getComputedStyle(el).position)).not.toBe('fixed'); } + // An engine-side build without a host spinner must keep its own progress. + task.review.state = 'building'; task.review.job = { id: 'engine-build', status: 'running', phase: 'build' }; + await sync(); + await expect(page.locator('.review-rebuild')).toHaveAttribute('aria-busy', 'false'); + await expect(viewer().locator('.bar .status[data-state="building"]')).toHaveCSS('clip-path', 'none'); + delete task.review.job; update(); await sync(); + // Rebuilding an existing comparison keeps the toolbar's height. In + // particular, the phase label must not wrap below the batch buttons. + for (const language of ['en', 'zh-CN']) { + await page.evaluate(language => localStorage.setItem('papilot:language', language), language); + await page.reload(); + await viewer().locator('.bar .accept-all').waitFor(); + for (const width of [1440, 1100, 900]) { + await page.setViewportSize({ width, height: 1000 }); + const bar = viewer().locator('.bar'); + await expect(bar.locator('.status[data-state="building"]')).toHaveCount(0); + await bar.evaluate(el => el.ownerDocument.fonts.ready); + // Platform fonts can wrap the ready toolbar at narrow desktop widths. + // Compilation must preserve that layout, whatever its initial height. + const height = await bar.evaluate(el => el.offsetHeight); + task.status = 'building'; task.review.state = 'building'; + task.review.job = { id: 'toolbar-build', status: 'running', phase: 'build' }; + await sync(); + await expect(page.locator('.review-rebuild')).toHaveAttribute('aria-busy', 'true'); + await expect(bar.locator('.status[data-state="building"]')).toHaveCSS('clip-path', 'inset(50%)'); + // These comparisons are already usable while the next round builds. + await expect(bar.locator('.accept-all')).toBeEnabled(); + await expect(bar.locator('.reject-all')).toBeEnabled(); + assert.equal(await bar.evaluate(el => el.offsetHeight), height, `compilation preserves the ${language} toolbar at ${width}px`); + if (language === 'zh-CN' && width === 1100) await page.screenshot({ path: 'test-results/review-building-zh.png' }); + task.status = 'reviewing'; delete task.review.job; update(); await sync(); + await expect(bar.locator('.accept-all')).toBeEnabled(); + await expect(page.locator('.review-rebuild')).toHaveAttribute('aria-busy', 'false'); + } + } + await page.evaluate(() => localStorage.setItem('papilot:language', 'en')); + await page.reload(); + await viewer().locator('.change.current').waitFor(); // The shared reader switches to phone controls at 680px. The toolbar can // take a second row, preserving progress, navigation and tools. for (const width of [680, 640, 390, 320]) { @@ -385,6 +423,16 @@ try { await expect(page.locator('.review-tools .review-rebuild')).toBeInViewport(); await expect.poll(() => viewer().locator('.change.current .dock').evaluate(el => getComputedStyle(el).position)).toBe('fixed'); assert.equal(await viewer().locator('#paper').evaluate(el => el.scrollWidth > el.clientWidth + 1), false); + await expect(viewer().locator('.bar .status[data-state="building"]')).toHaveCount(0); + const height = await viewer().locator('.bar').evaluate(el => el.offsetHeight); + task.status = 'building'; task.review.state = 'building'; task.review.job = { id: 'phone-build', status: 'running', phase: 'build' }; + await sync(); + await expect(page.locator('.review-rebuild')).toHaveAttribute('aria-busy', 'true'); + await expect(viewer().locator('.bar .status[data-state="building"]')).toHaveCSS('clip-path', 'inset(50%)'); + assert.equal(await viewer().locator('.bar').evaluate(el => el.offsetHeight), height, 'phone compilation preserves toolbar height'); + task.status = 'reviewing'; delete task.review.job; update(); await sync(); + await expect(page.locator('.review-rebuild')).toHaveAttribute('aria-busy', 'false'); + await expect(viewer().locator('.bar .status[data-state="building"]')).toHaveCount(0); } await page.setViewportSize({ width: 1440, height: 1000 }); // Resizing can make a different change the visible navigation target. diff --git a/scripts/check-source-edit.mjs b/scripts/check-source-edit.mjs index 6b5778e9..b7e0690f 100644 --- a/scripts/check-source-edit.mjs +++ b/scripts/check-source-edit.mjs @@ -19,6 +19,7 @@ await server.listen({ host: '127.0.0.1', port: 0 }); app.url = `http://127.0.0.1:${server.server.address().port}`; const browser = await chromium.launch({ channel: process.env.PAPILOT_BROWSER_CHANNEL || 'chrome', headless: true }); const page = await browser.newPage({ viewport: { width: 1200, height: 850 } }); +page.setDefaultTimeout(15000); const errors = []; page.on('pageerror', error => errors.push(error.message)); const edit = () => page.locator('.source-editor').getByRole('button', { name: '编辑', exact: true }); const field = () => page.getByRole('textbox', { name: '编辑 LaTeX 源码' }); @@ -29,13 +30,13 @@ const waitRound = async number => { }; const open = async () => { const source = page.locator('.source'); - if (!await source.isVisible()) await page.getByRole('button', { name: 'LaTeX', exact: true }).click(); + if (!await source.isVisible()) await page.locator('.change.current').getByRole('button', { name: 'LaTeX', exact: true }).click(); await edit().click(); }; try { await page.goto(app.url + '/?task=manual&lang=zh-Hans'); - await expect(page.locator('.change')).toBeVisible(); - await page.getByRole('button', { name: 'LaTeX', exact: true }).click(); + await expect(page.locator('.change.current')).toBeVisible(); + await page.locator('.change.current').getByRole('button', { name: 'LaTeX', exact: true }).click(); const originalBox = await page.locator('.source-code').boundingBox(); await open(); await expect(page.locator('.source-code')).toHaveCount(1); @@ -121,6 +122,10 @@ Literal symbols: \% and \$; \verb|% $ { }|. assert.equal(f.read(METHOD, task.worktree), f.read(METHOD)); assert.deepEqual(errors, []); console.log('Source editing passed: one source surface, in-place edit and diff switching, header icons, syntax highlighting, wrapping and scrolling, draft recovery, keyboard save, real PDF rebuild, stale write refusal, compilation repair, mobile layout, empty review.'); +} catch (error) { + console.error(error); + await page.screenshot({ path: 'test-results/source-edit-error.png' }).catch(() => {}); + throw error; } finally { await browser.close(); await server.close(); await f.close(); } diff --git a/src/server/app.ts b/src/server/app.ts index 720f713c..162d24af 100644 --- a/src/server/app.ts +++ b/src/server/app.ts @@ -10,7 +10,7 @@ import { createHash, randomUUID } from 'node:crypto'; import { registerFrontend } from './frontend.ts'; import type { Store } from './store.ts'; import type { Workspace } from './workspace.ts'; -import { AGENT_TURN, reviewClient, allowedReviewRoute, rewriteReviewHtml, rewriteReviewJson, confirmedReviewResponse, annotateReviewState, taskRoute } from './review.ts'; +import { AGENT_TURN, reviewClient, readReview, allowedReviewRoute, rewriteReviewHtml, rewriteReviewJson, confirmedReviewResponse, annotateReviewState, taskRoute } from './review.ts'; import { fullPaperPdf, nextPaperPass, paperPassPdf } from './pdf.ts'; import { type ChangeAction, Changes } from './changes.ts'; import { bibliography, confirmEntry } from './references.ts'; @@ -20,7 +20,7 @@ import { gitOverview } from './git-overview.ts'; import { findSkill, listedSkill, loadSkills } from './skills.ts'; import { registerSkillRoutes } from './skill-routes.ts'; import { type Listener, equals, guard } from './access.ts'; -import { isTaskReviewNote, isAgentId, isMarkKind, PRESET_QUICK_NOTES, QUICK_NOTE_LIMIT, type AgentId, type NoteSummary, type QuickNote } from '../shared.ts'; +import { isTaskReviewNote, isAgentId, isMarkKind, PRESET_QUICK_NOTES, QUICK_NOTE_LIMIT, type AgentId, type NoteSummary, type QuickNote, type Task } from '../shared.ts'; import { parseGroupInstructions } from '../note-groups.ts'; import { noteOptionContext, noteOptionsKey } from './note-options.ts'; import { taskNoteResults } from './note-results.ts'; @@ -36,24 +36,36 @@ import { systemRequirements } from './system-requirements.ts'; import { findExecutable } from '../../bin/agent-paths.mjs'; const importsByWorkspace = new WeakMap(); +const deliveredToManuscript = (delivery: Task['delivery']) => Boolean(delivery && !delivery.pending && delivery.mode !== 'branch'); +const completedLegacyBatch = (task: Task) => !task.deferred && Boolean(task.review && !task.review.counts.pending); +const legacyBatchListeners = new WeakSet(); // A review write's reply does not wait for the task's summary to be read // again: that runs right after it, once at a time per task, and once more // for writes that arrive meanwhile. The review page's reads of the task wait // for it, so its next poll shows what the reply used to. The workspace does // not wait, since one slow engine would hold every paper's state: the // summary's update announces itself through the event stream. -const settling = new WeakMap; again: boolean }>>(); -function settle(workspace: Workspace, taskId: string) { +const settling = new WeakMap; again: boolean; legacy: boolean }>>(); +function settle(workspace: Workspace, taskId: string, legacy = false) { let tasks = settling.get(workspace); if (!tasks) settling.set(workspace, tasks = new Map()); const running = tasks.get(taskId); - if (running) { running.again = true; return; } - const entry = { done: Promise.resolve(), again: false }; + if (running) { running.again = true; running.legacy ||= legacy; return; } + const entry = { done: Promise.resolve(), again: false, legacy }; + tasks.set(taskId, entry); entry.done = (async () => { - try { do { entry.again = false; await workspace.refresh(taskId); } while (entry.again); } - finally { tasks.delete(taskId); } + try { + let confirmed = false, confirmation: Promise | undefined; + do { + entry.again = false; + try { confirmation = workspace.refresh(taskId); confirmed = await confirmation; } + catch { confirmed = false; } + } while (entry.again); + const current = confirmed && entry.legacy && workspace.store.findTask(taskId); + if (current && completedLegacyBatch(current) && !current.pendingReview && !current.pendingBuild) + workspace.emit('reviewed', taskId, confirmation); + } finally { if (tasks.get(taskId) === entry) tasks.delete(taskId); } })().catch(() => undefined); - tasks.set(taskId, entry); } async function settled(workspace: Workspace, taskId: string) { await settling.get(workspace)?.get(taskId)?.done; @@ -141,24 +153,43 @@ export async function buildApp(options: AppOptions) { if (id) projectRequests.set(request, workspace.enterProjectRequest(id)); }); app.addHook('onSend', async (request, _reply, payload) => { projectRequests.get(request)?.(); return payload; }); - // Refresh when manuscript files can have changed. Deferred decisions and - // source edits affect only the task copy; its manuscript waits for Apply. - const WRITES = /^\/api\/projects\/:id\/git\/|^\/api\/tasks\/:id\/(?:commit|apply|reconcile)$|^\/review\/:projectId\/(?:api\/changes\/(?:decide|undo)|\*)$/; + // Refresh after applying the reviewed result. Intermediate decisions only + // update the proposal; the final selection can still be predicted ahead. + const WRITES = /^\/api\/projects\/:id\/git\/|^\/api\/tasks\/:id\/(?:commit|apply)$|^\/review\/:projectId\/(?:api\/changes\/(?:decide|undo)|\*)$/; + const completedTaskWrites = new WeakSet(); app.addHook('onResponse', async (request, reply) => { if (request.method !== 'POST' || reply.statusCode >= 400 || !WRITES.test(request.routeOptions.url || '')) return; - if (request.url.startsWith('/review/')) { - const review = /^\/review\/[^/]+\/api\/tasks\/([^/]+)\/(decide|undo)(?:\?|$)/.exec(request.url); - if (review) { - const current = store.tasks().find(task => task.name === decodeURIComponent(review[1])); - if (current?.deferred || (request.body as { action?: string } | undefined)?.action === 'comment') return; - } else if (!/^\/review\/[^/]+\/api\/changes\/(decide|undo)(?:\?|$)/.test(request.url)) return; - } const params = (request.params || {}) as Record; try { + // Save routes can return a confirmation or merge suggestion without + // changing the manuscript. A status check is not itself a write either. + if (request.routeOptions.url?.startsWith('/api/tasks/') && !completedTaskWrites.has(request)) return; + // Proxy decisions settle their summaries after replying and flush a + // completed legacy batch once, even when several writes share a read. + if (request.routeOptions.url === '/review/:projectId/*') return; const project = store.project(params.projectId ?? (request.routeOptions.url!.startsWith('/api/tasks/') ? store.task(params.id).projectId : params.id)); void manuscripts.refresh(project.id, project.path).catch(() => undefined); } catch { /* The paper or task is gone. */ } }); + // Decisions and resumed alternatives announce their confirmed result here. + // Flush only the completed legacy batch, once per workspace even with + // multiple listeners; a pending build announces its result when it finishes. + if (!legacyBatchListeners.has(workspace)) { + legacyBatchListeners.add(workspace); + const completed = new WeakSet>(); + workspace.on('reviewed', (taskId: string, confirmation?: Promise) => { + try { + const task = store.task(taskId); + if (!completedLegacyBatch(task) || task.pendingReview || task.pendingBuild) return; + // A resumed build and a decision can await the same summary read. + // Its promise identifies that confirmation without retaining tasks. + if (confirmation && completed.has(confirmation)) return; + if (confirmation) completed.add(confirmation); + const project = store.project(task.projectId); + void manuscripts.refresh(project.id, project.path).catch(() => undefined); + } catch { /* The paper or task is gone. */ } + }); + } app.addHook('onError', async request => { projectRequests.get(request)?.(); }); await app.register(compress); app.setErrorHandler((error, _request, reply) => { reply.code((error as any).statusCode || 400).send({ error: (error as Error).message }); }); @@ -336,25 +367,29 @@ export async function buildApp(options: AppOptions) { return { notes: await taskNoteResults(task, store.items(task.id), store.runs().filter(run => run.taskId === task.id), client) }; }); app.get<{ Params: { id: string }; Querystring: { key: string } }>('/api/tasks/:id/note-options', task, async request => { - const current = store.task(request.params.id), client = await connectReview(store.project(current.projectId).path); - return (await noteOptionContext(store, current, string(request.query.key, 'Note key', 200), client)).options; + const project = store.project(store.task(request.params.id).projectId); + return readReview(connectReview, project.path, async client => + (await noteOptionContext(store, store.task(request.params.id), string(request.query.key, 'Note key', 200), client)).options); }); app.get<{ Params: { id: string }; Querystring: { key: string; round: string; option: string; retry?: string; comparison?: string } }>('/api/tasks/:id/note-options/preview', task, async request => { - const current = store.task(request.params.id), client = await connectReview(store.project(current.projectId).path); - const context = await noteOptionContext(store, current, string(request.query.key, 'Note key', 200), client); - if (context.options.round !== Number(request.query.round)) throw new Error('The review changed. Reload the options.'); - if (request.query.comparison && request.query.comparison !== context.options.comparisonKey) throw new Error('The review changed. Reload the options.'); - if (request.query.option === context.options.selected && (!context.rejected.length || context.options.locked)) return rewriteReviewJson(context.detail, `/review/${current.projectId}`); - if (context.options.locked) throw new Error(context.options.locked); - const option = context.options.options.find(option => option.id === request.query.option); - if (!option) throw new Error('This option is no longer available.'); - if (option.previewError && request.query.retry !== '1') throw new Error(option.previewError); - const detail = await client.post(`${taskRoute(current.name)}/source/options/preview`, { id: option.id, round: context.options.round, edits: option.edits, rejected: context.rejected }, 120000); - if (option.previewError) { - delete option.previewError; - store.savePreference(noteOptionsKey(current.id, context.options.key, context.options.round), context.options); - } - return rewriteReviewJson(detail, `/review/${current.projectId}`); + const project = store.project(store.task(request.params.id).projectId); + return readReview(connectReview, project.path, async client => { + const current = store.task(request.params.id); + const context = await noteOptionContext(store, current, string(request.query.key, 'Note key', 200), client); + if (context.options.round !== Number(request.query.round)) throw new Error('The review changed. Reload the options.'); + if (request.query.comparison && request.query.comparison !== context.options.comparisonKey) throw new Error('The review changed. Reload the options.'); + if (request.query.option === context.options.selected && (!context.rejected.length || context.options.locked)) return rewriteReviewJson(context.detail, `/review/${current.projectId}`); + if (context.options.locked) throw new Error(context.options.locked); + const option = context.options.options.find(option => option.id === request.query.option); + if (!option) throw new Error('This option is no longer available.'); + if (option.previewError && request.query.retry !== '1') throw new Error(option.previewError); + const detail = await client.post(`${taskRoute(current.name)}/source/options/preview`, { id: option.id, round: context.options.round, edits: option.edits, rejected: context.rejected }, 120000); + if (option.previewError) { + delete option.previewError; + store.savePreference(noteOptionsKey(current.id, context.options.key, context.options.round), context.options); + } + return rewriteReviewJson(detail, `/review/${current.projectId}`); + }); }); app.post<{ Params: { id: string }; Body: { key: string; round: number; option?: string; id?: string; accept?: boolean; reject?: boolean } }>('/api/tasks/:id/note-options', task, async request => { const current = store.task(request.params.id), client = await connectReview(store.project(current.projectId).path), body = request.body; @@ -381,7 +416,10 @@ export async function buildApp(options: AppOptions) { return value; }, { route, body: input }); if (result.error) throw new Error(result.error); - await workspace.refresh(current.id); + // Share the proxy's settlement: a preceding undo and this final + // drawer choice may still be reading the same summary. + settle(workspace, current.id, !current.deferred); + await settled(workspace, current.id); return (await noteOptionContext(store, store.task(current.id), body.key, client)).options; } const route = `${taskRoute(current.name)}/source/options`, input = { id: string(body.id, 'Option request ID', 80), round: body.round, edits: option.edits, ...(body.accept ? { accept: true } : {}) }; @@ -402,11 +440,8 @@ export async function buildApp(options: AppOptions) { } throw new Error('Select an existing option.'); }); - // The Apply and Commit step is on screen: compile in the background what - // applying the decided task will write, so the manuscript's PDF is ready or - // on its way when the author saves it. Individual decisions start no build. - // Once every change of a task is decided, its manuscript as applying it - // would write compiles at once, so the PDF is ready when the author applies. + // When the final selection is ready or Apply/Commit appears, predict what + // applying writes. Intermediate decisions do not predict a manuscript. const predict = (taskId: string) => { const current = store.task(taskId), project = store.project(current.projectId); if (!current.deferred || current.mode === 'review' || current.delivery?.pending || current.status === 'deleting' || (current.review?.counts.pending ?? 0) > 0) return; @@ -470,14 +505,18 @@ export async function buildApp(options: AppOptions) { return workspace.enqueue(task.id, text, string(b.requestKey, 'Request ID', 100), true, { skill, feedback, reviewNotes, attachments }); }); // Commits what the author accepted from this task, with the message they confirmed. - app.post<{ Params: { id: string } }>('/api/tasks/:id/commit', task, request => { + app.post<{ Params: { id: string } }>('/api/tasks/:id/commit', task, async request => { const task = store.task(request.params.id), b = request.body as any; - return serialGit(task.projectId, () => workspace.commitTask(task.id, string(b.message, 'Commit message', 5000), b.push === true)); + const result = await serialGit(task.projectId, () => workspace.commitTask(task.id, string(b.message, 'Commit message', 5000), b.push === true)); + if ('delivery' in result ? deliveredToManuscript(result.delivery) : result.commit) completedTaskWrites.add(request); + return result; }); - app.post<{ Params: { id: string } }>('/api/tasks/:id/apply', task, request => { + app.post<{ Params: { id: string } }>('/api/tasks/:id/apply', task, async request => { const task = store.task(request.params.id), b = (request.body || {}) as any; if (b.mode !== undefined && b.mode !== 'workspace' && b.mode !== 'commit') throw new Error('Invalid save action.'); - return serialGit(task.projectId, () => workspace.applyTask(task.id, b.keepCurrent === true, b.mode === 'commit' ? { message: string(b.message, 'Commit message', 5000), ...(b.confirmation ? { confirmation: string(b.confirmation, 'Commit confirmation', 100) } : {}) } : undefined)); + const result = await serialGit(task.projectId, () => workspace.applyTask(task.id, b.keepCurrent === true, b.mode === 'commit' ? { message: string(b.message, 'Commit message', 5000), ...(b.confirmation ? { confirmation: string(b.confirmation, 'Commit confirmation', 100) } : {}) } : undefined)); + if (deliveredToManuscript(result.delivery)) completedTaskWrites.add(request); + return result; }); app.post<{ Params: { id: string } }>('/api/tasks/:id/commit-message', task, request => { const task = store.task(request.params.id); @@ -816,7 +855,7 @@ export async function buildApp(options: AppOptions) { if (request.method === 'POST' && !reviewTask) throw new Error('This review task does not belong to this workspace.'); let result: unknown; try { result = await (reviewTask ? workspace.reviewWrite(reviewTask.id, writesFiles, forward, writesFiles ? { route, body: request.body as Record } : undefined, besideAgent) : forward()); } - finally { if (settles) settle(workspace, settles); } + finally { if (settles) settle(workspace, settles, !reviewTask?.deferred && /\/(decide|undo)$/.test(route)); } return annotateReviewState(result, store.tasks().filter(task => task.projectId === project.id)); }, }); diff --git a/src/server/review.ts b/src/server/review.ts index 2892cd51..970d4d86 100644 --- a/src/server/review.ts +++ b/src/server/review.ts @@ -20,6 +20,16 @@ export function reviewConnectionError(error: unknown): boolean { return ['AbortError', 'TimeoutError'].includes(error.name) || /fetch failed|ECONNREFUSED|ECONNRESET|socket hang up|The review server failed to start/.test(error.message); } +// A preview is a repeatable read, even when the engine prepares its cache. +// Reconnect once if the engine restarts between connecting and reading it. +export async function readReview(connect: (root: string) => Promise, root: string, read: (client: ReviewClient) => Promise): Promise { + try { return await read(await connect(root)); } + catch (error) { + if (!reviewConnectionError(error)) throw error; + return read(await connect(root)); + } +} + export function confirmedReviewResponse(status: number, value: unknown, task: string): boolean { if (!value || typeof value !== 'object') return false; const result = value as Record; diff --git a/src/server/workspace.ts b/src/server/workspace.ts index 2ade7fcc..31c56925 100644 --- a/src/server/workspace.ts +++ b/src/server/workspace.ts @@ -118,7 +118,7 @@ export class Workspace extends EventEmitter { private reviewWrites = new Set(); // Settles when the task's review write in reviewWrite finishes. private reviewWriting = new Map>(); - private refreshes = new Map; again: boolean }>(); + private refreshes = new Map; again: boolean }>(); // Papers with a commit in progress: no review write may change their files meanwhile. private committing = new Set(); private commentWrites = new Map(); @@ -917,7 +917,10 @@ ${this.agentOf(task) === 'antigravity' ? 'Use file tools for manuscript edits. S const response = await fetch(client.url + pending.route, { method: 'POST', headers: { 'Content-Type': 'application/json', 'X-Review-Token': client.token }, body: JSON.stringify(pending.body), signal: AbortSignal.timeout(120000), redirect: 'error' }); const value = await response.json() as { error?: string }; if (!confirmedReviewResponse(response.status, value, task.name)) throw new Error('The review engine could not confirm this action. Check again when it is available.'); - await this.refresh(task.id); + // Keep the replay identity until both the action and its resulting + // summary are confirmed; old completed counts cannot confirm an undo. + const confirmation = this.refresh(task.id); + if (!await confirmation) return this.store.task(task.id); const sourceBuild = response.ok && startsBuild(pending.route, value); this.store.transaction(() => { // The status the task reached meanwhile, recorded by holdForReview. A @@ -931,6 +934,9 @@ ${this.agentOf(task) === 'antigravity' ? 'Use file tools for manuscript edits. S // So does the build held after the agent's turn for this action, and a // build whose connection was lost meanwhile. if (sourceBuild || this.store.task(task.id).pendingBuild) void this.resumeBuild(task.id); + // The replay succeeded even if a previous build's error stays visible. + // A still-pending build delays the legacy refresh until its own result. + if (response.ok && /\/(decide|undo)$/.test(pending.route)) this.emit('reviewed', task.id, confirmation); return this.store.task(task.id); } finally { this.reviewWrites.delete(task.id); this.schedule(); } } @@ -1138,7 +1144,10 @@ ${this.agentOf(task) === 'antigravity' ? 'Use file tools for manuscript edits. S } await delay(1500); } - await this.refresh(taskId); + // A saved note option may have accepted the last legacy change while + // this job ran. Notify listeners only after its result is confirmed. + const confirmation = this.refresh(taskId); + if (await confirmation) this.emit('reviewed', taskId, confirmation); } catch (e) { if (this.closing) return; if (reviewConnectionError(e)) { @@ -1174,25 +1183,29 @@ ${this.agentOf(task) === 'antigravity' ? 'Use file tools for manuscript edits. S // Reads the task's review summary again. Reads of one task run one at a // time, and a call during a read reads once more after it, so an older // answer never replaces a newer one. Each call resolves after a read that - // started after it. - refresh(taskId: string): Promise { + // started after it. The result says whether the final read was confirmed; + // failed reads keep the visible summary but cannot confirm a completed batch. + refresh(taskId: string): Promise { const running = this.refreshes.get(taskId); if (running) { running.again = true; return running.done; } - const entry = { done: Promise.resolve(), again: false }; + const entry = { done: Promise.resolve(false), again: false }; + this.refreshes.set(taskId, entry); entry.done = (async () => { - try { do { entry.again = false; await this.readReview(taskId); } while (entry.again); } - finally { this.refreshes.delete(taskId); } + try { + let confirmed = false; + do { entry.again = false; confirmed = await this.readReview(taskId); } while (entry.again); + return confirmed; + } finally { if (this.refreshes.get(taskId) === entry) this.refreshes.delete(taskId); } })(); - this.refreshes.set(taskId, entry); return entry.done; } private async readReview(taskId: string) { let release: (() => void) | undefined; try { const task = this.store.task(taskId); - if (task.mode === 'review') return; + if (task.mode === 'review') return false; release = this.enterProjectRequest(task.projectId); - if (task.status === 'deleting') return; + if (task.status === 'deleting') return false; const project = this.store.project(task.projectId), client = await this.connectReview(project.path); const summary = await client.get(taskRoute(task.name)); const { unsent, comments } = await this.unsentReviewFeedback(client, this.store.task(taskId)); @@ -1214,7 +1227,8 @@ ${this.agentOf(task) === 'antigravity' ? 'Use file tools for manuscript edits. S && !current.pendingReview && !this.store.runByKey('merge-review-' + reconciliation.id) && !this.store.runs().some(run => run.taskId === taskId && run.status === 'queued')) await this.queueMergeSuggestion(taskId, reconciliation); - } catch { /* The task remains visible even if its project is temporarily unavailable. */ } + return current.status !== 'deleting'; + } catch { /* The task remains visible even if its project is temporarily unavailable. */ return false; } finally { release?.(); } } // Review decisions, comments and undo updates the agent has not received yet. diff --git a/src/web/note-results.tsx b/src/web/note-results.tsx index f9b5d3a3..477909e3 100644 --- a/src/web/note-results.tsx +++ b/src/web/note-results.tsx @@ -11,7 +11,7 @@ import { NotePreviewCache } from './note-preview-cache.ts'; export interface NoteFocus { key: string; change: NoteChange; sequence: number; historical: boolean } type OptionsRequest = (id: string, key: string, body?: { round: number; option?: string; id?: string; accept?: boolean; reject?: boolean }) => Promise; -type Preview = { key: string; round: number; detail?: RoundDetail } | null; +type Preview = { key: string; round: number; option: string; version?: string; detail?: RoundDetail } | null; type PreviewRequest = (id: string, key: string, round: number, option: string, retry?: boolean, version?: string) => Promise; const previewKey = (id: string, key: string, round: number, option: string, version?: string) => JSON.stringify([id, key, round, option, version]); const comparisonCache = new NotePreviewCache(); @@ -40,6 +40,8 @@ function optionLockText(reason: string) { default: return errorText(reason); } } +const optionError = (message: string) => /fetch failed|failed to fetch|networkerror|load failed/i.test(message) + ? t('Request failed. Please try again.') : errorText(message); async function warmComparison(detail: RoundDetail) { const strips = [...detail.sections.flatMap(section => section.blocks), ...detail.loose].flatMap(block => block.kind === 'context' ? [block.image] : block.kind === 'change' ? [...block.before, ...block.after] : []); // Decode the first visible strips before swapping, so a ready comparison @@ -60,7 +62,7 @@ interface Links { cachedPreview: (id: string, key: string, round: number, option: string, version?: string) => RoundDetail | undefined; notes: NoteResult[]; error: boolean; loading: boolean; selected: string; reveal: number; focus: NoteFocus | null; scope: 'all' | 'note'; setScope: (scope: 'all' | 'note') => void; reload: () => void; - clearFocus: () => void; + clearFocus: () => void; decisionSaved: (key: string) => void; alternativeCounts: Record; focusNote: (key: string) => void; showChange: (key: string, change: NoteChange) => void; showNote: (key: string) => void; @@ -76,6 +78,7 @@ export function NoteResultsProvider({ task, items, read, optionsRequest, preview const [selected, setSelected] = useState(''), [focus, setFocus] = useState(null); const [reveal, setReveal] = useState(0), [scope, setScope] = useState<'all' | 'note'>('all'); const [preview, setPreview] = useState(null); + const [advance, setAdvance] = useState(null); const [available, setAvailable] = useState>({}); const sequence = useRef(0), actions = useRef({ read, openReview, openChat }); actions.current = { read, openReview, openChat }; const signature = items.filter(item => item.type === 'user' && (item.meta?.notes?.length || readNotesInstructions(item.text))).map(item => item.id).join(','); @@ -87,7 +90,7 @@ export function NoteResultsProvider({ task, items, read, optionsRequest, preview }, [id, signature]); const decisions = JSON.stringify(task?.review?.counts); useEffect(() => { reload(); return () => { ++sequence.current; }; }, [reload, task?.review?.round, task?.status, task?.updatedAt, decisions]); - useEffect(() => { setNotes([]); setAvailable({}); setScope('all'); setPreview(null); setFocus(null); setSelected(''); setError(false); }, [id]); + useEffect(() => { setNotes([]); setAvailable({}); setScope('all'); setPreview(null); setFocus(null); setSelected(''); setError(false); setAdvance(null); }, [id]); const showNote = (key: string) => { setSelected(key); setReveal(value => value + 1); actions.current.openChat(); requestAnimationFrame(() => { @@ -110,6 +113,20 @@ export function NoteResultsProvider({ task, items, read, optionsRequest, preview const note = notes.find(note => note.key === focus.key); if (note?.changes[0] && note.changes[0].round !== focus.change.round) setFocus({ ...focus, change: note.changes[0], sequence: focus.sequence + 1, historical: note.changes[0].round !== task?.review?.round }); }, [notes]); + useEffect(() => { + if (!advance) return; + if (focus?.key !== advance || ['failed', 'uncertain', 'interrupted'].includes(task?.status || '')) { setAdvance(null); return; } + if (task?.status !== 'reviewing' || task.pendingReview) return; + const current = notes.find(note => note.key === advance); + if (!current?.changes.length || current.changes.some(change => change.state === 'pending')) return; + // Read the confirmed decisions first: several notes can share one edit. + // Skip those together and keep the last result open when the batch is done. + const siblings = notes.filter(note => note.itemId === current.itemId), index = siblings.indexOf(current); + const next = [...siblings.slice(index + 1), ...siblings.slice(0, index)].find(note => note.round === task.review?.round + && note.changes.length && note.changes.every(change => change.state === 'pending')); + setAdvance(null); + if (next) showChange(next.key, next.changes[0]); + }, [advance, notes, focus?.key, task?.status, task?.pendingReview, task?.review?.round]); const requests = useRef({ optionsRequest, previewRequest }); requests.current = { optionsRequest, previewRequest }; const noteSignature = JSON.stringify(notes.map(note => [note.key, note.round, note.changes.map(change => change.state)])); useEffect(() => { @@ -139,7 +156,8 @@ export function NoteResultsProvider({ task, items, read, optionsRequest, preview }, [id, task?.status, task?.review?.round, task?.updatedAt, decisions, noteSignature]); const value: Links = { task, optionsRequest, previewRequest: previewRequest && ((id, key, round, option, retry, version) => comparisonCache.read(previewKey(id, key, round, option, version), () => previewRequest(id, key, round, option, retry, version).then(warmComparison))), cachedPreview: (id, key, round, option, version) => comparisonCache.peek(previewKey(id, key, round, option, version)), preview: preview?.key === focus?.key && preview?.round === task?.review?.round ? preview : null, setPreview, notes, loading, error, selected, reveal, focus, scope, setScope, reload, showNote, focusNote, alternativeCounts: Object.fromEntries(notes.map(note => [note.key, task?.status === 'reviewing' && note.round === task.review?.round && note.changes.every(change => change.state === 'pending') && available[note.key]?.round === note.round ? available[note.key].count : 0])), - clearFocus: () => { setScope('all'); setPreview(null); setFocus(null); setSelected(''); }, showChange }; + clearFocus: () => { setScope('all'); setPreview(null); setFocus(null); setSelected(''); setAdvance(null); }, + decisionSaved: key => { setAdvance(key); reload(); }, showChange }; return {children}; } @@ -255,9 +273,8 @@ function NoteAlternatives({ note, navigation }: { note: NoteResult; navigation: const [source, setSource] = useState(false); const [data, setData] = useState(remembered || null), [selected, setSelected] = useState(() => rememberedChoice(storageKey, remembered)); const [busy, setBusy] = useState(false), [applying, setApplying] = useState(false), [error, setError] = useState(''); - const [baseline, setBaseline] = useState(() => task && remembered ? links.cachedPreview(task.id, note.key, remembered.round, remembered.selected || 'current', remembered.comparisonKey) || null : null), [baselineError, setBaselineError] = useState(''); + const [baseline, setBaseline] = useState<{ key: string; detail: RoundDetail } | null>(null), [baselineError, setBaselineError] = useState(''); const [retry, setRetry] = useState(0), [previewBusy, setPreviewBusy] = useState(false); - const [shown, setShown] = useState(() => task && remembered ? links.cachedPreview(task.id, note.key, remembered.round, selected, remembered.comparisonKey) || baseline : null); const previewSequence = useRef(0), requestId = useRef<{ option: string; action: 'accept' | 'reject'; id: string } | null>(null); useEffect(() => { if (applying && task && ['failed', 'uncertain', 'interrupted'].includes(task.status)) { @@ -279,9 +296,11 @@ function NoteAlternatives({ note, navigation }: { note: NoteResult; navigation: useEffect(() => { if (editor) commentInput.current?.focus(); }, [editor]); useEffect(() => { if (!task || !data || !links.previewRequest) return; - let cancelled = false; setBaseline(links.cachedPreview(task.id, note.key, data.round, saved, data.comparisonKey) || null); setBaselineError(''); - void links.previewRequest(task.id, note.key, data.round, saved, false, data.comparisonKey).then(detail => { - if (!cancelled) setBaseline(detail); + const key = previewKey(task.id, note.key, data.round, saved, data.comparisonKey); + const cached = links.cachedPreview(task.id, note.key, data.round, saved, data.comparisonKey); + let cancelled = false; setBaseline(cached ? { key, detail: cached } : null); setBaselineError(''); + void links.previewRequest(task.id, note.key, data.round, saved, retry > 0, data.comparisonKey).then(detail => { + if (!cancelled) setBaseline({ key, detail }); }).catch(error => { if (!cancelled) setBaselineError(error.message); }); return () => { cancelled = true; }; }, [task?.id, note.key, data?.round, data?.comparisonKey, saved, retry]); @@ -289,12 +308,13 @@ function NoteAlternatives({ note, navigation }: { note: NoteResult; navigation: const sequence = ++previewSequence.current; setPreviewBusy(false); if (!task || !data || !links.previewRequest || links.focus?.key !== note.key || selected === saved || data.locked) { links.setPreview(null); return; } + const identity = { key: note.key, round: data.round, option: selected, version: data.comparisonKey }; const cached = links.cachedPreview(task.id, note.key, data.round, selected, data.comparisonKey); - if (cached) { links.setPreview({ key: note.key, round: data.round, detail: cached }); return; } - links.setPreview({ key: note.key, round: data.round }); setPreviewBusy(true); + if (cached) { links.setPreview({ ...identity, detail: cached }); return; } + links.setPreview(identity); setPreviewBusy(true); void links.previewRequest(task.id, note.key, data.round, selected, retry > 0, data.comparisonKey).then(detail => { if (previewSequence.current !== sequence) return; - links.setPreview({ key: note.key, round: data.round, detail }); + links.setPreview({ ...identity, detail }); }).catch(error => { if (previewSequence.current === sequence) setError(error.message); }) .finally(() => { if (previewSequence.current === sequence) setPreviewBusy(false); }); return () => { ++previewSequence.current; }; @@ -319,10 +339,12 @@ function NoteAlternatives({ note, navigation }: { note: NoteResult; navigation: // Read the prepared comparisons; tab switches share the same decoded cache. for (const option of data.options.filter(option => !option.previewError)) void links.previewRequest(task.id, note.key, data.round, option.id, false, data.comparisonKey).catch(() => {}); }, [task?.id, note.key, data?.round, data?.comparisonKey, data?.locked, optionIds]); - const readyComparison = selected === saved ? baseline : links.preview?.detail; - useEffect(() => { if (readyComparison) setShown(readyComparison); }, [readyComparison]); if (!request || !task) return null; - const comparison = readyComparison || shown; + // A tab must never label another option's PDF, including while loading or + // after an error. Late responses are matched to the complete comparison key. + const comparison = data && (selected === saved + ? baseline?.key === previewKey(task.id, note.key, data.round, saved, data.comparisonKey) ? baseline.detail : undefined + : links.preview?.option === selected && links.preview.version === data.comparisonKey ? links.preview.detail : undefined); const focusedUnits = [...new Set(note.changes.filter(change => change.round === comparison?.round).flatMap(change => change.units))]; const locked = data?.locked || (note.changes.some(change => change.state !== 'pending') ? 'This note includes a reviewed change. Undo that decision before replacing it.' : task.status !== 'reviewing' || task.pendingReview ? 'Wait for the current task and review actions to finish.' : ''); const choose = (option: string) => { @@ -341,10 +363,13 @@ function NoteAlternatives({ note, navigation }: { note: NoteResult; navigation: // Rejection restores the original edit and never waits for an option PDF. if (action === 'accept') await links.previewRequest(task.id, note.key, data.round, option.id, false, data.comparisonKey); const result = await request(task.id, note.key, { round: data.round, option: option.id, id: requestId.current.id, ...(action === 'accept' ? { accept: true } : { reject: true }) }); + // The drawer can remain mounted through undo. Only an unconfirmed + // request may reuse its ID; the next decision needs a fresh identity. + requestId.current = null; const rejected = optionFeedback(option.id, 'reject'); if (rejected) draft.remove(rejected.id); browserStorage.removeItem(storageKey); - if ('options' in result) { setData(result); setSelected(result.selected || 'current'); setApplying(false); links.clearFocus(); } - links.reload(); + if ('options' in result) { setData(result); setSelected(result.selected || 'current'); setApplying(false); } + links.decisionSaved(note.key); } catch (error) { setError((error as Error).message); setApplying(false); } }; @@ -383,7 +408,7 @@ function NoteAlternatives({ note, navigation }: { note: NoteResult; navigation: : {name})} {navigation} {active &&
- + @@ -396,11 +421,11 @@ function NoteAlternatives({ note, navigation }: { note: NoteResult; navigation:
} {comparison && (source ? : )} - {!comparison && !baselineError && !error &&

{t('Loading PDF comparison…')}

} - {baselineError && !comparison &&

{baselineError}

} + {!comparison && !(selected === saved && baselineError) && !error &&

{t('Loading PDF comparison…')}

} + {selected === saved && baselineError && !comparison &&

{optionError(baselineError)}

} {(saving || previewBusy) && {saving ? t('Saving choice…') : t('Preparing this option’s PDF diff…')}} {locked && !saving &&

{optionLockText(locked)}

} - {error &&

{errorText(error)}

} + {error &&

{optionError(error)}

} ; } diff --git a/src/web/review-frame.tsx b/src/web/review-frame.tsx index 83c3b799..b363ccff 100644 --- a/src/web/review-frame.tsx +++ b/src/web/review-frame.tsx @@ -239,6 +239,9 @@ function compactReviewFrame(doc: Document, reserve: number, lead: number, change /* Reserve the outer tools' row when there is too little room. Keep progress, batch counts and history; shorten only the batch labels. */ .bar{container:review-bar/inline-size;height:auto;gap:8px} + /* The host's compile button already shows build activity. Keep the phase + available to screen readers without pushing decisions into another row. */ + :root[data-papilot-build-indicator] .bar .status[data-state="building"]{position:absolute;width:1px;height:1px;padding:0;margin:-1px;overflow:hidden;clip-path:inset(50%)} .bar .accept-all,.bar .reject-all{flex-shrink:0;white-space:nowrap} .bar .batch-count{margin-left:0;background:var(--bg-muted);color:var(--text-2)} :root[data-papilot-toolbar-stacked] .bar{padding:calc(var(--papilot-bar-height,44px) + 4px) 8px 4px;min-height:calc(var(--papilot-bar-height,44px) + 44px)} @@ -382,6 +385,11 @@ export function ReviewFrame({ task, changesOf, tools, lead, completion, onCommen useEffect(() => { document?.defaultView?.dispatchEvent(new CustomEvent(VIEW_EVENT, { detail: view })); }, [document, view]); + useEffect(() => { + // Manuscript comparisons have their own build. Hide the duplicate phase + // only while the task's host toolbar is showing its compilation spinner. + document?.documentElement.toggleAttribute('data-papilot-build-indicator', task?.status === 'building'); + }, [document, task?.status]); useEffect(() => { const target = document?.defaultView; if (!target) return; diff --git a/src/web/review-note-connections.tsx b/src/web/review-note-connections.tsx index b58ee1a5..e4bba70a 100644 --- a/src/web/review-note-connections.tsx +++ b/src/web/review-note-connections.tsx @@ -45,24 +45,35 @@ export function ReviewNoteConnections({ doc, round }: { doc: Document | null; ro useEffect(() => { const focus = links?.focus; const selected = links?.notes.find(note => note.key === focus?.key); - for (const place of places) { - const linked = Boolean(links?.scope === 'note' && selected && place.notes.some(note => note.key === selected.key)); - place.element.classList.toggle('papilot-linked-change', linked); - place.element.classList.toggle('papilot-linked-target', linked && place.round === focus?.change.round && place.stop === focus.change.stop); - } - if (!doc || !focus || (jumped.current?.doc === doc && jumped.current.sequence === focus.sequence)) return; - const place = places.find(place => place.round === focus.change.round && place.stop === focus.change.stop && place.notes.some(note => note.key === focus.key)); - if (!place) return; - place.element.click(); - // Scroll only the PDF pane: scrollIntoView can also move the host page - // horizontally or leave the target behind the bottom drawer. - const paper = doc.getElementById('paper'); - if (paper) { - const padding = parseFloat(doc.defaultView?.getComputedStyle(paper).scrollPaddingTop || '') || 0; - paper.scrollBy({ top: place.element.getBoundingClientRect().top - paper.getBoundingClientRect().top - padding - 12, behavior: 'instant' }); + const highlight = () => { + for (const place of places) { + const linked = Boolean(links?.scope === 'note' && selected && place.notes.some(note => note.key === selected.key)); + const target = linked && place.round === focus?.change.round && place.stop === focus.change.stop; + if (place.element.classList.contains('papilot-linked-change') !== linked) place.element.classList.toggle('papilot-linked-change', linked); + if (place.element.classList.contains('papilot-linked-target') !== target) place.element.classList.toggle('papilot-linked-target', target); + } + }; + highlight(); + // The embedded renderer owns className and can repaint after our focus + // update. Restore the host's markers without scrolling or moving focus. + const observer = new MutationObserver(highlight); + for (const place of places) observer.observe(place.element, { attributes: true, attributeFilter: ['class'] }); + if (doc && focus && !(jumped.current?.doc === doc && jumped.current.sequence === focus.sequence)) { + const place = places.find(place => place.round === focus.change.round && place.stop === focus.change.stop && place.notes.some(note => note.key === focus.key)); + if (place) { + place.element.click(); + // Scroll only the PDF pane: scrollIntoView can also move the host page + // horizontally or leave the target behind the bottom drawer. + const paper = doc.getElementById('paper'); + if (paper) { + const padding = parseFloat(doc.defaultView?.getComputedStyle(paper).scrollPaddingTop || '') || 0; + paper.scrollBy({ top: place.element.getBoundingClientRect().top - paper.getBoundingClientRect().top - padding - 12, behavior: 'instant' }); + } + place.element.focus({ preventScroll: true }); + jumped.current = { doc, sequence: focus.sequence }; + } } - place.element.focus({ preventScroll: true }); - jumped.current = { doc, sequence: focus.sequence }; + return () => observer.disconnect(); }, [doc, places, links?.focus, links?.scope]); useEffect(() => { const target = doc?.defaultView; diff --git a/tests/app.test.ts b/tests/app.test.ts index 6bbaebd4..3ba5a722 100644 --- a/tests/app.test.ts +++ b/tests/app.test.ts @@ -1077,7 +1077,236 @@ async function decisionEngine(state: (route: string, body: any) => Record(resolve => engine.close(() => resolve())); } }; } -test('a review decision replies before the task summary is read again, and the next poll waits for that read', async () => { +// A real proxy with independently gated summaries: decisions answer before +// the summary, and only the last confirmed state can flush a legacy batch. +async function legacyBatchFixture(pending = 1) { + const f = await fixture(), engine = await decisionEngine(); + f.review.url = engine.url; + const refreshes: string[] = []; + f.manuscripts.refresh = async id => { refreshes.push(id); }; + const summary = (pending: number) => f.store.updateTask('thesis-task', { deferred: false, review: { state: pending ? 'reviewing' : 'done', round: 1, counts: { pending, accepted: 1 }, unsent: 0 } }); + summary(pending); + const base = '/review/thesis/api/tasks/p-thesis-task'; + const post = (route = 'decide', action = 'accept') => f.local.inject({ method: 'POST', url: `${base}/${route}`, headers: { ...LOCAL, 'x-review-token': 'review-token' }, payload: { id: crypto.randomUUID(), action, round: 1, changes: ['c001'], body: 'Shorter.' } }); + const poll = () => f.local.inject({ url: `${base}/state`, headers: LOCAL }); + return { ...f, engine, refreshes, summary, post, poll, async close() { await engine.close(); await f.close(); } }; +} +async function eventually(check: () => boolean) { + const end = Date.now() + 2000; + while (!check()) { assert.ok(Date.now() < end, 'the gated operation advances'); await new Promise(resolve => setImmediate(resolve)); } +} + +test('legacy final accept, reject and comment reply before their confirmed summaries flush the manuscript', { timeout: 10000 }, async () => { + for (const action of ['accept', 'reject', 'comment']) { + const f = await legacyBatchFixture(); + let release = () => {}, read = false; + f.workspace.refresh = async () => { + read = true; + await new Promise(resolve => { release = resolve; }); + f.summary(0); return true; + }; + try { + const response = await f.post('decide', action); + assert.equal(response.statusCode, 200, response.body); assert.equal(read, true); + assert.deepEqual(f.refreshes, [], `${action} has not confirmed the final counts`); + let polled = false; + const poll = f.poll().then(response => { polled = true; return response; }); + assert.equal((await f.local.inject({ url: '/api/state', headers: LOCAL })).statusCode, 200); + assert.equal(polled, false, 'only the review poll waits for its summary'); + release(); assert.equal((await poll).statusCode, 200); + assert.deepEqual(f.refreshes, ['thesis']); + } finally { release(); await f.close(); } + } +}); + +test('two rapid legacy decisions share one final manuscript refresh', { timeout: 10000 }, async () => { + const f = await legacyBatchFixture(2), releases: (() => void)[] = []; + f.workspace.refresh = async () => { + await new Promise(resolve => { releases.push(resolve); }); + f.summary(0); return true; + }; + try { + assert.equal((await f.post()).statusCode, 200); + assert.equal((await f.post('decide', 'reject')).statusCode, 200); + assert.equal(releases.length, 1); + releases[0](); await eventually(() => releases.length === 2); + assert.deepEqual(f.refreshes, [], 'the first read cannot flush the still-queued second decision'); + releases[1](); await f.poll(); assert.deepEqual(f.refreshes, ['thesis']); + } finally { releases.forEach(release => release()); await f.close(); } +}); + +test('a queued undo of an already completed legacy batch leaves pending changes without refreshing', { timeout: 10000 }, async () => { + const f = await legacyBatchFixture(0), releases: (() => void)[] = []; + f.workspace.refresh = async () => { + const index = releases.length; + await new Promise(resolve => { releases.push(resolve); }); + f.summary(index === 0 ? 0 : 1); return true; + }; + try { + assert.equal((await f.post()).statusCode, 200); assert.equal((await f.post('undo')).statusCode, 200); + assert.deepEqual(f.refreshes, [], 'cached completed counts do not confirm either request'); + releases[0](); await eventually(() => releases.length === 2); assert.deepEqual(f.refreshes, []); + releases[1](); await f.poll(); + assert.equal(f.store.task('thesis-task').review?.counts.pending, 1); assert.deepEqual(f.refreshes, []); + } finally { releases.forEach(release => release()); await f.close(); } +}); + +test('a failed final summary read cannot reuse an earlier complete legacy summary', { timeout: 10000 }, async () => { + const f = await legacyBatchFixture(0); + let release = () => {}, reads = 0; + f.review.get = async (route: string) => { + if (route.includes('/feedback')) return { feedback: [] } as any; + const number = ++reads; + if (number === 1) await new Promise(resolve => { release = resolve; }); + if (number === 2) throw new Error('Summary unavailable'); + return { state: 'done', round: 1, counts: { pending: 0, accepted: 1 } } as any; + }; + try { + assert.equal((await f.post()).statusCode, 200); assert.equal((await f.post('undo')).statusCode, 200); + release(); await f.poll(); assert.equal(reads, 2); + assert.deepEqual(f.refreshes, [], 'the last read failed despite cached pending=0'); + assert.equal((await f.post()).statusCode, 200); await f.poll(); assert.equal(reads, 3); + assert.deepEqual(f.refreshes, ['thesis'], 'a later successful read can finish a new batch'); + } finally { release(); await f.close(); } +}); + +test('summary throws and rejections release settlement so the next decision can confirm', { timeout: 10000 }, async () => { + for (const kind of ['throw', 'reject']) { + const f = await legacyBatchFixture(0); + let calls = 0; + f.workspace.refresh = () => { + if (++calls === 1) { + if (kind === 'throw') throw new Error('Synchronous summary failure'); + return Promise.reject(new Error('Asynchronous summary failure')); + } + f.summary(0); return Promise.resolve(true); + }; + try { + assert.equal((await f.post('undo')).statusCode, 200); await f.poll(); assert.deepEqual(f.refreshes, []); + assert.equal((await f.post()).statusCode, 200); await f.poll(); + assert.equal(calls, 2, `${kind} leaves no stale settlement entry or automatic retry`); + assert.deepEqual(f.refreshes, ['thesis']); + } finally { await f.close(); } + } +}); + +test('a task deleted during summary settlement cannot refresh its manuscript', { timeout: 10000 }, async () => { + const f = await legacyBatchFixture(0); + let release = () => {}, completed = false; + f.workspace.refresh = async () => { + await new Promise(resolve => { release = resolve; }); + completed = true; return true; + }; + try { + assert.equal((await f.post()).statusCode, 200); + f.store.deleteTask('thesis-task'); release(); await eventually(() => completed); + await new Promise(resolve => setImmediate(resolve)); + assert.deepEqual(f.refreshes, []); assert.ok(f.store.findTask('grant-task')); + } finally { release(); await f.close(); } +}); + +test('reconciliation retains its replay identity until its resulting summary is confirmed', { timeout: 10000 }, async () => { + const f = await legacyBatchFixture(0); + let unavailable = true, pending = 1; + f.review.get = async (route: string) => { + if (unavailable) throw new Error('Summary unavailable'); + return (route.includes('/feedback') ? { feedback: [] } : { state: pending ? 'reviewing' : 'done', round: 1, counts: { pending, accepted: 1 } }) as any; + }; + const replay = { route: '/api/tasks/p-thesis-task/undo', body: { id: 'undo-1' }, previousStatus: 'reviewing' as const }; + const check = () => f.local.inject({ method: 'POST', url: '/api/tasks/thesis-task/reconcile', headers: f.as, payload: {} }); + try { + f.store.updateTask('thesis-task', { status: 'uncertain', pendingReview: replay }); + const first = await check(); assert.equal(first.statusCode, 200, first.body); + assert.deepEqual(first.json().pendingReview, replay); assert.equal(first.json().status, 'uncertain'); assert.deepEqual(f.refreshes, []); + unavailable = false; + const second = await check(); assert.equal(second.statusCode, 200, second.body); + assert.equal(second.json().pendingReview, null); assert.equal(second.json().review.counts.pending, 1); + assert.deepEqual(f.refreshes, [], 'the confirmed undo reopens the batch'); + pending = 0; + f.store.updateTask('thesis-task', { status: 'uncertain', pendingReview: { ...replay, route: '/api/tasks/p-thesis-task/decide', body: { id: 'accept-2', action: 'accept' } } }); + assert.equal((await check()).statusCode, 200); assert.deepEqual(f.refreshes, ['thesis']); + assert.deepEqual(f.engine.calls.map(call => call.body.id), ['undo-1', 'undo-1', 'accept-2']); + } finally { await f.close(); } +}); + +test('a completed alternative build does not announce a legacy batch after an unconfirmed summary', { timeout: 10000 }, async () => { + for (const confirmed of [false, true]) { + const f = await legacyBatchFixture(0); + let summaries = 0; + f.review.get = async (route: string) => { + if (route.startsWith('/api/jobs/')) return { status: 'done', round: 1 } as any; + if (route.includes('/feedback')) return { feedback: [] } as any; + if (++summaries > 1 && !confirmed) throw new Error('Summary unavailable'); + return { state: 'done', round: 1, counts: { pending: 0, accepted: 1 } } as any; + }; + try { + f.store.updateTask('thesis-task', { status: 'building', pendingBuild: { requestId: 'saved-option', job: 'option-job', runs: [], sendBack: false, restarts: 0 } }); + assert.equal(await f.workspace.resumeBuild('thesis-task'), true); + assert.equal(f.store.task('thesis-task').status, 'reviewing', 'a temporarily missing summary does not fail a completed build'); + assert.deepEqual(f.refreshes, confirmed ? ['thesis'] : []); + } finally { await f.close(); } + } +}); + +test('recovering a final legacy decision waits for its pending alternative build before refreshing', { timeout: 10000 }, async () => { + const f = await legacyBatchFixture(0); + let finish = (_job: object) => {}; + const job = new Promise(resolve => { finish = resolve; }); + f.review.get = async (route: string) => { + if (route.startsWith('/api/jobs/')) return await job as any; + return (route.includes('/feedback') ? { feedback: [] } : { state: 'done', round: 1, counts: { pending: 0, accepted: 1 } }) as any; + }; + try { + f.store.updateTask('thesis-task', { status: 'uncertain', + pendingBuild: { requestId: 'saved-option', job: 'option-job', runs: [], sendBack: false, restarts: 0 }, + pendingReview: { route: '/api/tasks/p-thesis-task/decide', body: { id: 'final-accept', action: 'accept' }, previousStatus: 'building' } }); + const response = await f.local.inject({ method: 'POST', url: '/api/tasks/thesis-task/reconcile', headers: f.as, payload: {} }); + assert.equal(response.statusCode, 200, response.body); + assert.equal(response.json().pendingReview, null); assert.equal(response.json().pendingBuild.job, 'option-job'); + assert.deepEqual(f.refreshes, [], 'the recovered decision must not flush a result still being built'); + finish({ status: 'done', round: 1 }); await eventually(() => f.refreshes.length === 1); + assert.equal(f.store.task('thesis-task').pendingBuild, null); + assert.deepEqual(f.refreshes, ['thesis']); + } finally { finish({ status: 'done', round: 1 }); await eventually(() => !f.store.task('thesis-task').pendingBuild); await f.close(); } +}); + +test('a confirmed legacy replay refreshes without clearing an earlier build error', { timeout: 10000 }, async () => { + const f = await legacyBatchFixture(0); + f.review.get = async (route: string) => (route.includes('/feedback') ? { feedback: [] } : { state: 'done', round: 1, counts: { pending: 0, accepted: 1 } }) as any; + try { + f.store.updateTask('thesis-task', { status: 'uncertain', error: 'Check the action.', + pendingReview: { route: '/api/tasks/p-thesis-task/decide', body: { id: 'final-accept', action: 'accept' }, previousStatus: 'failed', previousError: 'PDF build failed.' } }); + const response = await f.local.inject({ method: 'POST', url: '/api/tasks/thesis-task/reconcile', headers: f.as, payload: {} }); + assert.equal(response.statusCode, 200, response.body); + assert.equal(response.json().pendingReview, null); + assert.equal(response.json().status, 'failed'); assert.equal(response.json().error, 'PDF build failed.'); + assert.deepEqual(f.refreshes, ['thesis'], 'the old error does not negate the confirmed final decision'); + } finally { await f.close(); } +}); + +test('a legacy build completion and a pending decision summary flush their shared result once', { timeout: 10000 }, async () => { + const f = await legacyBatchFixture(1); + let finish = (_job: object) => {}, release = () => {}, reads = 0; + const job = new Promise(resolve => { finish = resolve; }); + f.review.get = async (route: string) => { + if (route.startsWith('/api/jobs/')) return await job as any; + if (route.includes('/feedback')) return { feedback: [] } as any; + if (++reads === 1) await new Promise(resolve => { release = resolve; }); + return { state: 'done', round: 1, counts: { pending: 0, accepted: 1 } } as any; + }; + let building = Promise.resolve(false); + try { + f.store.updateTask('thesis-task', { status: 'building', pendingBuild: { requestId: 'saved-option', job: 'option-job', runs: [], sendBack: false, restarts: 0 } }); + building = f.workspace.resumeBuild('thesis-task'); + assert.equal((await f.post()).statusCode, 200); + assert.equal(reads, 1); + finish({ status: 'done', round: 1 }); await eventually(() => !f.store.task('thesis-task').pendingBuild); + release(); await building; await f.poll(); + assert.deepEqual(f.refreshes, ['thesis']); + } finally { finish({ status: 'done', round: 1 }); release(); await building; await f.close(); } +}); + +test('a review decision replies before the task summary is read again, and the next poll waits for that read', { timeout: 10000 }, async () => { const f = await fixture(), engine = await decisionEngine(); f.review.url = engine.url; const refreshes: { done: boolean; release: () => void }[] = [], reviewed: string[] = []; @@ -1086,6 +1315,7 @@ test('a review decision replies before the task summary is read again, and the n refreshes.push(entry); await new Promise(resolve => { entry.release = resolve; }); entry.done = true; + return true; }; f.workspace.on('reviewed', (id: string) => reviewed.push(id)); const headers = { ...LOCAL, 'x-review-token': 'review-token' }, base = '/review/thesis/api/tasks/p-thesis-task'; @@ -1118,8 +1348,10 @@ test('a review decision replies before the task summary is read again, and the n assert.equal((await poll).statusCode, 200); assert.deepEqual(reviewed, [], 'decisions refresh summaries without triggering speculative builds'); assert.deepEqual(engine.calls.map(call => call.method + ' ' + call.route), ['POST /api/tasks/p-thesis-task/decide', 'POST /api/tasks/p-thesis-task/decide', 'GET /api/tasks/p-thesis-task/state']); - // A poll with nothing to wait for answers at once. - assert.equal(await settled(f.local.inject({ url: base + '/state', headers: LOCAL })), true); + // A poll with nothing to settle reaches the engine without another read. + // Socket I/O need not finish within a fixed number of setImmediate turns. + assert.equal((await f.local.inject({ url: base + '/state', headers: LOCAL })).statusCode, 200); + assert.equal(refreshes.length, 2); } finally { for (const entry of refreshes) entry.release(); await engine.close(); await f.close(); } }); @@ -1128,7 +1360,7 @@ test('accepting and rejecting go ahead while the agent edits a deferred task; un const f = await fixture(), engine = await decisionEngine(route => route.endsWith('/undo') ? { restored: ['c001'] } : {}); f.review.url = engine.url; f.review.get = async (route: string) => (route.endsWith('/state') ? { undo } : {}) as any; - f.workspace.refresh = async () => {}; + f.workspace.refresh = async () => true; const headers = { ...LOCAL, 'x-review-token': 'review-token' }, base = '/review/thesis/api/tasks/p-thesis-task'; const post = (route: string, payload: Record) => f.local.inject({ method: 'POST', url: base + route, headers, payload }); try { @@ -1172,12 +1404,74 @@ test('accepting and rejecting go ahead while the agent edits a deferred task; un } finally { await engine.close(); await f.close(); } }); -test('writing a paper compiles its manuscript, while pending chapter proposals do not precompile the whole paper', async () => { +test('save previews and status checks do not compile an unapplied manuscript', async () => { + const f = await fixture(), engine = await decisionEngine(); + f.review.url = engine.url; + let refreshes = 0; + f.manuscripts.refresh = async () => { refreshes++; }; + const headers = { ...LOCAL, 'x-papilot-token': 'local-csrf' }; + try { + for (const result of [ + { commitReview: { token: 'confirm', diff: 'Review these changes first.' } }, + { reconciliation: { id: 'merge', ready: true } }, + { delivery: { mode: 'workspace', pending: true } }, + { delivery: { mode: 'branch' } }, + ]) { + f.workspace.applyTask = async () => result as any; + f.workspace.commitTask = async () => result as any; + for (const route of ['apply', 'commit']) { + const response = await f.local.inject({ method: 'POST', url: `/api/tasks/thesis-task/${route}`, headers, payload: { mode: 'commit', message: 'Save changes' } }); + assert.equal(response.statusCode, 200, response.body); + assert.equal(refreshes, 0, `${route} must wait for confirmed manuscript delivery`); + } + } + // A successful status check can merely confirm a decision or agent run. + f.store.updateTask('thesis-task', { deferred: true, review: { state: 'done', round: 1, counts: { pending: 0 }, unsent: 0 } }); + f.workspace.reconcile = async () => f.store.task('thesis-task'); + const checked = await f.local.inject({ method: 'POST', url: '/api/tasks/thesis-task/reconcile', headers, payload: {} }); + assert.equal(checked.statusCode, 200, checked.body); + assert.equal(refreshes, 0); + // Exercise the real replay adapter: only it knows whether the recovered + // action succeeded, independently of a previous build error. + f.workspace.reconcile = Workspace.prototype.reconcile.bind(f.workspace); + for (const [deferred, pending, state, expected] of [[true, 0, 'revise', 0], [false, 1, 'reviewing', 0], [false, 0, 'done', 1], [false, 0, 'revise', 2]] as const) { + f.store.updateTask('thesis-task', { deferred, status: 'uncertain', pendingReview: { route: '/api/tasks/p-thesis-task/decide', body: { id: 'lost', action: 'accept' }, previousStatus: 'reviewing' } }); + f.review.get = async (route: string) => (route.includes('/feedback') ? { feedback: [] } : { state, round: 1, counts: { pending } }) as any; + const recovered = await f.local.inject({ method: 'POST', url: '/api/tasks/thesis-task/reconcile', headers, payload: {} }); + assert.equal(recovered.statusCode, 200, recovered.body); + assert.equal(refreshes, expected); + } + } finally { await engine.close(); await f.close(); } +}); + +test('deferred commits refresh only after a completed manuscript delivery, even without a commit hash', async () => { const f = await fixture(); + let refreshes = 0; + f.manuscripts.refresh = async () => { refreshes++; }; + f.store.updateTask('thesis-task', { deferred: true }); try { - const refreshed: string[][] = [], predicted: [string, string, Record][] = []; + // Use the real commitTask adapter: current-branch engine deliveries can + // omit the commit hash, while branch-only deliveries can include one. + for (const [result, expected] of [ + [{ commitReview: { token: 'confirm', diff: 'Confirm earlier edits.' } }, 0], + [{ reconciliation: { id: 'merge', ready: true } }, 0], + [{ delivery: { mode: 'commit', accepted: 'accepted', pending: true } }, 0], + [{ delivery: { mode: 'branch', accepted: 'accepted', commit: 'branch-commit' } }, 0], + [{ delivery: { mode: 'commit', accepted: 'accepted' } }, 1], + ] as const) { + f.workspace.applyTask = async () => result as any; + const response = await f.local.inject({ method: 'POST', url: '/api/tasks/thesis-task/commit', headers: { ...LOCAL, 'x-papilot-token': 'local-csrf' }, payload: { message: 'Save reviewed changes' } }); + assert.equal(response.statusCode, 200, response.body); + assert.equal(refreshes, expected, JSON.stringify(result)); + } + } finally { await f.close(); } +}); + +test('intermediate decisions avoid manuscript refreshes and completed legacy batches refresh once', async () => { + const f = await fixture(); + try { + const refreshed: string[][] = []; f.manuscripts.refresh = async (id: string, root: string) => { refreshed.push([id, root]); }; - f.manuscripts.predict = async (id: string, root: string, files: Record) => { predicted.push([id, root, Object.fromEntries(Object.entries(files).map(([file, data]) => [file, data?.toString() ?? null]))]); }; f.workspace.commitTask = async () => ({ commit: 'abc', git: { branch: 'main', upstream: null, ahead: 0, behind: 0, files: [], head: null } }) as any; const headers = { ...LOCAL, 'x-papilot-token': 'local-csrf' }; const committed = await f.local.inject({ method: 'POST', url: '/api/tasks/thesis-task/commit', headers, payload: { message: 'Apply the task' } }); @@ -1188,24 +1482,61 @@ test('writing a paper compiles its manuscript, while pending chapter proposals d assert.equal(failed.statusCode, 400); await new Promise(resolve => setImmediate(resolve)); assert.equal(refreshed.length, 1, 'a request that changed nothing compiles nothing'); - // A review action through the review page, which may write the manuscript. - const engine = http.createServer((_request, response) => { response.setHeader('Content-Type', 'application/json'); response.end('{}'); }); + // Exercise the real review proxy, including its response hook, rather + // than just emitting the event that previously started speculative builds. + let pending = 2, commented = false; + f.store.updateTask('thesis-task', { deferred: true }); + f.workspace.refresh = async () => { + f.store.updateTask('thesis-task', { review: { state: pending ? 'reviewing' : commented ? 'revise' : 'done', round: 1, counts: { pending }, unsent: 0 } }); + return true; + }; + const engine = http.createServer((_request, response) => { + response.setHeader('Content-Type', 'application/json'); + response.end(JSON.stringify({ task: 'p-thesis-task', csrf: 'review-token', closed: false, decisions: {}, job: null })); + }); await new Promise(resolve => engine.listen(0, '127.0.0.1', resolve)); f.review.url = `http://127.0.0.1:${(engine.address() as { port: number }).port}`; try { - const noted = await f.local.inject({ method: 'POST', url: '/review/thesis/api/tasks/p-thesis-task/note', headers: { ...LOCAL, 'x-review-token': 'review-token' }, payload: { body: 'A note.' } }); - assert.equal(noted.statusCode, 200, noted.body); + const review = async (route: string, action?: string) => { + const response = await f.local.inject({ method: 'POST', url: `/review/thesis/api/tasks/p-thesis-task/${route}`, headers: { ...LOCAL, 'x-review-token': 'review-token' }, payload: { id: crypto.randomUUID(), action, body: 'A note.' } }); + assert.equal(response.statusCode, 200, response.body); + await new Promise(resolve => setImmediate(resolve)); + }; + for (const [route, action, remaining] of [['decide', 'accept', 1], ['decide', 'reject', 0], ['undo', undefined, 1], ['decide', 'accept', 0], ['note', undefined, 0]] as const) { + pending = remaining; + await review(route, action); + assert.equal(refreshed.length, 1, `${route} does not refresh the manuscript`); + } + f.workspace.applyTask = async () => ({ delivery: { mode: 'workspace', accepted: 'accepted' } }) as any; + const applied = await f.local.inject({ method: 'POST', url: '/api/tasks/thesis-task/apply', headers, payload: { mode: 'workspace' } }); + assert.equal(applied.statusCode, 200, applied.body); await new Promise(resolve => setImmediate(resolve)); - assert.equal(refreshed.length, 1, 'a note changes no manuscript files and starts no build'); - } finally { engine.close(); } - assert.deepEqual(predicted, [], 'review actions start no speculative manuscript build'); + assert.equal(refreshed.length, 2, 'application starts one manuscript refresh'); + + // Legacy tasks write to the manuscript while reviewing. They refresh + // only once the selection is complete, and never for a note or undo. + f.store.updateTask('thesis-task', { deferred: false }); + pending = 1; await review('decide', 'accept'); + assert.equal(refreshed.length, 2); + pending = 0; await review('decide', 'reject'); + assert.equal(refreshed.length, 3); + pending = 1; await review('undo'); + await review('note'); + assert.equal(refreshed.length, 3); + // A final comment resolves the last pending choice without writing a + // file, but must flush changes accepted earlier in this legacy batch. + await review('decide', 'accept'); + assert.equal(refreshed.length, 3); + pending = 0; commented = true; await review('decide', 'comment'); + assert.equal(refreshed.length, 4); + } finally { engine.closeAllConnections(); await new Promise(resolve => engine.close(() => resolve())); } } finally { await f.close(); } }); test('deferred decisions wait until application before refreshing the manuscript', async () => { const f = await fixture(), engine = await decisionEngine(() => ({})); f.review.url = engine.url; - f.workspace.refresh = async () => {}; + f.workspace.refresh = async () => true; const refreshed: string[] = [], predicted: string[] = []; f.manuscripts.refresh = async id => { refreshed.push(id); }; f.manuscripts.predict = async id => { predicted.push(id); }; @@ -1221,7 +1552,7 @@ test('deferred decisions wait until application before refreshing the manuscript await new Promise(resolve => setImmediate(resolve)); assert.deepEqual(refreshed, []); assert.deepEqual(predicted, []); - f.store.updateTask('thesis-task', { deferred: false }); + f.store.updateTask('thesis-task', { deferred: false, review: { state: 'done', round: 1, counts: { pending: 0, accepted: 1 }, unsent: 0 } }); const legacy = await f.local.inject({ method: 'POST', url: '/review/thesis/api/tasks/p-thesis-task/decide', headers, payload: { id: 'legacy', round: 1, action: 'accept', changes: ['c001'] } }); assert.equal(legacy.statusCode, 200, legacy.body); await new Promise(resolve => setImmediate(resolve)); diff --git a/tests/note-option-reject.test.ts b/tests/note-option-reject.test.ts index d9171404..b952c414 100644 --- a/tests/note-option-reject.test.ts +++ b/tests/note-option-reject.test.ts @@ -13,6 +13,70 @@ import type { Codex } from '../src/server/codex.ts'; import type { Task } from '../src/shared.ts'; class Agent extends EventEmitter { ready = false; close() {} } +for (const deferred of [false, true]) test(`${deferred ? 'deferred' : 'legacy'} note decisions respect the final manuscript batch boundary`, async () => { + const f = new Fixture(), engine = f.app(); await engine.ready; + const task = engine.tasks.create('note-batch', 'Note batch', [], ['paper'], deferred); + f.edit(task, METHOD, 'Method sentence 2.', 'Approach sentence 2.'); + f.edit(task, METHOD, 'Detail sentence 15.', 'Detail proposal 15.'); + const round = fakeRound(engine, task.name); + const note = round.units.find(unit => unit.new_text === 'Approach')!, earlier = round.units.find(unit => unit.new_text === 'proposal')!; + assert.equal(round.units.length, 2); + const detail = { ...round, round: 1, units: round.units.map(unit => { + const lines = engine.repo.blob(round.before, unit.file)!.toString().split('\n'); + return { ...unit, old_context: { text: lines[unit.old_start - 1], start: lines.slice(0, unit.old_start - 1).map(line => line + '\n').join('').length } }; + }), sections: [], loose: round.units.map(unit => ({ kind: 'change', units: [unit.id] })) }; + const server = createServer(engine); await server.listen({ host: '127.0.0.1', port: 0 }); + const transport = new Client(`http://127.0.0.1:${(server.server.address() as { port: number }).port}`, engine.csrf); + engine.url = transport.url; + let releaseBuild!: () => void, polledBuild!: () => void; + const buildGate = new Promise<{ status: string; round: number }>(resolve => { releaseBuild = () => resolve({ status: 'done', round: 1 }); }); + const buildStarted = new Promise(resolve => { polledBuild = resolve; }); + const client = { url: transport.url, token: transport.token, get: (route: string) => { + if (route === '/api/jobs/option-build') { polledBuild(); return buildGate as Promise; } + return route.includes('/rounds/') ? Promise.resolve(detail as T) : transport.get(route); + }, post: (route: string, body?: unknown) => transport.post(route, body) }; + const store = new Store(':memory:'), workspace = new Workspace(store, new Agent() as unknown as Codex, async () => client); + const manuscripts = new Manuscripts(); let refreshes = 0; + manuscripts.refresh = async () => { refreshes++; }; + store.saveProject({ id: 'paper', name: 'Paper', path: f.root, createdAt: 1 }); + store.saveTask({ id: 'task', projectId: 'paper', name: task.name, title: task.title, worktree: task.worktree, instructions: '', deferred, status: 'reviewing', createdAt: 1, updatedAt: 1, feedbackCursor: 0, review: { round: 1 }, error: null } as Task); + store.saveRun({ id: 'run', taskId: 'task', requestKey: 'run', prompt: '', status: 'completed', createdAt: 1, finishedAt: 2, reviewRound: 1 }); + store.saveItem({ id: 'run', taskId: 'task', type: 'user', text: '', createdAt: 1, meta: { notes: [{ where: `${METHOD} line ${note.old_start}`, quote: 'Method', comment: 'Improve wording.' }] } }); + const local = await buildApp({ listener: { kind: 'local', port: 7274, publicOrigin: '', owner: '' }, store, workspace, manuscripts, csrf: 'test-csrf', serialGit: gitQueue(), dist: null, version: 'test', connectReview: async () => client }); + const headers = { host: '127.0.0.1:7274', origin: 'http://127.0.0.1:7274', 'x-papilot-token': 'test-csrf', 'x-review-token': engine.csrf }; + const proxy = (route: string, payload: Record) => local.inject({ method: 'POST', url: `/review/paper/api/tasks/${task.name}/${route}`, headers, payload }); + const choose = (payload: Record) => local.inject({ method: 'POST', url: '/api/tasks/task/note-options', headers, payload: { key: 'run:0', round: 1, option: 'current', ...payload } }); + try { + const first = await proxy('decide', { id: 'earlier', round: 1, changes: [earlier.id], action: 'accept' }); + assert.equal(first.statusCode, 200, first.body); + assert.equal(refreshes, 0, 'earlier proxy acceptance waits for the remaining note'); + const selection = await choose({}); + assert.equal(selection.statusCode, 200, selection.body); + assert.equal((await choose({ id: 'invalid', round: 2, accept: true })).statusCode, 400); + assert.equal(refreshes, 0, 'a selection or failed decision is not a batch boundary'); + for (const [index, action] of ['accept', 'reject'].entries()) { + const response = await choose({ id: `final-${action}`, [action]: true }); + assert.equal(response.statusCode, 200, response.body); + assert.equal(store.task('task').review?.counts.pending, 0); + assert.equal(refreshes, deferred ? 0 : index + 1, 'the confirmed final drawer decision flushes only a legacy manuscript'); + const invalid = await choose({ id: `new-${action}`, [action]: true }); + assert.equal(invalid.statusCode, 400, invalid.body); + assert.equal(refreshes, deferred ? 0 : index + 1, 'a failed duplicate cannot trigger compilation'); + const undo = await proxy('undo', { id: `final-${action}` }); + assert.equal(undo.statusCode, 200, undo.body); + assert.equal(refreshes, deferred ? 0 : index + 1, 'undo reopens a pending batch'); + } + // An accepted alternative finishes its decision in the engine build job, + // after the drawer's HTTP response. Resuming that job must flush as well. + store.updateTask('task', { status: 'building', pendingBuild: { requestId: 'option-save', job: 'option-build', runs: [], sendBack: false, restarts: 0 } }); + const building = workspace.resumeBuild('task'); await buildStarted; + assert.equal(refreshes, deferred ? 0 : 2, 'an unfinished option build does not flush'); + await engine.review.decide(task.name, { id: 'built-option', round: 1, changes: [note.id], action: 'accept' }); + releaseBuild(); await building; + assert.equal(store.task('task').status, 'reviewing'); + assert.equal(refreshes, deferred ? 0 : 3, 'a confirmed option build flushes the final legacy batch'); + } finally { releaseBuild(); await local.close(); workspace.close(); store.close(); await server.close(); await f.close(); } +}); test('rejecting a note restores its source even when previewing an alternative; neighboring edits and undo survive', async () => { const f = new Fixture(), engine = f.app(); await engine.ready; const task = engine.tasks.create('reject-note', 'Reject a note', [], ['paper'], true); @@ -31,8 +95,13 @@ test('rejecting a note restores its source even when previewing an alternative; return { ...unit, old_context: { text: lines[unit.old_start - 1], start } }; }), sections: [], loose: round.units.map(unit => ({ kind: 'change', units: [unit.id] })) }; const previews: unknown[] = []; + let previewFailure: Error | null = null, failOnce = false, connections = 0; const client = { url: transport.url, token: transport.token, get: async (route: string) => route.includes('/rounds/') ? { ...detail, round: 1 } as T : transport.get(route), post: async (route: string, body?: unknown) => { - if (route.endsWith('/source/options/preview')) { previews.push(body); return { ...detail, round: 1, after: 'respects-rejection' } as T; } + if (route.endsWith('/source/options/preview')) { + previews.push(body); + if (previewFailure) { const error = previewFailure; if (failOnce) previewFailure = null; throw error; } + return { ...detail, round: 1, after: 'respects-rejection' } as T; + } return transport.post(route, body); } }; const store = new Store(':memory:'); @@ -45,7 +114,7 @@ test('rejecting a note restores its source even when previewing an alternative; { id: 'current', label: 'Main', edits: [{ unit: word.id, text: word.new_text }] }, { id: 'alternative', label: 'Alternative', reason: 'Another wording.', edits: [{ unit: word.id, text: 'Procedure' }] }, ] }); - const local = await buildApp({ listener: { kind: 'local', port: 7274, publicOrigin: '', owner: '' }, store, workspace, manuscripts: new Manuscripts(), csrf: 'test-csrf', serialGit: gitQueue(), dist: null, version: 'test', connectReview: async () => client }); + const local = await buildApp({ listener: { kind: 'local', port: 7274, publicOrigin: '', owner: '' }, store, workspace, manuscripts: new Manuscripts(), csrf: 'test-csrf', serialGit: gitQueue(), dist: null, version: 'test', connectReview: async () => { connections++; return client; } }); const headers = { host: '127.0.0.1:7274', origin: 'http://127.0.0.1:7274', 'x-papilot-token': 'test-csrf' }; try { const options = async () => (await local.inject({ method: 'GET', url: '/api/tasks/task/note-options?key=run:0', headers })).json(); @@ -60,6 +129,18 @@ test('rejecting a note restores its source even when previewing an alternative; assert.equal(currentPreview.json().after, 'respects-rejection', 'Current proposal also uses a decision-aware preview'); assert.equal(previews.length, 1); assert.deepEqual((previews[0] as { rejected: string[] }).rejected, [neighbor.id], 'The engine must validate the same rejection state as the browser comparison key'); + for (const [message, transient, expectedStatus] of [['fetch failed', true, 200], ['fetch failed', false, 400], ['Invalid optional LaTeX', false, 400]] as const) { + previewFailure = new Error(message); failOnce = transient; + const beforeConnections: number = connections, beforePreviews: number = previews.length; + const response = await local.inject({ method: 'GET', url: url + rejectedKey, headers }); + assert.equal(response.statusCode, expectedStatus, response.body); + const attempts = message === 'fetch failed' ? 2 : 1; + assert.equal(connections - beforeConnections, attempts, 'Only connection failures reconnect, at most once'); + assert.equal(previews.length - beforePreviews, attempts); + assert.equal(engine.review.decisions(task.name, round)[word.id], 'pending', 'Preview recovery never saves a decision'); + if (expectedStatus === 200) assert.equal(response.json().after, 'respects-rejection'); + } + previewFailure = null; await engine.review.undo(task.name, 'reject-other'); assert.equal((await options()).comparisonKey, initialKey, 'Undo restores the reusable preview identity'); for (const option of ['current', 'alternative']) {