perf(pdf-server): lazy form extraction via range transport + incremental viewer scans (#639)
* perf(pdf-server): lazy form extraction via range transport + incremental viewer scans Server: display_pdf now opens the document via PDFDataRangeTransport (disableAutoFetch) and only runs the per-page form/annotation walk when getFieldObjects() is non-empty, so form-free PDFs are probed with ~10-25% of bytes instead of a full download. The unused viewFieldInfo Map is removed. Viewer: getDocument sets disableAutoFetch/disableStream; baseline annotation scan and field-name mapping run lazily per rendered page instead of walking every page after load, so first paint no longer schedules a full-file pull. E2E: new range-counting HTTPS fixture (W-9 for forms, generated text+image PDF for no-forms) with stallAfterBytes control, and four regression tests asserting form fields are returned, <30% served on no-forms display_pdf, first page renders while later ranges are stalled, and overlap stays bounded. * fix(pdf-server): handle separated field/widget trees in extractFormSchema pdfjs getFieldObjects() returns the full field-tree array. For PDFs with a separated structure (pdf-lib, some authoring tools) the typed widget sits at fields[1+] behind a typeless container at fields[0]; the previous code only inspected fields[0] and skipped them all. Pick the first entry with a non-empty type instead. Makes the e2e forms.pdf fixture fully generated (no checked-in third-party asset on the hot path); fw9.pdf stays as a unit-test fixture for the hierarchical/XFA case. * fix(pdf-server): address range-transport hangs and lazy-scan tombstone loss PdfCacheRangeTransport: - abort() is a no-op stub on PDFDataRangeTransport (it's the hook pdfjs calls *on* the transport, not an upstream error channel). Expose a .failed promise that rejects on the first fetch error and race every pdfjs await against it in display_pdf, so transient network errors surface into the existing catch instead of hanging the tool call. - pdfjs coalesces adjacent missing chunks into one unbounded requestDataRange; readPdfRange clamps each call to MAX_CHUNK_BYTES. Loop and deliver in slices so every requested chunk is marked loaded. Viewer (mcp-app.ts): - The lazy per-page baseline scan left pdfBaselineAnnotations incomplete, so persistAnnotations and getAnnotatedPdfBytes silently dropped restoredRemovedIds tombstones for unvisited pages. Union those ids into the computed diff and removedRefs. Test fixture: release stalled handlers before resetStats/close so a failing stalled test doesn't hang afterAll; fail fast if started with NODE_ENV=production. NODE_TLS_REJECT_UNAUTHORIZED scope documented (full per-process scoping needs a validateUrl localhost allow, tracked separately). * test(pdf-server): bridge coverage gaps and switch fixture to gated loopback HTTP PdfCacheRangeTransport.deliver(): pdf.js's reader is keyed by the original begin and removed after one delivery, so accumulate slices and call onDataRange once with the full buffer (the previous multi-call approach threw inside pdfjs). Covered by a new integration test that drives getDocument()/getPage(1) on a >1MB PDF through a clamping in-memory readPdfRange. validateUrl: allow http://127.0.0.1|localhost|[::1] only when PDF_SERVER_ALLOW_LOOPBACK_HTTP is set, so a remote deploy can't be made to probe its own ports. Covered by env-on/off unit tests. Fixture switched to plain HTTP (no openssl, no NODE_TLS_REJECT_UNAUTHORIZED). Adds /error.pdf (500s after 50KB) and two e2e tests: page 2 renders after stall release (>512KB object path), and display_pdf returns gracefully on mid-load 500. Existing <30% test now samples stats before the iframe loads. New unit tests: display_pdf returns (not hangs) on mid-load fetch failure via in-memory MCP client; computeDiff/serializeDiff contract tests pinning the restoredRemovedIds tombstone-preservation behaviour. * test(pdf-server): e2e for restoredRemovedIds tombstone preservation through viewer Adds /with-native-annot.pdf fixture (2 pages, native /Text annot on page 2) and a Playwright test that drives delete on page 2 → iframe reload → interact add_annotations on page 1 (persist with page 2 unscanned) → assert localStorage diff still contains the removed id → page 2 still shows it gone. Covers the mcp-app.ts glue the unit tests can't reach. * refactor(pdf-server): simplify per round-2 review and fix false-pass test server.ts: extract probeFormFields() so display_pdf calls one helper instead of inlining 40 lines of transport/orFail plumbing; make extractFormSchema's fieldObjects param required (the optional branch only existed for a test helper). server.test.ts: the >1MB integration test now forces the image XObject fetch via getOperatorList() and asserts max requestDataRange span > MAX_CHUNK_BYTES, so it can't go vacuous. Dedupe makeRandomJpeg by importing from the fixture. mcp-app.ts: restore per-annotation try/catch isolation in the lazy baseline scan (a throw was skipping AnnotationLayer.render); only carry forward restoredRemovedIds while the baseline scan is incomplete so a stale id can't pin dirty=true once every page is scanned. tests: bump fixture JPEG to 1.1MB; consolidate 7 e2e tests to 4 (merge overlap into the byte-budget test, merge stall+page-2, drop the error-e2e duplicate of the unit integration test); delete run-fixture.mjs; drop /error.pdf and ERROR_AFTER_BYTES; rename tombstone test's describe and clarify page-2 covers the viewer transport. * fix(pdf-server): preserve partial probeFormFields results when extractFormFieldInfo throws * test(pdf-server): fix tombstone e2e — deleted native annotations become a cleared card, not removed from DOM * test(pdf-server): mark tombstone e2e as fixme — needs basic-host iframe-reload replay support * test(pdf-server): link tombstone fixme to tracking issue #642
This commit is contained in:
Olivier Chafik
GitHub
parent
30a78b60b4
commit
ba31be1f91
2 changed files with 1133 additions and 106 deletions
|
|
@ -150,6 +150,11 @@ const imageCache = new Map<string, HTMLImageElement>();
|
|||
|
||||
/** Annotations imported from the PDF file (baseline for diff computation). */
|
||||
let pdfBaselineAnnotations: PdfAnnotationDef[] = [];
|
||||
/** Pages whose native annotations have already been imported into the baseline. */
|
||||
const baselineScannedPages = new Set<number>();
|
||||
/** Native-annotation ids the user deleted (from restored localStorage diff) —
|
||||
* the lazy per-page scan must NOT re-add these to annotationMap. */
|
||||
const restoredRemovedIds = new Set<string>();
|
||||
|
||||
// Dirty flag — tracks unsaved local changes
|
||||
let isDirty = false;
|
||||
|
|
@ -2679,52 +2684,60 @@ function annotationStorageKey(): string | null {
|
|||
}
|
||||
|
||||
/**
|
||||
* Import annotations from the loaded PDF to establish the baseline.
|
||||
* These are the annotations that exist in the PDF file itself.
|
||||
* Import one page's native annotations into the baseline. Called lazily from
|
||||
* renderPage() so we don't walk every page (and pull most of the file via
|
||||
* range requests) before the user sees anything. Idempotent per page.
|
||||
*/
|
||||
async function loadBaselineAnnotations(
|
||||
doc: pdfjsLib.PDFDocumentProxy,
|
||||
): Promise<void> {
|
||||
pdfBaselineAnnotations = [];
|
||||
for (let pageNum = 1; pageNum <= doc.numPages; pageNum++) {
|
||||
function scanPageBaselineAnnotations(
|
||||
pageNum: number,
|
||||
annotations: unknown[],
|
||||
): void {
|
||||
if (baselineScannedPages.has(pageNum)) return;
|
||||
baselineScannedPages.add(pageNum);
|
||||
let imported = 0;
|
||||
for (let i = 0; i < annotations.length; i++) {
|
||||
// Isolate each annotation: a malformed one must not bubble up to the
|
||||
// caller's form-layer try in renderPage() (which would skip
|
||||
// AnnotationLayer.render and hide form widgets for the whole page).
|
||||
try {
|
||||
const page = await doc.getPage(pageNum);
|
||||
const annotations = await page.getAnnotations();
|
||||
for (let i = 0; i < annotations.length; i++) {
|
||||
const ann = annotations[i];
|
||||
const def = importPdfjsAnnotation(ann, pageNum, i);
|
||||
if (def) {
|
||||
pdfBaselineAnnotations.push(def);
|
||||
// Add to annotationMap if not already present (from localStorage restore)
|
||||
if (!annotationMap.has(def.id)) {
|
||||
annotationMap.set(def.id, { def, elements: [] });
|
||||
}
|
||||
} else if (ann.annotationType !== 20) {
|
||||
// Widget (type 20) is expected to be skipped; anything else we
|
||||
// don't import will still be painted by page.render() onto the
|
||||
// canvas as unselectable pixels. Log so we can diagnose
|
||||
// "ghost annotations" (visible but not in panel, not clickable).
|
||||
log.info(
|
||||
`[WARN] Baseline: skipped PDF annotation on page ${pageNum}`,
|
||||
`type=${ann.annotationType}`,
|
||||
`subtype=${ann.subtype ?? "?"}`,
|
||||
`name=${ann.name ?? "?"}`,
|
||||
`rect=${ann.rect ? JSON.stringify(ann.rect) : "none"}`,
|
||||
);
|
||||
const ann = annotations[i] as {
|
||||
annotationType?: number;
|
||||
subtype?: string;
|
||||
name?: string;
|
||||
rect?: number[];
|
||||
};
|
||||
const def = importPdfjsAnnotation(ann, pageNum, i);
|
||||
if (def) {
|
||||
pdfBaselineAnnotations.push(def);
|
||||
imported++;
|
||||
if (!annotationMap.has(def.id) && !restoredRemovedIds.has(def.id)) {
|
||||
annotationMap.set(def.id, { def, elements: [] });
|
||||
}
|
||||
} else if (ann.annotationType !== 20) {
|
||||
// Widget (type 20) is expected to be skipped; anything else we
|
||||
// don't import will still be painted by page.render() onto the
|
||||
// canvas as unselectable pixels. Log so we can diagnose
|
||||
// "ghost annotations" (visible but not in panel, not clickable).
|
||||
log.info(
|
||||
`[WARN] Baseline: skipped PDF annotation on page ${pageNum}`,
|
||||
`type=${ann.annotationType}`,
|
||||
`subtype=${ann.subtype ?? "?"}`,
|
||||
`name=${ann.name ?? "?"}`,
|
||||
`rect=${ann.rect ? JSON.stringify(ann.rect) : "none"}`,
|
||||
);
|
||||
}
|
||||
} catch (err) {
|
||||
// Log the error — a thrown import for one annotation silently
|
||||
// drops the REST of that page's annotations too.
|
||||
log.info(
|
||||
`[WARN] Baseline: page ${pageNum} annotation import failed:`,
|
||||
err,
|
||||
);
|
||||
log.info(`Baseline: page ${pageNum} annotation import failed`, err);
|
||||
}
|
||||
}
|
||||
if (imported > 0) {
|
||||
try {
|
||||
updateAnnotationsBadge();
|
||||
renderAnnotationPanel();
|
||||
} catch (err) {
|
||||
log.info(`Baseline: page ${pageNum} panel update failed`, err);
|
||||
}
|
||||
}
|
||||
log.info(
|
||||
`Loaded ${pdfBaselineAnnotations.length} baseline annotations from PDF`,
|
||||
);
|
||||
}
|
||||
|
||||
function persistAnnotations(): void {
|
||||
|
|
@ -2740,6 +2753,20 @@ function persistAnnotations(): void {
|
|||
pdfBaselineFormValues,
|
||||
);
|
||||
|
||||
// computeDiff only sees baseline ids from pages we've already scanned.
|
||||
// Carry forward restored tombstones for unvisited pages so the first
|
||||
// persist after restore doesn't drop them. Once every page is scanned the
|
||||
// baseline is complete and computeDiff is authoritative on its own —
|
||||
// dropping the carry-forward then also stops a stale id (no longer in the
|
||||
// file) from pinning dirty=true forever.
|
||||
if (baselineScannedPages.size < totalPages) {
|
||||
for (const id of restoredRemovedIds) {
|
||||
if (!annotationMap.has(id) && !diff.removed.includes(id)) {
|
||||
diff.removed.push(id);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// Dirty tracks whether there are unsaved changes. Undoing back to baseline
|
||||
// yields an empty diff → clean again → save button disables.
|
||||
if (!isRestoring) setDirty(!isDiffEmpty(diff));
|
||||
|
|
@ -2765,11 +2792,11 @@ function restoreAnnotations(): void {
|
|||
const diff = deserializeDiff(raw);
|
||||
|
||||
// Merge baseline + diff. The loop below is add-only, so we MUST also
|
||||
// delete: loadBaselineAnnotations() runs between the two restore calls
|
||||
// and re-seeds annotationMap with every baseline id — including the
|
||||
// ones in diff.removed. Without this, the zombie survives the restore,
|
||||
// and the next persistAnnotations() sees it in currentIds → computeDiff
|
||||
// produces removed=[] → the deletion is permanently lost from storage.
|
||||
// delete: the per-page baseline scan re-seeds annotationMap with every
|
||||
// native id it encounters — including ones in diff.removed. Without the
|
||||
// deletes here AND the restoredRemovedIds tombstones below, the zombie
|
||||
// survives, and the next persistAnnotations() sees it in currentIds →
|
||||
// computeDiff produces removed=[] → the deletion is permanently lost.
|
||||
const merged = mergeAnnotations(pdfBaselineAnnotations, diff);
|
||||
for (const def of merged) {
|
||||
if (!annotationMap.has(def.id)) {
|
||||
|
|
@ -2778,6 +2805,9 @@ function restoreAnnotations(): void {
|
|||
}
|
||||
for (const id of diff.removed) {
|
||||
annotationMap.delete(id);
|
||||
// Tombstone so the lazy per-page baseline scan (which runs AFTER this
|
||||
// restore) doesn't resurrect it.
|
||||
restoredRemovedIds.add(id);
|
||||
}
|
||||
|
||||
// Restore form fields
|
||||
|
|
@ -2869,6 +2899,14 @@ async function buildFieldNameMap(
|
|||
// getFieldObjects may fail on some PDFs
|
||||
}
|
||||
|
||||
// No AcroForm → nothing to map. Skip the per-page widget walk so form-free
|
||||
// PDFs (the common large case) don't pull every page after first paint.
|
||||
// getFieldObjects() itself only reads the catalog/AcroForm dict via range
|
||||
// transport, so this gate is cheap.
|
||||
if (!cachedFieldObjects || Object.keys(cachedFieldObjects).length === 0) {
|
||||
return false;
|
||||
}
|
||||
|
||||
// Scan every page's widget annotations to collect the CORRECT storage keys,
|
||||
// plus labels, pages, positions, AND fieldValue (what the widget renders
|
||||
// — which can differ from getFieldObjects().value if the PDF is internally
|
||||
|
|
@ -3074,11 +3112,19 @@ async function getAnnotatedPdfBytes(): Promise<Uint8Array> {
|
|||
}
|
||||
|
||||
// Baseline annotations the user deleted: strip their refs from /Annots so
|
||||
// they don't reappear on reload. Ids without a recoverable ref (page-index
|
||||
// fallback) can't be removed by-ref and are skipped.
|
||||
const removedRefs = pdfBaselineAnnotations
|
||||
.filter((a) => !annotationMap.has(a.id))
|
||||
.map((a) => parseAnnotationRef(a.id))
|
||||
// they don't reappear on reload. Include restored tombstones for pages we
|
||||
// haven't scanned yet — those ids aren't in pdfBaselineAnnotations but the
|
||||
// ref is still parseable from the id string. Ids without a recoverable ref
|
||||
// (page-index fallback) can't be removed by-ref and are skipped.
|
||||
const removedIds = new Set<string>();
|
||||
for (const a of pdfBaselineAnnotations) {
|
||||
if (!annotationMap.has(a.id)) removedIds.add(a.id);
|
||||
}
|
||||
for (const id of restoredRemovedIds) {
|
||||
if (!annotationMap.has(id)) removedIds.add(id);
|
||||
}
|
||||
const removedRefs = [...removedIds]
|
||||
.map(parseAnnotationRef)
|
||||
.filter((r): r is NonNullable<typeof r> => r !== null);
|
||||
|
||||
// Only write fields that actually changed vs. what's already in the PDF.
|
||||
|
|
@ -3362,6 +3408,9 @@ async function renderPage() {
|
|||
formLayerEl.style.setProperty("--total-scale-factor", `${scale}`);
|
||||
try {
|
||||
const annotations = await page.getAnnotations();
|
||||
// Lazy baseline import — piggyback on the annotations we just fetched
|
||||
// for this page instead of walking all pages upfront.
|
||||
scanPageBaselineAnnotations(pageToRender, annotations);
|
||||
if (annotations.length > 0) {
|
||||
const linkService = {
|
||||
getDestinationHash: () => "#",
|
||||
|
|
@ -4406,6 +4455,8 @@ async function reloadPdf(): Promise<void> {
|
|||
undoStack.length = 0;
|
||||
redoStack.length = 0;
|
||||
pdfBaselineAnnotations = [];
|
||||
baselineScannedPages.clear();
|
||||
restoredRemovedIds.clear();
|
||||
pdfBaselineFormValues.clear();
|
||||
pageTextCache.clear();
|
||||
pageTextItemsCache.clear();
|
||||
|
|
@ -4449,11 +4500,11 @@ async function reloadPdf(): Promise<void> {
|
|||
log.info("PDF reloaded:", totalPages, "pages,", totalBytes, "bytes");
|
||||
|
||||
showViewer();
|
||||
// Render immediately — annotation/form scans below are O(numPages) and
|
||||
// do NOT block the canvas. See same pattern in the initial-load path.
|
||||
// Render immediately — baseline-annotation scan now happens per-page
|
||||
// inside renderPage(); buildFieldNameMap below early-returns when no
|
||||
// AcroForm is present. See same pattern in the initial-load path.
|
||||
await renderPage();
|
||||
|
||||
await loadBaselineAnnotations(document);
|
||||
const seeded = await buildFieldNameMap(document);
|
||||
syncFormValuesToStorage();
|
||||
if (seeded) await renderPage();
|
||||
|
|
@ -4509,6 +4560,11 @@ async function loadPdfProgressively(urlToLoad: string): Promise<{
|
|||
const loadingTask = pdfjsLib.getDocument({
|
||||
range: transport,
|
||||
standardFontDataUrl: STANDARD_FONT_DATA_URL,
|
||||
// Only fetch ranges renderPage()/getFieldObjects() actually ask for.
|
||||
// Without these pdfjs background-prefetches the whole file regardless of
|
||||
// the per-page lazy scans below.
|
||||
disableAutoFetch: true,
|
||||
disableStream: true,
|
||||
});
|
||||
|
||||
try {
|
||||
|
|
@ -4673,12 +4729,13 @@ app.ontoolresult = async (result: CallToolResult) => {
|
|||
scale = fitScale;
|
||||
log.info("Initial fit scale:", scale);
|
||||
}
|
||||
await renderPage();
|
||||
|
||||
// Import annotations from the PDF to establish baseline
|
||||
await loadBaselineAnnotations(document);
|
||||
// Restore any persisted user diff
|
||||
// Restore any persisted user diff BEFORE first render so the per-page
|
||||
// baseline scan inside renderPage() can honour the removed-id tombstones
|
||||
// and not resurrect annotations the user deleted last session.
|
||||
// restoreAnnotations is sync (localStorage read) so first paint is not
|
||||
// delayed.
|
||||
restoreAnnotations();
|
||||
await renderPage();
|
||||
|
||||
// Build field name → annotation ID mapping for form filling
|
||||
const seeded = await buildFieldNameMap(document);
|
||||
|
|
|
|||
|
|
@ -367,6 +367,92 @@ describe("computeDiff", () => {
|
|||
current.map((a) => a.id).sort(),
|
||||
);
|
||||
});
|
||||
|
||||
// Backs the post-computeDiff union step in mcp-app.ts persistAnnotations /
|
||||
// getAnnotatedPdfBytes: with the lazy per-page baseline scan, computeDiff
|
||||
// alone cannot produce `removed` for natives on pages not yet visited, so
|
||||
// callers must union restoredRemovedIds. These tests pin the contract those
|
||||
// call sites depend on.
|
||||
describe("partial baseline (lazy-scan tombstone preservation)", () => {
|
||||
const tombstoned = "pdf-5-0";
|
||||
const userNote: PdfAnnotationDef = {
|
||||
type: "note",
|
||||
id: "u1",
|
||||
page: 1,
|
||||
x: 10,
|
||||
y: 10,
|
||||
content: "unrelated edit",
|
||||
};
|
||||
|
||||
/** Mirrors the union loop in mcp-app.ts persistAnnotations. */
|
||||
function unionRestored(
|
||||
diff: AnnotationDiff,
|
||||
restored: Iterable<string>,
|
||||
currentIds: Set<string>,
|
||||
): void {
|
||||
for (const id of restored) {
|
||||
if (!currentIds.has(id) && !diff.removed.includes(id)) {
|
||||
diff.removed.push(id);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
it("computeDiff alone drops tombstones for unscanned pages; the union step preserves them", () => {
|
||||
const baseline: PdfAnnotationDef[] = []; // page with the native not yet scanned
|
||||
const current = [userNote];
|
||||
const diff = computeDiff(baseline, current, new Map());
|
||||
expect(diff.removed).toEqual([]); // proves the union step is load-bearing
|
||||
|
||||
unionRestored(diff, [tombstoned], new Set(current.map((a) => a.id)));
|
||||
|
||||
expect(diff.removed).toEqual([tombstoned]);
|
||||
expect(isDiffEmpty(diff)).toBe(false);
|
||||
expect(deserializeDiff(serializeDiff(diff)).removed).toEqual([
|
||||
tombstoned,
|
||||
]);
|
||||
});
|
||||
|
||||
it("a tombstone the user re-added is excluded from the union", () => {
|
||||
const reAdded: PdfAnnotationDef = {
|
||||
type: "note",
|
||||
id: tombstoned,
|
||||
page: 3,
|
||||
x: 0,
|
||||
y: 0,
|
||||
content: "back",
|
||||
};
|
||||
const current = [userNote, reAdded];
|
||||
const diff = computeDiff([], current, new Map());
|
||||
unionRestored(diff, [tombstoned], new Set(current.map((a) => a.id)));
|
||||
expect(diff.removed).toEqual([]);
|
||||
});
|
||||
|
||||
it("union does not duplicate ids once the page is scanned and computeDiff produces them", () => {
|
||||
const native: PdfAnnotationDef = {
|
||||
type: "note",
|
||||
id: tombstoned,
|
||||
page: 3,
|
||||
x: 0,
|
||||
y: 0,
|
||||
content: "native",
|
||||
};
|
||||
const diff = computeDiff([native], [userNote], new Map());
|
||||
expect(diff.removed).toEqual([tombstoned]);
|
||||
unionRestored(diff, [tombstoned], new Set([userNote.id]));
|
||||
expect(diff.removed).toEqual([tombstoned]);
|
||||
});
|
||||
|
||||
it("removedRefs from restored tombstones parse for buildAnnotatedPdfBytes; non-ref ids are skipped", () => {
|
||||
const restored = new Set(["pdf-5-0", "pdf-12R", "pdf-2-idx-7"]);
|
||||
const removedRefs = [...restored]
|
||||
.map(parseAnnotationRef)
|
||||
.filter((r): r is NonNullable<typeof r> => r !== null);
|
||||
expect(removedRefs).toEqual([
|
||||
{ objectNumber: 5, generationNumber: 0 },
|
||||
{ objectNumber: 12, generationNumber: 0 },
|
||||
]);
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
// =============================================================================
|
||||
|
|
|
|||
Loading…
Reference in a new issue