fix: resolve MCP App compatibility issues with ChatGPT

[arcaputo3]

Summary

• Fix "Waiting for media..." stuck state by making ontoolinput work with partial args (media_type + filename only — size_bytes and mime_type are resolved from the first _get_media_data chunk)
• Add _meta.ui.prefersBorder to resource registration for proper widget rendering
• Set _get_media_data tool visibility to ["app"] so the model doesn't call it directly
• ontoolresult now acts as fallback if ontoolinput didn't fire

Test plan

• All 197 unit tests pass
• Ruff lint + format clean
• mypy passes
• Test in Claude Desktop: view_media renders media player
• Test in ChatGPT: widget loads and plays media via chunked fetch

🤖 Generated with Claude Code

[claude]

Code Review

Good targeted fixes for ChatGPT MCP App compatibility. The dual ontoolinput/ontoolresult pattern is the right approach and the implementation is clean. A few notes:

---

Correctness / Potential Issues

useCallback dependency on input.mime_type is stale-safe but slightly misleading

loadMedia lists input.mime_type as a dependency, but the actual mime type used for the final Blob is always resolved from the first chunk (resolvedMime = chunk.mime_type). The input's mime_type is only used as an initial value for the local resolvedMime variable but gets overwritten immediately on the first chunk regardless. This means the dependency doesn't trigger any meaningful re-render path and could be removed to simplify the deps array. Not a bug, but worth a comment or cleanup.

// resolvedMime from input is immediately overwritten at offset === 0 anyway
}, [app, input.filename, input.media_type]); // input.mime_type not needed

app.onerror silently swallows errors to console only

app.onerror = console.error;

Errors from the app SDK are only logged to the browser console. If onerror fires (e.g., transport-level failure before any chunks load), the UI will remain stuck on "Waiting for media..." with no visible feedback. Consider propagating to setErrorMsg:

app.onerror = (err) => {
  console.error(err);
  setErrorMsg(err instanceof Error ? err.message : String(err));
};

This is a minor UX issue but worth fixing given the PR's stated goal of eliminating the stuck state.

---

Performance

atob + manual loop is fine but verbose

The base64 → Uint8Array decoding pattern works correctly. For clarity, the one-liner form is equivalent:

const bytes = Uint8Array.from(atob(chunk.data), (c) => c.charCodeAt(0));

Not blocking, just a style note.

---

Security

meta={"ui": {"visibility": ["app"]}} on _get_media_data is a meaningful improvement — good call restricting this so the LLM can't be prompted to call it directly and exfiltrate arbitrary local file chunks.

_meta.ui.prefersBorder on the resource — correct placement. This should improve rendering in compliant hosts.

---

Minor

• The serve_media HTTP route comment says "Path traversal protection handled by storage backend" which is accurate for local storage (using validate_safe_path), but worth double-checking that the Databricks backend also validates filename — a raw path component from the URL is passed directly to storage.read().

• _get_media_data description starts with "Internal tool used by..." — this is good. Consider also adding it to descriptions.py for consistency with the rest of the codebase's pattern, though this is cosmetic.

---

Tests

The 197 passing unit tests are reassuring. The frontend React logic (ontoolinput/ontoolresult fallback) is the main untested path here — reasonable given the MCP App SDK's mocking complexity. A manual test in ChatGPT (as noted in the test plan) is the right verification approach for this one.

---

Overall: ✅ Approve with suggestions. The core fix is solid and the ontoolresult-as-fallback approach with prev ?? parsed is idiomatic React. The onerror gap is the only change I'd recommend before merging.

[claude]

PR Review: fix/mcp-app-chatgpt-compat

Overall this is a well-focused, well-reasoned fix. The root cause analysis is clear, the approach is sound, and the changes are minimal. A few observations below.

---

✅ What's good

ontoolinput / ontoolresult dual-path logic is a clean solution to the "Waiting for media..." stuck state. Using setMediaInput((prev) => prev ?? parsed) in ontoolresult correctly ensures it only fires as a true fallback — no race condition possible.

Making size_bytes/mime_type optional in MediaToolInput is the right fix. The interface now accurately reflects what's available at ontoolinput time vs. what gets resolved from the first chunk.

visibility: ["app"] on _get_media_data is good hygiene — it prevents the model from discovering and calling an internal tool it has no business calling directly.

prefersBorder: True meta on the resource — reasonable UI hint.

resolvedMime ?? "application/octet-stream" fallback on Blob construction is a safe improvement over the previous code which would have passed undefined as the MIME type.

---

🔸 Minor issues

1. mimeType state is set but never read

// media-viewer.tsx:95
const [mimeType, setMimeType] = useState<string | null>(input.mime_type ?? null);

setMimeType is called at line 136 (on first chunk), but mimeType is never used in the render output. The local resolvedMime variable does all the actual work for the Blob. This is dead state — worth removing to avoid confusion, or wiring it up to a display element if that was the intent.

2. Implicit type contract in ontoolresult

app.ontoolresult = async (result) => {
    const parsed = extractJson<MediaToolInput>(result);
    if (parsed?.filename && parsed?.media_type) {
        setMediaInput((prev) => prev ?? parsed);
    }
};

ontoolresult receives the result of view_media, not _get_media_data. The cast to MediaToolInput works because view_media happens to return fields matching that interface, but this is an implicit contract. It would be clearer to either define a separate ViewMediaResult type, or add a comment explaining why the view_media result is shaped like MediaToolInput. Low priority, but could bite a future contributor who refactors view_media's return shape.

3. useCallback dep array includes input.mime_type

}, [app, input.filename, input.media_type, input.mime_type]);

input.mime_type is now optional and almost always undefined at the time loadMedia runs (since ontoolinput doesn't provide it). Including it in the dep array is technically correct but means a re-fetch would trigger if the parent somehow re-renders MediaPlayer with a different mime_type. Since mime_type is now resolved from the server's first chunk rather than from the prop, removing it from the dep array might make the intent clearer — though leaving it is also safe.

---

🔍 The dist rebuild

The mcp-app.html diff shows 46 lines in / 46 out, which looks like a clean bundle rebuild of the TSX changes. No concerns there — this is the intended workflow per the project docs.

---

Summary

The core fix is correct and handles the ChatGPT compatibility issue well. The main actionable item before merge is cleaning up the unused mimeType state (item 1 above). Items 2 and 3 are minor and can be addressed in follow-up if desired.

🤖 Reviewed with Claude Code

[claude]

Code Review

Overall, this is a clean, well-targeted fix. The logic changes are sensible and the ChatGPT compatibility strategy is solid.

---

ontoolinput / ontoolresult fallback pattern (media-viewer.tsx)

The (prev) => prev ?? parsed idiom in ontoolresult is exactly right: idempotent and avoids overwriting state that ontoolinput already set.

loadMedia dependency array (media-viewer.tsx)

input.mime_type and input.size_bytes are intentionally excluded from the useCallback deps (resolved server-side on the first chunk), but a one-line comment explaining the omission would help future readers avoid a false "missing dep" reflex.

Base64 decoding (media-viewer.tsx)

The loop is correct. A slightly more idiomatic alternative: Uint8Array.from(atob(chunk.data), c => c.charCodeAt(0)) — style nit only. For 2 MB chunks on the main thread this is fine in modern browsers; on older WebViews (Safari) large atob calls can momentarily block the UI. Not a blocker, but worth tracking.

mimeType state vs. resolvedMime local var (media-viewer.tsx)

Correctly uses resolvedMime (local) when constructing the Blob rather than async state — avoids a stale-closure bug.

Retry / reload button (media-viewer.tsx)

When errorMsg is set there is no way to retry without a page refresh. loadMedia is already a stable useCallback, so a Retry button wired to it would be a small UX win. Out of scope here, but worth a follow-up.

---

meta={"ui": {"prefersBorder": True}} on resource (server.py) — looks correct.

_get_media_data visibility hint (server.py)

Good defensive practice. Worth a short comment noting this is a hint — clients that do not understand the field will still expose the tool to the model, so the _ prefix and description text remain the primary guards.

---

read_range protocol coverage (media_viewer.py — unchanged, but related)

get_media_data calls storage.read_range(), which is not listed in the StorageBackend protocol documented in CLAUDE.md. Either the docs are stale or one or more backends (e.g. Databricks) may be missing the implementation. A missing method surfaces as AttributeError at runtime rather than a clear import-time error. Recommend verifying all StorageBackend implementations include read_range and adding it to the protocol interface explicitly if it is not already there.

---

.gitignore — adding .claude/settings.json and media/ are both sensible. No concerns.

dist/mcp-app.html — minified bundle diff is only variable-name reassignments from the minifier (pt to ut, etc.), expected after a rebuild. Fine.

---

Area	Status
ontoolinput partial-args fix	Correct
ontoolresult fallback	Idempotent, correct
prefersBorder meta	Good
_get_media_data visibility hint	Good (hint only)
read_range protocol coverage	Worth verifying before merge
Base64 decode	Correct, minor style nit
Retry UX on error	Follow-up suggestion

The read_range protocol coverage is the one item worth confirming before merge. Everything else looks good.