feat(apps): AppRenderer with imperative ref (#1262)

[cliffhall]

Summary

• New element clients/web/src/components/elements/AppRenderer/ — a thin host-side shell for embedded MCP Apps. Owns the iframe and the AppBridge lifecycle; delegates bridge construction to a bridgeFactory callback so the wiring layer can supply the active MCP Client, Implementation, and host capabilities.
• Imperative ref (AppRendererHandle) exposes sendToolInput, sendToolResult, sendToolCancelled, and teardown so the wiring layer drives the bridge without prop drilling.
• Storybook stories cover Loading, Loaded (mock factory), Error, and Maximized states.
• Vitest suite (16 tests) covers ref forwarding, factory invocation with the iframe element, error wrapping, idempotent teardown, sandbox-path-driven re-creation, post-unmount disposal, and best-effort cleanup when teardownResource or close rejects.
• Coverage on the new file: 100% lines, 98.21% statements, 100% functions, 90.9% branches — exceeds the per-file gate.

Closes #1262.

Test plan

• npm run validate from clients/web/ (format, lint, build, full coverage suite — 634 tests pass)
• Visual check in Storybook once the wiring layer lands
• Manual smoke test against a real MCP App once Phase 3 wiring is in place

🤖 Generated with Claude Code

[cliffhall]

@claude review

[claude]

Claude Code is working…

I'll analyze this and get back to you.

View job run

[claude]

Claude finished @cliffhall's task in 1m 53s —— View job

---

Code Review — AppRenderer (#1262)

• Gather context and understand the request
• Read changed files
• Analyze implementation quality
• Review tests and stories
• Post comprehensive review feedback

---

Overall this is a clean, well-thought-out component. The lifecycle management is solid and the test suite is thorough. A few things worth discussing:

---

AppRenderer.tsx

1. bridgeFactory in effect deps — stability risk

bridgeFactory is in the effect deps array ([bridgeFactory, sandboxPath] — line 94). If the caller creates the factory inline or fails to wrap it in useCallback, the bridge will be torn down and recreated on every render. The component offers no protection against this. Worth either:

• Adding a // MUST be stable (wrap with useCallback) JSDoc on BridgeFactory, or
• Accepting it via a ref inside the component so instability can't accidentally cause bridge churn.

2. Teardown race between imperative teardown() and unmount cleanup (AppRenderer.tsx:114–120)

teardownStartedRef guards against calling the imperative handle twice, but it doesn't guard against a concurrent unmount. The race:

1. Caller calls ref.current.teardown() — flag is set, disposeBridge starts awaiting teardownResource
2. Component unmounts before disposeBridge resolves — cleanup effect runs, reads bridgeRef.current (still non-null), calls disposeBridge a second time

bridgeRef.current is only nulled after await disposeBridge returns (line 119), so an unmount mid-teardown can trigger a double dispose. Both teardownResource and close are swallowed in disposeBridge, so it probably doesn't crash in practice, but it's an undocumented footgun.

Simplest fix: null bridgeRef.current synchronously at the top of teardown() before the await:

async teardown() {
  const bridge = bridgeRef.current;
  if (!bridge || teardownStartedRef.current) return;
  teardownStartedRef.current = true;
  bridgeRef.current = null; // ← prevents cleanup double-dispose
  await disposeBridge(bridge);
},

3. No sandbox attribute on the iframe (AppRenderer.tsx:125–136)

The component is specifically designed to host external MCP App sandboxes. Without a sandbox attribute the embedded page inherits same-origin access to window.parent. Even for trusted MCP content, a sandbox="allow-scripts allow-same-origin" (or similar) would meaningfully reduce the blast radius of a malicious or buggy app. If the allow-same-origin flag is intentionally required for postMessage bridging, that's worth a comment. If not, consider sandbox="allow-scripts".

You could expose a sandbox prop with a sensible default, or hard-code a permissive-but-bounded set. Either way, the current unconstrained iframe is worth revisiting before Phase 3 wiring lands.

4. useImperativeHandle empty deps — correct (line 122)

The empty dep array is intentional and correct here — all bridge state is read through refs at call time, so the handle object never needs to be re-created.

---

AppRenderer.stories.tsx

5. Error export name shadows the global (line 94)

export const Error: Story = { ... };

This shadows globalThis.Error. Storybook will show it as "Error" in the sidebar regardless of the export name, so renaming to BridgeError or ErrorState costs nothing while avoiding the shadowing and any potential ESLint no-shadow warning. Fix this →

---

AppRenderer.test.tsx

6. flushAsync is subtly fragile (lines 41–46)

async function flushAsync(): Promise<void> {
  await act(async () => {
    await Promise.resolve();
    await Promise.resolve();
  });
}

Two microtask ticks flush Promise.resolve(bridgeFactory(iframe)).then(...) — correct for the current implementation. But if bridgeFactory itself is async (a real scenario), this won't flush far enough. The tests that use a real async factory (pendingFactory tests, disposes a bridge that resolves after unmounts) handle this correctly by flushing inline, so it's only a risk for future tests that use flushAsync with a multi-hop factory. A comment explaining why two ticks are sufficient for a sync factory would prevent the next person from either removing a tick or being confused by it.

7. Double-teardown test doesn't cover the unmount-while-tearing-down case

The idempotency test (line 151) calls teardown() twice in sequence. It doesn't cover the concurrent unmount case from point #2 above. If the race is fixed, adding a test for it would be worthwhile.

8. Test coverage: excellent — 16 scenarios, all the important edge cases (cancelled in-flight, post-unmount resolution, swallowed errors). No gaps beyond the race noted above.

---

Summary

Severity	Item
Medium	iframe missing sandbox attribute — worth a design decision before wiring lands
Low	teardown + unmount double-dispose race — benign now, worth fixing
Low	bridgeFactory stability not documented or enforced
Nit	Error story name shadows global
Nit	flushAsync missing comment on microtask count
Branch