fix(pdf-server): radio + dropdown in fill_form/save (#577)

* fix(pdf-server): fail fast on never-polled viewer + surface batch errors

interact's get_screenshot/get_text waited the full 45s for a viewer that
never mounted (host went idle before iframe reached startPolling), then
timed out with a generic message. Now tracks viewsPolled and fails in ~8s
with a recovery hint. Batch errors were also ordered success-first —
hosts that flatten isError to content[0] showed "Queued: Filled N fields"
and dropped the actual failure entirely. Viewer side: poll even when PDF
load throws, and catch connect() rejection instead of silently blanking.

* fix(pdf-server): radio button + dropdown handling in fill_form and save

Three related bugs around non-text form fields:

Viewer fill_form (mcp-app.ts): writing a string to annotationStorage on
every radio widget trips pdf.js's inverted string coercion
(`value !== buttonValue` → true) — the first-rendered widget checks
itself and clears the rest, so whatever you ask for, you get the first
option. Fix: extract a setFieldInStorage() helper that writes
{value: true} only on the widget whose buttonValue matches, {value: false}
on siblings, matching pdf.js's own change handler. Also covers the
syncFormValuesToStorage() path (localStorage restore).

Save (pdf-annotations.ts): buildAnnotatedPdfBytes only knew about
getTextField/getCheckBox — dropdowns and radios threw in the try block
and fell through the catch, so they were silently dropped on save.
Fix: instanceof dispatch on getFieldMaybe(). For radios, accept either
the option label or a numeric buttonValue (indexed into getOptions()),
since the viewer stores the latter for PDFs with /Opt.

display_pdf output (server.ts): radio groups showed as N identical
`name [Btn]` lines with no way to tell them apart; dropdowns didn't
list their options. Fix: surface buttonValue as exportValue and the
options array from the annotation data already in hand.
This commit is contained in:
Olivier Chafik 2026-04-01 22:28:28 +01:00 • committed by GitHub
commit 38700569e7
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
5 changed files with 434 additions and 72 deletions

View file

@ -1054,4 +1054,142 @@ describe("interact tool", () => {
await server.close();
});
});
describe("viewer liveness", () => {
// get_screenshot/get_text fail fast when the iframe never polled, instead
// of waiting 45s for a viewer that isn't there. Reproduces the case where
// the host goes idle before the iframe reaches startPolling().
it("get_screenshot fails fast when viewer never polled", async () => {
const { server, client } = await connect();
const uuid = "never-polled-screenshot";
const started = Date.now();
const r = await client.callTool({
name: "interact",
arguments: { viewUUID: uuid, action: "get_screenshot", page: 1 },
});
const elapsed = Date.now() - started;
expect(r.isError).toBe(true);
expect(firstText(r)).toContain("never connected");
expect(firstText(r)).toContain(uuid);
expect(firstText(r)).toContain("display_pdf again"); // recovery hint
// Fast-fail bound (~8s grace), well under the 45s page-data timeout.
// 15s upper bound leaves slack for CI scheduling.
expect(elapsed).toBeLessThan(15_000);
await client.close();
await server.close();
}, 20_000);
it("get_screenshot waits full timeout when viewer polled then went silent", async () => {
// Viewer polled once (proving it mounted) then hung on a heavy render.
// The grace check passes, so we fall through to the 45s page-data wait —
// verified here by racing against a 12s deadline that should NOT win.
const { server, client } = await connect();
const uuid = "polled-then-silent";
// Viewer's first poll: drain whatever's there so it returns fast.
// Enqueue a trivial command first so poll returns via the batch-wait
// path (~200ms) instead of blocking on the 30s long-poll.
await client.callTool({
name: "interact",
arguments: { viewUUID: uuid, action: "navigate", page: 1 },
});
await poll(client, uuid);
// Now get_screenshot — viewer has polled, so no fast-fail. But viewer
// never calls submit_page_data → should wait beyond the grace period.
const outcome = await Promise.race([
client
.callTool({
name: "interact",
arguments: { viewUUID: uuid, action: "get_screenshot", page: 1 },
})
.then(() => "completed" as const),
new Promise<"still-waiting">((r) =>
setTimeout(() => r("still-waiting"), 12_000),
),
]);
expect(outcome).toBe("still-waiting");
await client.close();
await server.close();
}, 20_000);
it("get_screenshot succeeds when viewer polls during grace window", async () => {
// Model calls interact before the viewer has polled — but the viewer
// shows up within the grace period and completes the roundtrip.
const { server, client } = await connect();
const uuid = "late-arriving-viewer";
const interactPromise = client.callTool({
name: "interact",
arguments: { viewUUID: uuid, action: "get_screenshot", page: 1 },
});
// Viewer connects 500ms late — well inside the grace window.
await new Promise((r) => setTimeout(r, 500));
const cmds = await poll(client, uuid);
const getPages = cmds.find((c) => c.type === "get_pages");
expect(getPages).toBeDefined();
// Viewer responds with the page data.
await client.callTool({
name: "submit_page_data",
arguments: {
requestId: getPages!.requestId as string,
pages: [
{ page: 1, image: Buffer.from("fake-jpeg").toString("base64") },
],
},
});
const r = await interactPromise;
expect(r.isError).toBeFalsy();
expect((r.content as Array<{ type: string }>)[0].type).toBe("image");
await client.close();
await server.close();
}, 15_000);
it("batch failure puts error message first", async () => {
// When [fill_form, get_screenshot] runs and get_screenshot times out,
// content[0] must describe the failure — not the earlier success. Some
// hosts flatten isError results to content[0].text only, which previously
// showed "Queued: Filled N fields" with isError:true and dropped the
// actual timeout entirely.
const { server, client } = await connect();
const uuid = "batch-error-ordering";
const r = await client.callTool({
name: "interact",
arguments: {
viewUUID: uuid,
commands: [
{ action: "navigate", page: 3 }, // succeeds → "Queued: ..."
{ action: "get_screenshot", page: 1 }, // never-polled → fast-fail
],
},
});
expect(r.isError).toBe(true);
const texts = (r.content as Array<{ type: string; text: string }>).map(
(c) => c.text,
);
// content[0]: batch-failed summary naming the culprit
expect(texts[0]).toContain("failed");
expect(texts[0]).toContain("2/2");
expect(texts[0]).toContain("get_screenshot");
// content[1]: the actual error
expect(texts[1]).toContain("never connected");
// content[2]: the earlier success, pushed to the back
expect(texts[2]).toContain("Queued");
await client.close();
await server.close();
}, 15_000);
});
});

View file

@ -196,6 +196,14 @@ export type { PdfCommand };
// reject first and return a real error instead of the client cancelling us.
const GET_PAGES_TIMEOUT_MS = 45_000;
/**
* Grace period for the viewer's first poll. If interact() arrives before the
* iframe has ever polled, we wait this long for it to show up (iframe mount +
* PDF load + startPolling). If no poll comes, the viewer almost certainly
* never rendered — failing fast beats a silent 45s hang.
*/
const VIEWER_FIRST_POLL_GRACE_MS = 8_000;
interface PageDataEntry {
page: number;
text?: string;
@ -232,6 +240,33 @@ function waitForPageData(
});
}
/**
* Wait for the viewer's first poll_pdf_commands call.
*
* Called before waitForPageData() so a viewer that never mounted fails in ~8s
* with a specific message instead of a generic 45s "Timeout waiting for page
* data" that gives no hint why.
*
* Intentionally does NOT touch pollWaiters: piggybacking on that single-slot
* Map races with poll_pdf_commands' batch-wait branch (which never cancels the
* prior waiter) and with concurrent interact calls (which would overwrite each
* other). A plain check loop on viewsPolled is stateless — multiple callers
* can wait independently and all observe the same add() when it happens.
*/
async function ensureViewerIsPolling(uuid: string): Promise<void> {
const deadline = Date.now() + VIEWER_FIRST_POLL_GRACE_MS;
while (!viewsPolled.has(uuid)) {
if (Date.now() >= deadline) {
throw new Error(
`Viewer never connected for viewUUID ${uuid} (no poll within ${VIEWER_FIRST_POLL_GRACE_MS / 1000}s). ` +
`The iframe likely failed to mount — this happens when the conversation ` +
`goes idle before the viewer finishes loading. Call display_pdf again to get a fresh viewUUID.`,
);
}
await new Promise((r) => setTimeout(r, 100));
}
}
interface QueueEntry {
commands: PdfCommand[];
/** Timestamp of the most recent enqueue or dequeue */
@ -243,6 +278,15 @@ const commandQueues = new Map<string, QueueEntry>();
/** Waiters for long-poll: resolve callback wakes up a blocked poll_pdf_commands */
const pollWaiters = new Map<string, () => void>();
/**
* viewUUIDs that have been polled at least once. A view missing from this set
* means the iframe never reached startPolling() — usually because it wasn't
* mounted yet, or ontoolresult threw before the poll loop started. Used to
* fail fast in get_screenshot/get_text instead of waiting the full 45s for
* a viewer that was never there.
*/
const viewsPolled = new Set<string>();
/** Valid form field names per viewer UUID (populated during display_pdf) */
const viewFieldNames = new Map<string, Set<string>>();
@ -288,6 +332,7 @@ function pruneStaleQueues(): void {
commandQueues.delete(uuid);
viewFieldNames.delete(uuid);
viewFieldInfo.delete(uuid);
viewsPolled.delete(uuid);
stopFileWatch(uuid);
}
}
@ -812,6 +857,10 @@ interface FormFieldInfo {
y: number;
width: number;
height: number;
/** Radio button export value (buttonValue) — distinguishes widgets that share a field name. */
exportValue?: string;
/** Dropdown/listbox option values, as seen in the widget's `options` array. */
options?: string[];
}
/**
@ -864,6 +913,16 @@ async function extractFormFieldInfo(
// Convert to model coords (top-left origin): modelY = pageHeight - pdfY - height
const modelY = pageHeight - y2;
// Choice widgets (combo/listbox) carry `options` as
// [{exportValue, displayValue}]. Expose export values — that's
// what fill_form needs.
let options: string[] | undefined;
if (Array.isArray(ann.options) && ann.options.length > 0) {
options = ann.options
.map((o: { exportValue?: string }) => o?.exportValue)
.filter((v: unknown): v is string => typeof v === "string");
}
fields.push({
name: fieldName,
type: fieldType,
@ -873,6 +932,12 @@ async function extractFormFieldInfo(
width: Math.round(width),
height: Math.round(height),
...(ann.alternativeText ? { label: ann.alternativeText } : undefined),
// Radio: buttonValue is the per-widget export value — the only
// thing distinguishing three `size [Btn]` lines from each other.
...(ann.radioButton && ann.buttonValue != null
? { exportValue: String(ann.buttonValue) }
: undefined),
...(options?.length ? { options } : undefined),
});
}
}
@ -1282,6 +1347,14 @@ Set \`elicit_form_inputs\` to true to prompt the user to fill form fields before
y: z.number(),
width: z.number(),
height: z.number(),
exportValue: z
.string()
.optional()
.describe("Radio button value — pass this to fill_form"),
options: z
.array(z.string())
.optional()
.describe("Dropdown/listbox option values"),
}),
)
.optional()
@ -1453,8 +1526,14 @@ URL: ${normalized}`,
for (const f of fields) {
const label = f.label ? ` "${f.label}"` : "";
const nameStr = f.name || "(unnamed)";
// Radio: =<exportValue> tells the model what value to pass.
// Dropdown: options:[...] lists valid choices.
const exportSuffix = f.exportValue ? `=${f.exportValue}` : "";
const optsSuffix = f.options
? ` options:[${f.options.join(", ")}]`
: "";
lines.push(
` ${nameStr}${label} [${f.type}] at (${f.x},${f.y}) ${f.width}×${f.height}`,
` ${nameStr}${exportSuffix}${label} [${f.type}] at (${f.x},${f.y}) ${f.width}×${f.height}${optsSuffix}`,
);
}
}
@ -1925,6 +2004,7 @@ URL: ${normalized}`,
let pageData: PageDataEntry[];
try {
await ensureViewerIsPolling(uuid);
pageData = await waitForPageData(requestId, signal);
} catch (err) {
return {
@ -1973,6 +2053,7 @@ URL: ${normalized}`,
let pageData: PageDataEntry[];
try {
await ensureViewerIsPolling(uuid);
pageData = await waitForPageData(requestId, signal);
} catch (err) {
return {
@ -2202,7 +2283,7 @@ Example — add a signature image and a stamp, then screenshot to verify:
// Process commands sequentially, collecting all content parts
const allContent: ContentPart[] = [];
let hasError = false;
let failedAt = -1;
for (let i = 0; i < commandList.length; i++) {
const result = await processInteractCommand(
@ -2211,15 +2292,27 @@ Example — add a signature image and a stamp, then screenshot to verify:
extra.signal,
);
if (result.isError) {
hasError = true;
// Error content first. Some hosts flatten isError results to
// content[0].text — if we push the error after prior successes,
// the user sees "Queued: Filled 7 fields" with isError:true and
// the actual failure is silently dropped.
allContent.unshift(...result.content);
failedAt = i;
break;
}
allContent.push(...result.content);
if (hasError) break; // Stop on first error
}
if (failedAt >= 0 && commandList.length > 1) {
allContent.unshift({
type: "text",
text: `Batch failed at step ${failedAt + 1}/${commandList.length} (${commandList[failedAt].action}):`,
});
}
return {
content: allContent,
...(hasError ? { isError: true } : {}),
...(failedAt >= 0 ? { isError: true } : {}),
};
},
);
@ -2280,6 +2373,7 @@ Example — add a signature image and a stamp, then screenshot to verify:
_meta: { ui: { visibility: ["app"] } },
},
async ({ viewUUID: uuid }): Promise<CallToolResult> => {
viewsPolled.add(uuid);
// If commands are already queued, wait briefly to let more accumulate
if (commandQueues.has(uuid)) {
await new Promise((r) => setTimeout(r, POLL_BATCH_WAIT_MS));

View file

@ -2708,6 +2708,60 @@ async function buildFieldNameMap(
log.info(`Built field name map: ${fieldNameToIds.size} fields`);
}
/**
* Set one form field's value in pdf.js's annotationStorage, in the format
* AnnotationLayer expects to READ when it re-renders.
*
* Radio buttons need per-widget booleans: pdf.js's RadioButtonWidgetAnnotation
* render() has inverted string coercion (`value !== buttonValue` → true for
* every NON-matching widget), so a string value on all widgets checks the
* first rendered one and clears the rest regardless of what you asked for.
* Match pdf.js's own change handler instead: `{value: true}` on the widget
* whose buttonValue matches, `{value: false}` on the siblings.
*
* Also patches the live DOM element for the current page so the user sees the
* change without waiting for a full re-render.
*/
function setFieldInStorage(name: string, value: string | boolean): void {
if (!pdfDocument) return;
const ids = fieldNameToIds.get(name);
if (!ids) return;
const storage = pdfDocument.annotationStorage;
// Radio group: at least one widget ID has a buttonValue recorded.
const isRadio = ids.some((id) => radioButtonValues.has(id));
if (isRadio) {
const want = String(value);
for (const id of ids) {
const checked = radioButtonValues.get(id) === want;
storage.setValue(id, { value: checked });
const el = formLayerEl.querySelector(
`input[data-element-id="${id}"]`,
) as HTMLInputElement | null;
if (el) el.checked = checked;
}
return;
}
// Text / checkbox / select: same value on every widget (a field can have
// multiple widget annotations sharing one /V).
const storageValue = typeof value === "boolean" ? value : String(value);
for (const id of ids) {
storage.setValue(id, { value: storageValue });
const el = formLayerEl.querySelector(`[data-element-id="${id}"]`) as
| HTMLInputElement
| HTMLSelectElement
| HTMLTextAreaElement
| null;
if (!el) continue;
if (el instanceof HTMLInputElement && el.type === "checkbox") {
el.checked = !!value;
} else {
el.value = String(value);
}
}
}
/** Sync formFieldValues into pdfDocument.annotationStorage so AnnotationLayer renders pre-filled values.
* Skips values that match the PDF's baseline — those are already in storage
* in pdf.js's native format (which may differ from our string/bool repr,
@ -2715,17 +2769,9 @@ async function buildFieldNameMap(
* form can break the Reset button's ability to restore defaults. */
function syncFormValuesToStorage(): void {
if (!pdfDocument || fieldNameToIds.size === 0) return;
const storage = pdfDocument.annotationStorage;
for (const [name, value] of formFieldValues) {
if (pdfBaselineFormValues.get(name) === value) continue;
const ids = fieldNameToIds.get(name);
if (ids) {
for (const id of ids) {
storage.setValue(id, {
value: typeof value === "boolean" ? value : String(value),
});
}
}
setFieldInStorage(name, value);
}
}
@ -4093,6 +4139,15 @@ app.ontoolresult = async (result: CallToolResult) => {
} catch (err) {
log.error("Error loading PDF:", err);
showError(err instanceof Error ? err.message : String(err));
// Poll anyway. The server's interact tool has no way to know we choked —
// without a poll it waits 45s on every get_screenshot against this
// viewUUID. handleGetPages already null-guards pdfDocument, so a failed
// load just means empty page data → server returns "No screenshot
// returned" (fast, actionable) instead of "Timeout waiting for page data
// from viewer" (slow, opaque).
if (viewUUID && interactEnabled) {
startPolling();
}
}
};
@ -4213,44 +4268,10 @@ async function processCommands(commands: PdfCommand[]): Promise<void> {
case "fill_form":
for (const field of cmd.fields) {
formFieldValues.set(field.name, field.value);
// Set in PDF.js annotation storage and update DOM elements directly
if (pdfDocument) {
const ids = fieldNameToIds.get(field.name);
if (ids) {
for (const id of ids) {
pdfDocument.annotationStorage.setValue(id, {
value:
typeof field.value === "boolean"
? field.value
: String(field.value),
});
// Update the live DOM element if it exists on the current page
const el = formLayerEl.querySelector(
`[data-element-id="${id}"]`,
) as
| HTMLInputElement
| HTMLSelectElement
| HTMLTextAreaElement
| null;
if (el) {
if (
el instanceof HTMLInputElement &&
el.type === "checkbox"
) {
el.checked = !!field.value;
} else if (el instanceof HTMLSelectElement) {
el.value = String(field.value);
} else {
el.value = String(field.value);
}
}
}
} else {
log.info(
`fill_form: no annotation IDs for field "${field.name}"`,
);
}
if (!fieldNameToIds.has(field.name)) {
log.info(`fill_form: no annotation IDs for field "${field.name}"`);
}
setFieldInStorage(field.name, field.value);
}
// Re-render to show updated form values (handles fields on other pages)
renderPage();
@ -4440,16 +4461,27 @@ app.onteardown = async () => {
app.onhostcontextchanged = handleHostContextChanged;
// Connect to host
app.connect().then(() => {
log.info("Connected to host");
const ctx = app.getHostContext();
if (ctx) {
handleHostContextChanged(ctx);
}
// Restore annotations early using toolInfo.id (available before tool result)
restoreAnnotations();
updateAnnotationsBadge();
});
app
.connect()
.then(() => {
log.info("Connected to host");
const ctx = app.getHostContext();
if (ctx) {
handleHostContextChanged(ctx);
}
// Restore annotations early using toolInfo.id (available before tool result)
restoreAnnotations();
updateAnnotationsBadge();
})
.catch((err: unknown) => {
// ui/initialize failed or transport rejected. Without a catch this is an
// unhandled rejection — iframe shows blank, server times out on every
// interact call with no clue why.
log.error("Failed to connect to host:", err);
showError(
`Failed to connect to host: ${err instanceof Error ? err.message : String(err)}`,
);
});
// Debug helper: dump all annotation state. Run in DevTools console as
// `__pdfDebug()` to diagnose ghost annotations (visible on canvas but not

View file

@ -1031,6 +1031,81 @@ describe("buildAnnotatedPdfBytes", () => {
const bytes2 = await doc.save();
expect(bytes2.length).toBeGreaterThan(0);
});
describe("form field persistence", () => {
// One fixture with every field type we support. pdf-lib's addOptionToPage
// writes radio buttonValues as numeric index strings ("0","1","2"), which
// is the stress case — the viewer stores those, but .select() wants labels.
let formPdfBytes: Uint8Array;
beforeAll(async () => {
const doc = await PDFDocument.create();
const page = doc.addPage([612, 792]);
const form = doc.getForm();
form.createTextField("name").addToPage(page, { x: 10, y: 700 });
form.createCheckBox("agree").addToPage(page, { x: 10, y: 660 });
const dd = form.createDropdown("country");
dd.addOptions(["USA", "UK", "Canada"]);
dd.addToPage(page, { x: 10, y: 620 });
const rg = form.createRadioGroup("size");
rg.addOptionToPage("small", page, { x: 10, y: 580 });
rg.addOptionToPage("medium", page, { x: 50, y: 580 });
rg.addOptionToPage("large", page, { x: 90, y: 580 });
formPdfBytes = await doc.save();
});
it("writes text, checkbox, dropdown, and radio (by label) in one pass", async () => {
const out = await buildAnnotatedPdfBytes(
formPdfBytes,
[],
new Map<string, string | boolean>([
["name", "Alice"],
["agree", true],
["country", "Canada"],
["size", "medium"],
]),
);
const form = (await PDFDocument.load(out)).getForm();
expect(form.getTextField("name").getText()).toBe("Alice");
expect(form.getCheckBox("agree").isChecked()).toBe(true);
expect(form.getDropdown("country").getSelected()).toEqual(["Canada"]);
expect(form.getRadioGroup("size").getSelected()).toBe("medium");
});
it("maps numeric radio buttonValue to option label by index", async () => {
// The viewer stores what pdf.js reports as buttonValue ("2"), not the
// label. Save must translate or the radio is silently dropped.
const out = await buildAnnotatedPdfBytes(
formPdfBytes,
[],
new Map<string, string | boolean>([["size", "2"]]),
);
const form = (await PDFDocument.load(out)).getForm();
expect(form.getRadioGroup("size").getSelected()).toBe("large");
});
it("leaves radio unset when value is neither label nor valid index", async () => {
const out = await buildAnnotatedPdfBytes(
formPdfBytes,
[],
new Map<string, string | boolean>([["size", "bogus"]]),
);
const form = (await PDFDocument.load(out)).getForm();
expect(form.getRadioGroup("size").getSelected()).toBeUndefined();
});
it("skips unknown field names without throwing", async () => {
const out = await buildAnnotatedPdfBytes(
formPdfBytes,
[],
new Map<string, string | boolean>([
["nonexistent", "x"],
["name", "kept"],
]),
);
const form = (await PDFDocument.load(out)).getForm();
expect(form.getTextField("name").getText()).toBe("kept");
});
});
});
// =============================================================================

View file

@ -18,6 +18,10 @@ import {
PDFString,
PDFHexString,
StandardFonts,
PDFTextField,
PDFCheckBox,
PDFDropdown,
PDFRadioGroup,
} from "pdf-lib";
// =============================================================================
@ -810,26 +814,45 @@ export async function buildAnnotatedPdfBytes(
// Add proper PDF annotation objects
await addAnnotationDicts(pdfDoc, annotations);
// Apply form fills
// Apply form fills. Dispatch on actual field type — getTextField(name) throws
// for dropdowns/radios, so the old try/catch silently dropped those on save.
if (formFields.size > 0) {
try {
const form = pdfDoc.getForm();
for (const [name, value] of formFields) {
try {
if (typeof value === "boolean") {
const checkbox = form.getCheckBox(name);
if (value) checkbox.check();
else checkbox.uncheck();
const field = form.getFieldMaybe(name);
if (!field) continue;
if (field instanceof PDFCheckBox) {
if (value) field.check();
else field.uncheck();
} else if (field instanceof PDFRadioGroup) {
// The viewer stores pdf.js's buttonValue, which for PDFs with an
// /Opt array is a numeric index ("0","1","2") rather than the
// option label pdf-lib's select() expects. Try the label first,
// then fall back to indexing into getOptions().
const opts = field.getOptions();
const s = String(value);
if (opts.includes(s)) {
field.select(s);
} else {
const textField = form.getTextField(name);
textField.setText(value);
const idx = Number(s);
if (Number.isInteger(idx) && idx >= 0 && idx < opts.length) {
field.select(opts[idx]);
}
// else: value is neither label nor index — leave unset
}
} catch {
// Field not found or wrong type — skip
} else if (field instanceof PDFDropdown) {
// select() auto-enables edit mode for values outside getOptions(),
// so this works for both enumerated and free-text combos.
field.select(String(value));
} else if (field instanceof PDFTextField) {
field.setText(String(value));
}
// PDFButton, PDFOptionList, PDFSignature: no fill_form support yet
}
} catch {
// Form not available — skip
// pdfDoc.getForm() throws if the PDF has no AcroForm
}
}