diff --git a/engine/src/web/FullPaper.tsx b/engine/src/web/FullPaper.tsx index 8249c009..d5be7172 100644 --- a/engine/src/web/FullPaper.tsx +++ b/engine/src/web/FullPaper.tsx @@ -32,7 +32,7 @@ const CMapData = cMapData( // The whole paper with its changes in place: the new PDF's pages, opened up // where a change's new lines are, and that change shown there as in the -// changes view, old lines above new. Pages are painted only near the +// changes view, old lines beside new. Pages are painted only near the // viewport, so a long thesis scrolls like a short paper. interface Cut { top: number; @@ -273,13 +273,11 @@ function Pages({ useLayoutEffect(() => ready.current(), [paper]); const slice = (page: number, top: number, bottom: number) => bottom - top > 0.5 && ( -
-
+
+ {(["before", "after"] as const).map(side =>
+
+
)}
); return ( diff --git a/engine/src/web/review.tsx b/engine/src/web/review.tsx index 96926ad9..e50918c0 100644 --- a/engine/src/web/review.tsx +++ b/engine/src/web/review.tsx @@ -874,8 +874,10 @@ export function ReviewApp({ })}` : ""}

- {strips(place.before, "before")} - {strips(place.after, "after")} +
+
{strips(place.before, "before")}
+
{strips(place.after, "after")}
+
))} {total > places.length && ( @@ -1005,6 +1007,7 @@ export function ReviewApp({ {location} {actionBar(index)} + {reviewed && state?.undo?.changes.some(id => block.units.includes(id)) && } {kindLabel && {kindLabel}} {status !== "pending" && ( {stateLabel(status)} @@ -1044,11 +1047,10 @@ export function ReviewApp({ ))}
)} - {!block.identical && - !block.reflow && - block.before.length > 0 && - images(block.before, "before")} - {block.after.length > 0 && images(block.after, "after")} +
+ {images(block.before.length ? block.before : block.identical || block.reflow ? block.after : [], "before")} + {images(resolved === "before" ? block.before : block.after, "after")} +
{block.identical && !bib.length && (

{t("No visible change in the PDF; see the LaTeX source.")} @@ -1581,7 +1583,8 @@ export function ReviewApp({ onRetry={references.retry} /> )} -

+
+ {detail &&
{t("Before")}{t("After")}
} {detail && paperView ? ( <>
}> @@ -1615,12 +1618,10 @@ export function ReviewApp({
{section.blocks.map((block, b) => block.kind === "context" ? ( -
- +
+ {(["before", "after"] as const).map(side =>
+ +
)}
) : block.kind === "gap" ? (
*{min-width:0} +.comparison-head{position:sticky;top:0;z-index:5;padding:10px 0;margin-bottom:12px;background:var(--frame);font:600 13px/1.4 var(--sans)} +.comparison-head>span{padding:0 14px} +.comparison-head>span:first-child{color:var(--before-edge)} +.comparison-head>span:last-child{color:var(--after-edge)} +.comparison .sheet,.comparison .paper-page{background:linear-gradient(to right,var(--pdf-page,var(--paper)) calc(50% - var(--comparison-gap)/2),transparent 0,transparent calc(50% + var(--comparison-gap)/2),var(--pdf-page,var(--paper)) 0);box-shadow:none} +.comparison .change .side{display:block;align-self:stretch;border:0;padding:0;min-height:30px} +.comparison .side:empty{background:repeating-linear-gradient(135deg,transparent 0 8px,var(--line) 8px 9px);opacity:.4} +.comparison .side::after{display:none} +.comparison .context,.comparison .paper-context{background:var(--pdf-page,var(--paper))} +.comparison .pdf-strip{max-width:none} +.comparison .paper-slice{width:100%;margin-left:0} +.comparison .change-head{scroll-margin-top:45px} +.comparison .reviewed-change>.change-head{display:flex} +.comparison .mixed-reviewed .comparison-row{display:none} +.comparison .change.current{box-shadow:0 0 0 2px var(--accent);border-radius:4px} +.comparison .fold{margin:0;background:var(--frame)} +@media(max-width:680px){.comparison{--comparison-gap:12px}.comparison-head{position:static}} + +/* Resolved source editors stay in History; do not expose a toggle for a hidden editor. */ +.comparison .reviewed-change>.change-head>.link:not(.undo):not(.copy-reference){display:none} diff --git a/scripts/check-review-comparison.mjs b/scripts/check-review-comparison.mjs new file mode 100644 index 00000000..17018dbc --- /dev/null +++ b/scripts/check-review-comparison.mjs @@ -0,0 +1,162 @@ +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 { annotateReviewState, rewriteReviewHtml } from '../src/server/review.ts'; + +fs.mkdirSync('test-results', { recursive: true }); +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 }); +const page = await browser.newPage({ locale: 'en-US', viewport: { width: 1440, height: 1000 } }); +const errors = []; page.on('pageerror', error => errors.push(error.message)); +const prefix = '/review/paper'; +const reviewDist = path.resolve(process.env.PAPILOT_REVIEW_DIST || 'engine/dist/web'); +const task = { id: 'review-flow-fixture', projectId: 'paper', name: 'flow', title: 'Review the new wording', status: 'reviewing', createdAt: 1, updatedAt: 1, review: { state: 'reviewing', round: 1, counts: {}, unsent: 0 } }; +const snapshot = { version: 'test', csrf: 'fixture', projects: [{ id: 'paper', name: 'Paper', path: '/tmp/paper' }], tasks: [task], requests: [], runtime: { connected: true, authenticated: true, models: [] } }; +let latest = 1, serial = 0, undo = null; +let effectFixture = false, effectRequests = 0; +const rounds = { 1: { first: 'pending', second: 'pending', third: 'pending', fourth: 'pending' } }; +const actions = [], writes = [], messageRequests = [], commitRequests = [], applyRequests = [], archiveRequests = [], deleteRequests = []; +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 }); +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])); + task.review.state = values.includes('pending') ? 'reviewing' : values.includes('commented') ? 'revise' : 'done'; +} +update(); +const image = (name, label) => ({ src: `${name}.svg`, clean: `${name}-clean.svg`, width: 600, height: 130, page: 0, label: '1', text: [{ chars: [{ text: label, rect: [60, 40, 500, 62] }] }] }); +const effectPlace = name => ({ page: '2', section: 'Contents', before: [image(`${name}-old`, 'Section 2')], after: [image(`${name}-new`, 'Section 3')] }); +function detail(round) { + const ids = Object.keys(rounds[round]); + const groups = round === 1 ? [['first'], ['second'], ['third', 'fourth']] : [['new']]; + const result = { task: 'flow', round, before: `before-${round}`, after: `after-${round}`, warnings: [], files: `${prefix}/files/flow/${round}/`, + sections: [{ node: 'intro', title: 'Introduction', number: '1', blocks: groups.map((units, i) => ({ kind: 'change', units, identical: false, before: [image(`before-${i}`, `Original paragraph ${i + 1}.`)], after: [image(`after-${i}`, `Revised paragraph ${i + 1}.`)] })) }], loose: [], + units: ids.map((id, i) => ({ id, file: 'main.tex', status: 'modified', kind: 'text', whole_file: false, ops: [], old_start: i + 1, old_end: i + 1, new_start: i + 1, new_end: i + 1, old_text: `Original paragraph ${i + 1}`, new_text: `Revised paragraph ${i + 1}`, truncated: false })) }; + if (effectFixture && round === 1) { + for (const [i, visual] of ['text', 'unmapped', 'identical'].entries()) Object.assign(result.units[i], { + visual, effects: { total: i === 0 ? 2 : 1, shown: [effectPlace(`effect-${i}`)] }, + }); + // A macro definition has no crop of its own; its uses are its preview. + Object.assign(result.units[1], { file: 'macros.tex', old_text: '\\newcommand{\\term}{Old}', new_text: '\\newcommand{\\term}{New}' }); + Object.assign(result.sections[0].blocks[1], { before: [], after: [] }); + } + result.pdf = prefix + '/full.pdf'; + result.sections[0].blocks[0].after[0].height = 220; + result.sections[0].blocks[1].before = []; + result.sections[0].blocks[2].after = []; + for (const block of result.sections[0].blocks) for (const strip of [...block.before, ...block.after]) Object.assign(strip, { page: 1, top: 200 }); + result.sections[0].blocks.unshift({ kind: 'context', image: image('context', 'Unchanged context.') }); + return result; +} +await page.route('**/api/**', async route => { + const url = new URL(route.request().url()); + if (url.pathname === '/api/updates') return route.fulfill({ json: { currentVersion: snapshot.version, latestVersion: null, phase: 'idle', checkedAt: 1, error: null, canUpdate: true } }); + if (url.pathname === '/api/state') return route.fulfill({ json: snapshot }); + if (url.pathname.endsWith('/skills')) return route.fulfill({ json: { skills: [] } }); + if (url.pathname === '/api/tasks/question-fixture/items') return route.fulfill({ json: { items: [{ id: 'answer', taskId: 'question-fixture', type: 'assistant', text: 'This equation expresses conservation of mass.', createdAt: 1 }] } }); + if (url.pathname.endsWith('/items')) return route.fulfill({ json: { items: timeline } }); + if (url.pathname.endsWith('/delete')) { + deleteRequests.push(route.request().postDataJSON()); + snapshot.tasks = snapshot.tasks.filter(item => item.id !== task.id); + return route.fulfill({ json: { ok: true } }); + } + if (url.pathname.endsWith('/archive')) { + const body = route.request().postDataJSON(); archiveRequests.push(body); + task.archivedAt = body.archived ? Date.now() : null; + return route.fulfill({ json: task }); + } + if (url.pathname.endsWith('/git')) return route.fulfill({ json: { branch: 'main', upstream: 'origin/main', remote: true, ahead: 0, behind: 0, head: null, files: gitFiles } }); + if (url.pathname.endsWith('/manuscript')) return route.fulfill({ json: { git: { branch: 'main', remote: true, ahead: 0, behind: 0, head: null, files: gitFiles }, build: { status: 'building', current: false }, annotations: [] } }); + if (url.pathname.endsWith('/git/message') || url.pathname.endsWith('/commit-message')) { messageRequests.push(route.request().postDataJSON()); return route.fulfill({ json: { message: 'Explain the method', agent: 'codex' } }); } + if (url.pathname.endsWith('/apply')) { + const body = route.request().postDataJSON(); + applyRequests.push(body); + if (conflictOnApply) { + conflictOnApply = false; task.status = 'queued'; + task.reconciliation = { id: 'merge-fixture', round: latest, files: ['main.tex'], ready: true, intent: { mode: body.mode, message: body.message || '' } }; + return route.fulfill({ json: { reconciliation: task.reconciliation } }); + } + if (combinedOnCommit && body.mode === 'commit' && body.confirmation !== 'combined-version') return route.fulfill({ json: { commitReview: { token: 'combined-version', files: ['main.tex'], diff: '-Original title\n+Final reviewed title' } } }); + combinedOnCommit = false; + task.review.files = []; task.reconciliation = null; + task.delivery = { mode: body.mode || 'workspace', accepted: 'accepted', ...(body.mode === 'commit' ? { branch: 'main', commit: 'abc1234' } : {}) }; + if (body.mode === 'commit') commitRequests.push(body); + return route.fulfill({ json: { delivery: task.delivery } }); + } + if (url.pathname.endsWith('/commit')) { + const body = route.request().postDataJSON(); + commitRequests.push(body); gitFiles = []; task.review.files = []; + if (task.deferred) task.delivery = { mode: 'branch', branch: 'papilot/flow', commit: 'abc1234', accepted: 'accepted' }; + else task.committed = { hash: 'abc1234', accepted: `accepted-${commitRequests.length}`, at: Date.now(), pushed: body.push && !pushError }; + return route.fulfill({ json: { commit: 'abc1234', pushError, git: { branch: 'main', upstream: 'origin/main', remote: true, ahead: 0, behind: 0, head: { hash: 'abc1234', subject: 'Explain the method', date: 2 }, files: [] } } }); + } + if (url.pathname.endsWith('/events')) return route.fulfill({ contentType: 'text/event-stream', body: ': fixture\n\n' }); + if (url.pathname.endsWith('/messages')) { task.status = 'running'; task.review.unsent = 0; return route.fulfill({ json: { ok: true } }); } + throw Error(`Unexpected app request ${url.pathname}`); +}); +await page.route('**/review/**', async route => { + const url = new URL(route.request().url()), relative = url.pathname.slice(prefix.length); + if (relative === '/') return route.fulfill({ contentType: 'text/html', body: rewriteReviewHtml(fs.readFileSync(path.join(reviewDist, 'index.html'), 'utf8'), prefix) }); + if (relative.startsWith('/assets/')) return route.fulfill({ path: path.join(reviewDist, relative), contentType: /\.m?js$/.test(relative) ? 'text/javascript' : relative.endsWith('.css') ? 'text/css' : 'font/otf' }); + if (relative === '/full.pdf') return route.fulfill({ contentType: 'application/pdf', body: fs.readFileSync('tests/fixtures/outline.pdf') }); + if (relative.startsWith('/files/')) return route.fulfill({ contentType: 'image/svg+xml', body: 'A paragraph in the review PDF.' }); + const round = Number(url.searchParams.get('round') || latest); + if (/\/rounds\/\d+$/.test(relative)) return route.fulfill({ json: detail(Number(relative.split('/').at(-1))) }); + if (relative.endsWith('/effects/first')) { effectRequests++; return route.fulfill({ json: { places: [effectPlace('extra')] } }); } + let restored; + if (route.request().method() === 'POST') { + const body = route.request().postDataJSON(); writes.push(body); + if (relative.endsWith('/decide')) { + actions.push({ decisions: structuredClone(rounds[latest]), undo }); + for (const id of body.changes) rounds[latest][id] = { accept: 'accepted', reject: 'rejected', comment: 'commented' }[body.action]; + undo = { id: `action-${++serial}`, kind: body.action, changes: body.changes }; + task.review.unsent++; + } else if (relative.endsWith('/undo')) { + restored = undo.changes; + const previous = actions.pop(); rounds[latest] = previous.decisions; undo = previous.undo; + } + update(); + } + return route.fulfill({ json: { ...annotateReviewState(state(round), [task]), ...(restored ? { restored } : {}) } }); +}); +const viewer = () => page.frameLocator('.review-frame'); +const sync = async () => page.evaluate(() => window.dispatchEvent(new Event('online'))); +try { + await page.goto(origin); + const frame = viewer(); + await frame.locator('.change.current').waitFor(); + async function aligned() { + const boxes = await frame.locator('.change .comparison-row').evaluateAll(rows => rows.map(row => [...row.children].map(el => { const b=el.getBoundingClientRect(); return {x:b.x,y:b.y,width:b.width,height:b.height}; }))); + assert.equal(boxes.length,3); + for (const [left,right] of boxes) { assert.ok(Math.abs(left.y-right.y)<1); assert.ok(Math.abs(left.width-right.width)<1); assert.ok(right.x>left.x+left.width); assert.ok(Math.abs(left.height-right.height)<1); } + } + await aligned(); + assert.equal(await frame.locator('.context-pair .context').count(),2); + await expect(frame.locator('.change[data-stop="1"] .before')).toBeEmpty(); + await expect(frame.locator('.change[data-stop="2"] .after')).toBeEmpty(); + await page.screenshot({path:'test-results/review-side-by-side.png',fullPage:true}); + await frame.locator('.change.current').getByRole('button',{name:'Accept',exact:true}).click(); + await expect.poll(()=>rounds[1].first).toBe('accepted'); + await frame.getByRole('button',{name:'Undo',exact:true}).first().click(); + await expect.poll(()=>rounds[1].first).toBe('pending'); + await frame.locator('.change.current').getByRole('button',{name:'Reject',exact:true}).click(); + await expect.poll(()=>Object.values(rounds[1]).includes('rejected')).toBe(true); + await frame.getByRole('button',{name:'Undo',exact:true}).first().click(); + await page.getByRole('group',{name:'Show',exact:true}).getByRole('button',{name:'PDF',exact:true}).click(); + await expect(frame.locator('.paper-page').first()).toBeVisible(); + await expect(frame.locator('.paper-context-pair canvas').first()).toBeVisible(); + await aligned(); + await page.screenshot({path:'test-results/review-side-by-side-paper.png',fullPage:true}); + await page.setViewportSize({width:1000,height:900}); + await aligned(); + assert.deepEqual(errors,[]); + console.log('PASS: paired context, unequal heights, insertions, deletions, accept/reject/undo, full-paper canvases and narrow desktop alignment.'); +} finally { await browser.close(); await server.close(); } diff --git a/scripts/check-review-flow.mjs b/scripts/check-review-flow.mjs index ce430962..fd48b07a 100644 --- a/scripts/check-review-flow.mjs +++ b/scripts/check-review-flow.mjs @@ -302,7 +302,7 @@ try { await viewer().locator('.change.current .change-number').click(); await page.keyboard.press('b'); await expect.poll(() => rounds[1].first).toBe('accepted'); - await expect(viewer().locator('[data-stop="0"] .change-head')).toBeHidden(); + await expect(viewer().locator('[data-stop="0"] .change-head')).toBeVisible(); await expect(viewer().locator('.change.current button.accept')).toBeEnabled(); await viewer().locator('.change.current .change-number').click(); await page.keyboard.press('ControlOrMeta+z'); @@ -351,14 +351,14 @@ try { const position = await viewer().locator('.change.current .dock').evaluate(el => getComputedStyle(el).position); if (width > 520) assert.notEqual(position, 'fixed'); else assert.equal(position, 'fixed'); - assert.equal(await viewer().locator('#paper').evaluate(el => el.scrollWidth > el.clientWidth + 1), false); + assert.equal(await viewer().locator('html').evaluate(el => el.scrollWidth > el.clientWidth + 1), false); } await page.setViewportSize({ width: 1440, height: 1000 }); await viewer().locator('.change.current').getByRole('button', { name: 'Accept', exact: true }).click(); const accepted = viewer().locator('[data-stop="0"]'); - await expect(accepted.locator('.change-head')).toBeHidden(); + await expect(accepted.locator('.change-head')).toBeVisible(); await expect(accepted.locator('.side.after')).toBeVisible(); - await expect(accepted.locator('.side.before')).toBeHidden(); + await expect(accepted.locator('.side.before')).toBeVisible(); await expect(viewer().locator('.change-map-mark')).toHaveCount(2); await expect(viewer().locator('.change.current')).toHaveAttribute('data-stop', '1'); // The accepted paragraph remains selectable for another note. @@ -368,12 +368,12 @@ try { await viewer().getByRole('button', { name: 'Add note', exact: true }).click(); await viewer().getByRole('button', { name: 'Close review notes' }).click(); await viewer().locator('.change.current').getByRole('button', { name: 'Reject', exact: true }).click(); - await expect(viewer().locator('[data-stop="1"] .change-head')).toBeHidden(); + await expect(viewer().locator('[data-stop="1"] .change-head')).toBeVisible(); await expect(viewer().locator('[data-stop="1"] .side.before')).toBeVisible(); await viewer().locator('.change.current').getByRole('button', { name: 'Comment', exact: true }).click(); await viewer().getByRole('textbox', { name: /^Comment on change/ }).fill('Try a clearer explanation.'); await viewer().getByRole('button', { name: 'Send', exact: true }).click(); - await expect(viewer().locator('.change-head:visible')).toHaveCount(0); + await expect(viewer().locator('.change-head:visible')).toHaveCount(await viewer().locator('.change').count()); await expect(viewer().locator('.change-map-mark')).toHaveCount(0); await expect(viewer().locator('.review-complete')).toBeVisible(); // Only History mode adds a row under the toolbar. @@ -390,11 +390,11 @@ try { await viewer().getByRole('button', { name: 'Back to review', exact: true }).click(); await expect(viewer().getByText('All changes reviewed', { exact: true })).toBeVisible(); await expect(viewer().getByText('There are no new changes to review. Previous decisions are available in History.', { exact: true })).toBeVisible(); - await expect(viewer().locator('.change-head:visible')).toHaveCount(0); + await expect(viewer().locator('.change-head:visible')).toHaveCount(await viewer().locator('.change').count()); await viewer().getByRole('button', { name: 'History', exact: true }).click(); await viewer().getByRole('button', { name: 'Undo last decision', exact: true }).click(); await expect(viewer().getByRole('button', { name: 'History', exact: true })).toBeVisible(); - await expect(viewer().locator('.change-head:visible')).toHaveCount(1); + await expect(viewer().locator('.change-head:visible')).toHaveCount(await viewer().locator('.change').count()); await expect(viewer().locator('.change.current')).toHaveAttribute('data-stop', '2'); // Mixed decisions cannot be shown as an entirely accepted or rejected PDF strip. await viewer().locator('.change.current').getByRole('button', { name: /LaTeX$/ }).click(); @@ -587,14 +587,14 @@ try { await expect(viewer().getByLabel('Review history')).toHaveCount(0); await expect(viewer().locator('.change[data-stop="0"]')).toHaveAttribute('data-state', 'accepted'); await viewer().locator('body').evaluate(() => { location.href = '?task=flow'; }); - await expect(viewer().locator('.change-head:visible')).toHaveCount(0); + await expect(viewer().locator('.change-head:visible')).toHaveCount(await viewer().locator('.change').count()); // A new result replaces the current review. Earlier results are inspection only. latest = 2; rounds[2] = { new: 'pending' }; task.review.round = 2; task.review.unsent = 0; update(); await sync(); await expect(viewer().locator('html')).toHaveAttribute('data-papilot-round', '2'); await expect(taskStatus).toHaveText('Ready for review'); assert.doesNotMatch(await viewer().locator('body').innerText(), /Round \d+/); - await expect(viewer().locator('.change-head:visible')).toHaveCount(1); + await expect(viewer().locator('.change-head:visible')).toHaveCount(await viewer().locator('.change').count()); await viewer().getByRole('button', { name: 'History', exact: true }).click(); assert.doesNotMatch(await viewer().getByLabel('Review history').innerText(), /Round \d+/); await expect(viewer().getByLabel('Review history').locator('option[value="1"]')).toContainText('2026'); @@ -612,7 +612,7 @@ try { await page.setViewportSize({ width: 390, height: 844 }); await page.getByRole('button', { name: /^Review(?:,|$)/ }).click(); await viewer().locator('.change.current').getByRole('button', { name: 'Accept', exact: true }).click(); - await expect(viewer().locator('.change-head:visible')).toHaveCount(0); + await expect(viewer().locator('.change-head:visible')).toHaveCount(await viewer().locator('.change').count()); await viewer().getByRole('button', { name: 'History', exact: true }).click(); await expect(viewer().getByRole('button', { name: 'Undo last decision', exact: true })).toBeInViewport(); await expect(viewer().getByRole('button', { name: 'Back to review', exact: true })).toBeInViewport();