Publish Claude Artifact HTML as a Screenly Edge App

[salmanfarisvp]

Summary

• Stacked on Prepare Screenly MCP for Claude Desktop marketplace listing [T10708] #304 (feat/mcp-annotations-and-mcpb-bundle). This PR only adds the HTML → Edge App publish path.
• Adds edge_app_publish_from_html so Claude can turn Artifact / webpage HTML into a Screenly Edge App (screenly.js + theme CSS variables).
• Omit app_id to create; pass app_id to deploy a new revision (same as screenly edge-app deploy). Phrasing like “upload this as a Screenly app” maps to the same tool.

Test plan

• cargo check and cargo clippy -- -D warnings
• cargo test --bin screenly mcp::
• In Claude: “upload this as a Screenly app named Lobby Board”
• Ask Claude to update the same app and confirm a new revision

Demo

1. Use MCP to list Screenly players and stats

2. Ask Claude to create a website to visualize

3. Ask to create a Screenly App/Edge App based on the view

3.1 Edge App Installation is ready to be added to the playlist.

3.2 Apps running on Screenly anywhere

4. Ask to make some modifications

4.1 Modified app uploaded and running on Screenly anywhere.

[sergey-borovkov]

Nice feature, but there are two blockers that make edge_app_publish_from_html fail on essentially every real invocation, plus a set of heuristics that will misrender ordinary pages. Line numbers are for 4974e1e.

Blocking

1. src/mcp/tools/edge_app.rs:320 — the registry path collides with the token file.

registry_path() returns ~/.screenly/mcp-edge-apps.json, but ~/.screenly is a regular file holding the API token (src/authentication.rs:124 does fs::write(home.join(".screenly"), token), and read_token reads it back). For anyone who has run screenly login, save_registry's fs::create_dir_all(parent) at line 342 fails — I reproduced it: Err(Os { code: 17, kind: AlreadyExists, message: "File exists" }).

So remember_published_app fails on every call and the tool returns {"error":"Failed to create /home/<user>/.screenly: File exists"} even though the app was created, deployed and instanced. Symmetrically, load_registry sees path.exists() == false and always returns an empty registry, so the "remember by name" feature never works at all. Please pick a different location (~/.screenly.d/, ~/.config/screenly/, or dirs::data_dir()); the README, manifest and tool descriptions advertise the old path too.

2. src/mcp/tools/edge_app.rs:296 — local bookkeeping failure is fatal after irreversible remote work.

ensure_instance(...)? (line 294) and remember_published_app(...)? (line 296) run after create_in_place + deploy have already created the app and published a revision. Any error there discards the whole response, including app_id. Together with #1 this fires on every call: the caller sees a failure, retries with the same name, lookup_remembered_app returns None, and a brand-new Edge App is created — one orphan app plus one published revision per retry. These post-deploy steps should be best-effort (surface a warning in the payload) and the response should always carry app_id.

Should fix before merge

3. src/mcp/tools/edge_app.rs:352 — the name→app_id registry is not scoped to account or API host, and never self-heals. McpEdgeAppRegistry.apps is keyed by display name only. The same machine used against staging and prod (API_BASE_URL), or with two different API_TOKENs, resolves "Lobby Board" to an app id in the other account; deploy then fails with 404/permission and there's no way to clear the entry from the tool. Same for a name whose app was deleted server-side. Suggest keying on (api url, account, name) and falling back to creating a new app when the remembered id no longer resolves.

4. src/mcp/tools/edge_app.rs:91 — the auto-rotate selectors .page, .tile, .view are too generic. collectPages() returns the first selector match with length > 1, then showOnly hides all but one. A stat dashboard built from <div class="tile"> KPI cards — exactly the artifact shape in the demo — is detected as a slideshow: 5 of 6 tiles get hidden + display:none and the grid becomes a one-card-at-a-time carousel every 8s. Restrict to unambiguous markers ([role="tabpanel"], .carousel-item, [data-slide]) or require an explicit opt-in attribute.

5. src/mcp/tools/edge_app.rs:98 — the last-resort body * heuristic misfires on any page with a single hidden child. The loop returns kids for the first element having ≥1 hidden and ≥1 visible element child. A <template>, a hidden modal, an sr-only span or a display:none legend anywhere under <body> qualifies — so a page with <header>, <main>, <footer> and one hidden dialog gets its three real sections rotated one at a time, and the hidden dialog is shown as one of the "pages" (showOnly removes its hidden attribute at line 78). I'd drop this fallback.

6. src/mcp/tools/edge_app.rs:80 — showOnly clears inline display on the element it's trying to show. el.style.display = on ? "" : "none" resets the active item to its stylesheet value. With the common .carousel-item { display: none } pattern where the page's own JS sets inline display:block, the active item resolves back to display:none — every item hidden, blank screen. Cache and restore the original inline value instead of assigning "".

7. src/mcp/tools/edge_app.rs:117 — every <dialog> is force-opened. querySelectorAll("dialog").forEach(el => el.show()) opens confirmation/error modals the page deliberately keeps closed; on a screen they become permanent overlays with nobody there to dismiss them. Milder version of the same concern for details.open = true on line 116.

8. src/mcp/tools/edge_app.rs:437 (with :260) — the screenly.js substring check plus hardcoded ready_signal: true can leave the player permanently blank. Injection is skipped whenever the document merely contains the string screenly.js anywhere — a comment, a code sample, a CSP note — or on a <base href> page where the relative screenly.js?version=1 resolves off-origin. When that happens window.screenly is undefined, the bootstrap hits if (!window.screenly) return; at line 166 and never calls signalReadyForRendering(). Per docs/EdgeApps.md:360, if ready_signal is true and the function is never called, the content is not displayed. Either detect an actual <script src=…screenly.js…> tag, or only set ready_signal: true when the bootstrap was injected.

Minor

9. src/mcp/tools/edge_app.rs:268 — the manifest is hardcoded to screenly.yml, but deploy resolves it through transform_edge_app_path_to_manifest (src/commands/edge_app/utils.rs:75), which honours MANIFEST_FILE_NAME. A user with that generically-named env var exported gets InvalidManifest on every publish.

10. src/mcp/tools/edge_app.rs:383 — ensure_instance does rows.iter().find_map(|row| row.get("id")), taking whichever instance the API lists first, with no name match and no use of McpEdgeAppRecord.instance_id (stored but never read back). For an app with several instances the reported instance_id changes between runs and overwrites the remembered one.

11. src/mcp/server.rs:880 — edge_app_publish_from_html is a synchronous #[tool] fn running the full blocking deploy, including ensure_assets_processing_finished (src/commands/edge_app/app.rs:396), which thread::sleeps in a poll loop for up to MAX_WAIT_TIME = 1000 seconds. That parks a tokio worker for the whole duration; the existing read-only tools block only for a single request. tokio::task::spawn_blocking would keep the stdio server responsive.

cargo check --all-targets is clean — everything above is behavioural.

[salmanfarisvp]

@sergey-borovkov Thanks for the review: the blockers and the rest of the list are addressed in this branch.

Blocking

1. Registry path vs token file
Memory is no longer under ~/.screenly/ (that path is the login token file). It now lives at ~/.screenly.d/mcp-edge-apps.json. Tool copy / MCP instructions match.

2. Bookkeeping failure after deploy
ensure_instance and local remember run after create/deploy, but they no longer swallow the result. The payload always includes app_id, plus warnings / saved_to_memory if instance or disk write fails, so a retry can pass app_id instead of creating another app.

Should fix

3. Name cache scope / self-heal
The map is keyed by API host + SHA-256 of the token (not the raw token), then display name. If a GET for the remembered id returns empty, the entry is dropped and the next publish creates a new app. Network errors keep the cached id.

4–7. Signage wrap heuristics
Rotation is limited to [role="tabpanel"], .carousel-item, [data-slide], and [data-screenly-page]. The .tile / .page / .view selectors and the body * fallback are gone. showOnly stores and restores inline display (falls back to block). <dialog> / <details> are left alone.

8. screenly.js + ready_signal
Injection is skipped only when an opening <script src=…screenly.js…> tag exists, not when the string appears in a comment. ready_signal is true only when that script tag is present after wrap.

Minor

9. Temp app files use transform_edge_app_path_to_manifest / instance helpers, so MANIFEST_FILE_NAME / INSTANCE_FILE_NAME work.

10. Instance reuse prefers the remembered instance_id if it still exists, then a name match, then the sole instance — not whichever row the API listed first.

11. edge_app_publish_from_html is async and runs the blocking deploy on tokio::task::spawn_blocking.

Would you mind taking another look?

[sergey-borovkov]

Re-reviewed at a6aab8a. All 11 findings from the previous round are addressed, most with tests — cargo test mcp:: is 52/52 green and clippy is clean locally.

#	Fix
1	Registry moved to ~/.screenly.d/mcp-edge-apps.json with a SCREENLY_MCP_EDGE_APPS_PATH override; a test asserts it is not the token file
2	ensure_instance / remember_published_app now collect into warnings; app_id is always returned, with saved_to_memory and a fallback message
3	Registry keyed by `{api_url}
4, 5	Selectors cut to [role="tabpanel"], .carousel-item, [data-slide], [data-screenly-page]; the body * fallback is gone
6	showOnly caches the original inline display in data-screenly-orig-display
7	The dialog.show() / details.open block is removed
8	has_screenly_js_script parses real <script src=…> tags; ready_signal is derived from it
9	publish_dir_paths goes through transform_edge_app_path_to_manifest / …instance_manifest, honouring both env overrides
10	pick_existing_instance: remembered id → name match → sole instance
11	edge_app_publish_from_html is async + spawn_blocking; the manifest-parity test regex handles async fn

Three things left before I approve — one is just CI, the other two are minor:

1. The format check is red. cargo fmt --check on nightly (this repo's rustfmt.toml uses the unstable group_imports / imports_granularity) wants two changes in src/mcp/tools/edge_app.rs: the top imports collapse to use std::{env, fs};, and in registry_tests the use super::*; / use crate::authentication::Config; lines move below the external-crate imports. cargo +nightly fmt does both.

2. src/mcp/tools/edge_app.rs:88 — showOnly forces display:block when there was no inline display. orig is "" for an element styled only by the stylesheet, so (orig && orig !== "none") ? orig : "block" lands on block. A .carousel-item { display: flex } slide loses its flex layout the first time it is shown. Restoring orig and only forcing block when it stays hidden avoids that:

el.style.display = orig;
if (getComputedStyle(el).display === "none") el.style.display = "block";

3. src/mcp/tools/edge_app.rs:214 — ready_signal is effectively always true. After wrapping, has_screenly_js_script(&wrapped) is true in every path (the tag is either injected or already there), so the blank-screen case from the previous round survives in two narrow shapes: a document with <base href="…">, where the relative screenly.js?version=1 resolves off-origin, and one whose only <script src="…screenly.js"> sits inside an HTML comment. window.screenly is then undefined, the bootstrap returns at if (!window.screenly) return;, and per docs/EdgeApps.md:360 the content is never displayed. A cheap guard is to set ready_signal = false when the document contains a <base> tag.

Nice cleanup overall — the tests around the registry scoping and the manifest env overrides are exactly the right ones to have.

[salmanfarisvp]

@sergey-borovkov The three leftover items from your re-review at a6aab8a are in a5e34de.

1. Format check
   src/mcp/tools/edge_app.rs now matches nightly rustfmt (use std::{env, fs};, and crate imports in registry_tests sit below the external crates). cargo +nightly fmt --all --check is green locally.

2. showOnly / display:block
   It restores the original inline display, then sets block only if getComputedStyle is still none, so a .carousel-item { display: flex } slide keeps flex.

3. ready_signal always true
   ready_signal is now has_screenly_js_script && !html_has_base_tag. A document will not wait on a relative screenly.js that resolved off-origin. A <script src="…screenly.js"> inside an HTML comment is ignored, so we inject a real tag.

[sergey-borovkov]

Reviewed a5e34de. All three items from the last round are fixed, and CI is green (format passes, clippy clean, cargo test mcp:: is 54/54 locally).

• rustfmt — imports collapsed and reordered; cargo fmt --check on nightly is clean.
• showOnly — now el.style.display = orig with block forced only when the computed value stays none, so flex/grid slides keep their layout.
• ready_signal — ready_signal_for_html requires a real screenly.js script and no <base> tag, and the tag scanner skips <!-- --> regions. I probed it directly: uppercase <BASE HREF> is detected, a <base> inside a comment is ignored, a > inside an attribute value doesn't derail the scan, and an IE conditional comment is ignored (which matches what Chromium does anyway).

Approving. Three small things I noticed on this pass — all new, none blocking, happy for them to land in a follow-up:

1. src/mcp/tools/edge_app.rs:585 — a document with no explicit <head> fails the whole publish. <head> is optional in HTML, so <html><body><h1>hi</h1></body></html> is valid input, but has_html_shell is true and both injection attempts miss, giving Err("HTML document is missing a <head> element to inject screenly.js"). I confirmed this against the branch. An .or_else(|| inject_before_tag(&out, "<body", …)) fallback puts the script in the implied head.

2. src/mcp/tools/edge_app.rs:565 — has_html_shell also requires the closing </html>. A document that opens but never closes <html> (also legal) is treated as a fragment and nested inside a fresh shell — the output ends up with two <html tags and the inner <title> in the body. Keying on the opening <html or on <body> is enough.

3. src/mcp/tools/edge_app.rs:618 — the scanner skips comments but not inline <script> bodies. <script>var s='<script src="screenly.js"></script>';</script> reads as a real tag, so injection is skipped, window.screenly never exists, and ready_signal is still true → blank screen. Same class if a </body> literal appears in JS before the real one, which would park the bootstrap inside a string. Narrow enough to leave as is, but the doc comment ("inline script text that merely mention the filename do not count") currently promises a bit more than the code delivers.

Thanks for working through all of these carefully — the registry scoping, the self-healing stale-id lookup, and the tests around the manifest env overrides are all solid.