modelcontextprotocol/ext-apps
Watch
1
0
Fork
You've already forked ext-apps
 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

fix(pdf-server): viewer liveness, 1:1 batch results, fullscreen jitter (#579)

Browse source 
* 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): squash batch errors into one block, warn on phantom annotation updates

Three issues from a hung interact in production:

1. Batch error swallowed by host flattening. The earlier fix unshifted the
   error before successes, but the prefix went on top of THAT — so
   content[0] was just "Batch failed at step 2/2 (get_screenshot):" with
   the actual timeout text in content[1], which the host dropped. Now all
   text joins into one block; images survive after.

2. update_annotations silent no-op. The viewer's processCommands does
   `if (!existing) continue` for unknown ids. If a prior add_annotations
   failed mid-batch the model gets a happy "Queued" while nothing renders.
   Track ids server-side; warn (not error — user might have drawn the
   annotation manually in the iframe).

3. Per-def isolation in the viewer. A throw in addAnnotation killed the
   whole processCommands batch including any get_pages later in the queue,
   so submit_page_data never fired and the server waited the full 45s.
   Now each def gets its own try/catch.

Also: stderr logging on interact failure (only — not chatty per-call) so
the next hang shows step timing in mcp-server-*.log instead of nowhere.

* fix(pdf-server): images first on success too, not just on error

The host's content[0] flattening drops images on successful batches just
the same: [add_annotations, get_screenshot] → content = ["Queued: ...",
<image>] → model sees only the text. Observed in audit logs where the
exact same iframe + viewUUID returned an image when the batch was
[get_screenshot, get_screenshot] but only text when add_annotations
came first.

Stable-sort images to the front. The Queued lines still arrive, just
after what the model actually asked to see.

* Revert "fix(pdf-server): images first on success too, not just on error"

This reverts commit 129a2dcd7a04e5bd116e59c4d6fc211db4203e2e.

* refactor(pdf-server): 1:1 batch results, drop isError when there's something to lose

LocalAgentMode SDK collapses isError:true results to a bare string of
content[0].text. The squash-everything-into-one-block defense from
0dbb8ed3 worked but lost the positional contract — the model couldn't
tell which command produced which result.

Cleaner: content[i] is the result of commands[i]. If a step fails the
batch stops there with that slot's text starting with "ERROR", and
content.length tells the model how far it got. No isError on multi-step
failures — there's no way to set it without the SDK eating the prior
images. Single-command failures keep isError:true since there's nothing
before content[0] to lose; the SDK's flatten-to-content[0].text is
exactly the ERROR text we want shown.

Tool description documents the contract.

* fix(pdf-server): isError only for single-command failures, not failedAt===0

failedAt===0 in a multi-step batch still has a positional contract to
honor — content.length=1 vs commandList.length=3 is a signal the model
loses if the SDK flattens the array to a string. The ERROR text encodes
"step 1/3" but the simpler rule ("batches never set isError") is
easier to document and reason about.

* fix(pdf-viewer): read live displayMode in requestFitToContent, guard tiny measurements

currentDisplayMode defaults to "inline" and handleHostContextChanged
only writes it `if (ctx.displayMode)`. If the host's initial context
omits the field — or the update lands one tick after a renderPage() —
requestFitToContent measures a near-empty pageWrapper and sends ~85px
(toolbar + padding only), shrinking a fullscreen iframe to a sliver.
Workaround was exit + re-enter fullscreen to force a context update.

Read app.getHostContext().displayMode directly (always fresh), and
separately refuse to send when pageWrapper measures smaller than the
toolbar — that's never a real layout, just an early render.

* revert(pdf-server): drop viewAnnotationIds — wrong about source of truth

The viewer's annotationMap is baseline (loadBaselineAnnotations from the
PDF) + diff restored from localStorage + manual edits + model adds.
viewAnnotationIds only tracks the last category, so the warning fires
for any update to an annotation the model didn't itself add — including
the entire restored state.

That's not "best-effort" miss rate, it's structurally wrong about
where the data lives. Model would learn to ignore the warning. The
viewer-side log.error on the silent skip (mcp-app.ts:4251) is the right
layer for this.
This commit is contained in:
Olivier Chafik 2026-04-02 00:49:29 +01:00 • committed by GitHub
commit 800f6d08fd
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
3 changed files with 139 additions and 66 deletions
Download patch file Download diff file Expand all files Collapse all files

38
examples/pdf-server/server.test.ts
View file

@ -1444,12 +1444,12 @@ describe("interact tool", () => {
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.
it("batch step failure: 1:1 content, no isError, ERROR-prefixed slot", async () => {
// LocalAgentMode SDK 2.1.87 collapses isError:true results to a bare
// string of content[0].text — drops images from earlier successful
// steps. So we don't set isError on a step failure: each command gets
// one content slot, the failed one starts with "ERROR", and the batch
// stops there. content.length tells the model how far it got.
const { server, client } = await connect();
const uuid = "batch-error-ordering";
@ -1460,22 +1460,24 @@ describe("interact tool", () => {
commands: [
{ action: "navigate", page: 3 }, // succeeds → "Queued: ..."
{ action: "get_screenshot", page: 1 }, // never-polled → fast-fail
{ action: "navigate", page: 5 }, // never reached
],
},
});
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");
// Not isError — that would trigger the SDK flatten
expect(r.isError).toBeFalsy();
const content = r.content as Array<{ type: string; text?: string }>;
// Stopped at step 2; step 3 never ran
expect(content).toHaveLength(2);
// Slot 0: step 1's success, untouched
expect(content[0].text).toContain("Queued");
expect(content[0].text).not.toMatch(/^ERROR/);
// Slot 1: step 2's failure, ERROR-prefixed with the actual message
expect(content[1].text).toMatch(/^ERROR/);
expect(content[1].text).toContain("2/3");
expect(content[1].text).toContain("get_screenshot");
expect(content[1].text).toContain("never connected");
await client.close();
await server.close();

65
examples/pdf-server/server.ts
View file

@ -2254,7 +2254,7 @@ URL: ${normalized}`,
description: `Interact with a PDF viewer: annotate, navigate, search, extract text/screenshots, fill forms.
IMPORTANT: viewUUID must be the exact UUID returned by display_pdf (e.g. "a1b2c3d4-..."). Do NOT use arbitrary strings.
**BATCHING**: Send multiple commands in one call via \`commands\` array. Commands run sequentially. TIP: End with \`get_screenshot\` to verify your changes.
**BATCHING**: Send multiple commands in one call via \`commands\` array. Commands run sequentially; results are returned in the same order, one content item per command. If a command fails, the batch stops there and that command's slot contains text starting with \`ERROR\` — content.length tells you how far it got. TIP: End with \`get_screenshot\` to verify your changes.
**ANNOTATION** — add_annotations with array of annotation objects. Each needs: id (unique string), type, page (1-indexed).
@ -2449,9 +2449,21 @@ Example — add a signature image and a stamp, then screenshot to verify:
};
}
// Process commands sequentially, collecting all content parts
// 1:1 content array — content[i] is the result of commands[i].
//
// For multi-step batches we do NOT set isError on a step failure:
// LocalAgentMode SDK 2.1.87 collapses isError:true results to a
// bare string of content[0].text, which would drop any images
// from earlier successful steps. Instead, the failed step's slot
// is text starting with "ERROR", and the batch stops there — the
// model reads content.length to see how far it got.
//
// Single-command calls have no prior results to lose, so they
// keep isError:true. The SDK's flatten-to-content[0].text is
// exactly the ERROR text we want it to see.
const allContent: ContentPart[] = [];
let failedAt = -1;
const t0 = Date.now();
for (let i = 0; i < commandList.length; i++) {
const result = await processInteractCommand(
@ -2460,27 +2472,48 @@ Example — add a signature image and a stamp, then screenshot to verify:
extra.signal,
);
if (result.isError) {
// 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);
const errText = result.content
.map((c) => (c.type === "text" ? c.text : null))
.filter((t) => t != null)
.join(" — ");
// Normalize the prefix — processInteractCommand's catch blocks
// are inconsistent ("Error: ...", "add_annotations: ...", etc.)
const stripped = errText.replace(/^error:\s*/i, "");
allContent.push({
type: "text",
text:
commandList.length > 1
? `ERROR at step ${i + 1}/${commandList.length} (${commandList[i].action}): ${stripped}`
: `ERROR: ${stripped}`,
});
failedAt = i;
console.error(
`[interact] uuid=${uuid} step ${i + 1}/${commandList.length} ` +
`(${commandList[i].action}) failed after ${Date.now() - t0}ms: ${stripped}`,
);
break;
}
allContent.push(...result.content);
}
if (failedAt >= 0 && commandList.length > 1) {
allContent.unshift({
type: "text",
text: `Batch failed at step ${failedAt + 1}/${commandList.length} (${commandList[failedAt].action}):`,
});
// Squash multi-part successes (e.g. get_text returning multiple
// text blocks) into one slot so 1:1 indexing holds. Preserve a
// lone image as-is — that's the screenshot the model wants.
if (result.content.length === 1) {
allContent.push(result.content[0]);
} else {
const texts = result.content
.map((c) => (c.type === "text" ? c.text : null))
.filter((t) => t != null);
allContent.push({ type: "text", text: texts.join("\n") });
}
}
return {
content: allContent,
...(failedAt >= 0 ? { isError: true } : {}),
// isError flattens to a string in some SDKs, losing the array
// shape. Only set it when there was never going to be an array
// — single command, no positional contract to break.
...(failedAt >= 0 && commandList.length === 1
? { isError: true }
: {}),
};
},
);

102
examples/pdf-server/src/mcp-app.ts
View file

@ -287,8 +287,14 @@ async function computeFitToWidthScale(): Promise<number | null> {
* Only applies in inline mode - fullscreen mode uses scrolling.
*/
function requestFitToContent() {
if (currentDisplayMode === "fullscreen") {
return; // Fullscreen uses scrolling
// Read the host's current state, not our cached currentDisplayMode.
// currentDisplayMode defaults to "inline" and handleHostContextChanged
// only updates it `if (ctx.displayMode)` — if the host omits the field
// or the update lands one tick late, the cached value lies. We've seen
// this measure a near-empty pageWrapper (~85px = toolbar + padding) and
// shrink a fullscreen iframe to a sliver.
if (app.getHostContext()?.displayMode === "fullscreen") {
return;
}
const canvasHeight = canvasEl.height;
@ -324,6 +330,16 @@ function requestFitToContent() {
// In inline mode (this function early-returns for fullscreen) the side panel is hidden
const totalWidth = pageWrapperEl.offsetWidth + BUFFER;
// pageWrapper measuring ≈ 0 means the canvas hasn't laid out yet (early
// render, hidden ancestor, etc). Sending toolbar-height-only would shrink
// the iframe to a sliver. The next renderPage() will measure correctly.
if (pageWrapperHeight < toolbarHeight) {
log.info(
`requestFitToContent: pageWrapper ${pageWrapperHeight}px < toolbar ${toolbarHeight}px — skipping`,
);
return;
}
app.sendSizeChanged({ width: totalWidth, height: totalHeight });
}
@ -4229,42 +4245,64 @@ async function processCommands(commands: PdfCommand[]): Promise<void> {
}
break;
case "add_annotations":
// Per-def isolation. If convertFromModelCoords or addAnnotation throws
// for one def (bad shape, NaN coords), the rest still apply — and
// critically, a get_pages later in this batch still runs. Without
// this, a single bad annotation makes the whole batch throw out of
// processCommands, the iframe never reaches submit_page_data, and the
// server's interact() waits the full 45s for a reply that never comes.
for (const def of cmd.annotations) {
const pageHeight = await getPageHeight(def.page);
addAnnotation(convertFromModelCoords(def, pageHeight));
try {
const pageHeight = await getPageHeight(def.page);
addAnnotation(convertFromModelCoords(def, pageHeight));
} catch (err) {
log.error(`add_annotations: failed for id=${def.id}:`, err);
}
}
break;
case "update_annotations":
for (const update of cmd.annotations) {
const existing = annotationMap.get(update.id);
if (!existing) continue;
// The model sends model coords (y-down, y = top-left). existing.def
// is internal coords (y-up, y = bottom-left). For rect/circle/image
// the converted internal y = pageHeight - modelY - height — a function
// of BOTH y AND height. If the model patches only {height}, we must
// still rewrite internal y to keep the top fixed; otherwise the
// bottom stays fixed and the top shifts. Same coupling applies to
// {page} changes across differently-sized pages.
//
// Fix: round-trip through model space. Convert existing to model
// coords, spread the patch on top (all-model now), convert back.
// convertToModelCoords is self-inverse (pdf-annotations.ts:192) so
// unchanged fields pass through unmolested.
const srcPageH = await getPageHeight(existing.def.page);
const existingModel = convertToModelCoords(existing.def, srcPageH);
const mergedModel = {
...existingModel,
...update,
} as PdfAnnotationDef;
const dstPageH =
update.page != null && update.page !== existing.def.page
? await getPageHeight(update.page)
: srcPageH;
const mergedInternal = convertFromModelCoords(mergedModel, dstPageH);
// Pass the FULL merged def. updateAnnotation() already merges over
// the tracked def, so passing everything is correct and avoids the
// "only copy back Object.keys(update)" loop that caused the bug.
updateAnnotation(mergedInternal);
if (!existing) {
log.error(
`update_annotations: id=${update.id} not found — skipping`,
);
continue;
}
try {
// The model sends model coords (y-down, y = top-left). existing.def
// is internal coords (y-up, y = bottom-left). For rect/circle/image
// the converted internal y = pageHeight - modelY - height — a function
// of BOTH y AND height. If the model patches only {height}, we must
// still rewrite internal y to keep the top fixed; otherwise the
// bottom stays fixed and the top shifts. Same coupling applies to
// {page} changes across differently-sized pages.
//
// Fix: round-trip through model space. Convert existing to model
// coords, spread the patch on top (all-model now), convert back.
// convertToModelCoords is self-inverse (pdf-annotations.ts:192) so
// unchanged fields pass through unmolested.
const srcPageH = await getPageHeight(existing.def.page);
const existingModel = convertToModelCoords(existing.def, srcPageH);
const mergedModel = {
...existingModel,
...update,
} as PdfAnnotationDef;
const dstPageH =
update.page != null && update.page !== existing.def.page
? await getPageHeight(update.page)
: srcPageH;
const mergedInternal = convertFromModelCoords(
mergedModel,
dstPageH,
);
// Pass the FULL merged def. updateAnnotation() already merges over
// the tracked def, so passing everything is correct and avoids the
// "only copy back Object.keys(update)" loop that caused the bug.
updateAnnotation(mergedInternal);
} catch (err) {
log.error(`update_annotations: failed for id=${update.id}:`, err);
}
}
break;
case "remove_annotations":