|
| 1 | +# Reviewing this PR |
| 2 | + |
| 3 | +110 files changed, but **~90 of them are mechanical**. This guide maps what to read vs skim. |
| 4 | + |
| 5 | +## If you have 10 minutes |
| 6 | + |
| 7 | +1. [`BREAKING.md`](./BREAKING.md) — the public-surface delta. This is the contract. |
| 8 | +2. [`src/app.ts`](./src/app.ts) — `class App` declaration (search for `class App extends EventDispatcher`). The whole composition pattern is visible in the first ~50 lines of the class body: `readonly client: Client`, `readonly ui: ExtensionHandle<...>`, then `ui.setRequestHandler(...)` calls in the constructor. |
| 9 | +3. [`src/app-bridge.ts`](./src/app-bridge.ts) — same, search for `class AppBridge extends EventDispatcher`. Note `readonly server: Server` and the `server.setRequestHandler('tools/call', ...)` proxy wiring alongside `ui.*` for `ui/*` methods. |
| 10 | +4. Skim the test diff: `src/app-bridge.test.ts` — most of the file is unchanged. That's the point. |
| 11 | + |
| 12 | +## File map |
| 13 | + |
| 14 | +### Read carefully (~7 files, the actual design) |
| 15 | + |
| 16 | +| File | What changed | Why it matters | |
| 17 | +|---|---|---| |
| 18 | +| `BREAKING.md` | new, 94 lines | The contract. What consumers must change. | |
| 19 | +| `src/app.ts` | rewrite, −1,595 lines net | `App` now composes `Client` + `ExtensionHandle`. The 5 `assert*Capability` no-op overrides are gone. | |
| 20 | +| `src/app-bridge.ts` | rewrite, −2,063 lines net | `AppBridge` composes `Server` + `ExtensionHandle`. `tools/call` etc. proxy via standard `setRequestHandler`. | |
| 21 | +| `src/events.ts` | −178 lines | `ProtocolWithEvents` (subclass of SDK `Protocol`) → standalone `EventDispatcher<EventMap>`. No SDK coupling. | |
| 22 | +| `src/sdk-compat.ts` | new, 102 lines | `z.custom<T>` pass-through schemas for spec types v2 doesn't export validators for. See the comment block — this shrinks once typescript-sdk#1887 lands. | |
| 23 | +| `src/message-transport.ts` | −59 lines | `PostMessageTransport` on the v2 `Transport` interface. | |
| 24 | +| `src/server/index.ts` | −431 lines | `registerAppTool`/`registerAppResource` on v2 `McpServer`. | |
| 25 | + |
| 26 | +### Skim (mechanical, verify pattern then move on) |
| 27 | + |
| 28 | +| Area | Files | What changed | |
| 29 | +|---|---|---| |
| 30 | +| `src/spec.types.ts`, `src/types.ts` | 2 | Import paths v1→v2; `interface McpUi*Capabilities` → `type` (for `JSONObject` constraint). | |
| 31 | +| `src/generated/{schema.ts,schema.json,schema.test.ts}` | 3 | Auto-generated; shrinks because it stops composing v1 SDK schemas. Don't review line-by-line. | |
| 32 | +| `src/app-bridge.test.ts` | 1 | 73/88 v1 tests pass unmodified. Diff shows: 2 deleted (removed `ProtocolWithEvents` surface), ~10 with shallow setup tweaks, 1 skip. Final: 85 pass / 1 skip / 0 fail. | |
| 33 | +| `src/react/useApp.tsx`, `src/server/index.test.ts`, `src/*.examples.ts` | 5 | Import-path changes only. | |
| 34 | +| `examples/**` | 81 files across 25 workspaces | Per-example: `package.json` deps, `main.ts` import path (`StdioServerTransport` from `/stdio`), `vite.config.ts` external removal, `server.ts` v2 `McpServer` shape. Read one (`examples/basic-server-react/`) to see the pattern, skip the rest. | |
| 35 | +| `docs/patterns.tsx`, `docs/quickstart.md`, `scripts/generate-schemas.ts` | 3 | Import paths; one schema-gen path fix. | |
| 36 | +| `package.json`, `build.bun.ts`, `tsconfig.json`, `package-lock.json` | 4 | Dep swap; `tsconfig` exclusions for unported `*.examples.ts` (see "Known gaps" below). | |
| 37 | + |
| 38 | +## Recommended reading order (by commit) |
| 39 | + |
| 40 | +The 23 commits are intentionally staged. Read bottom-up in groups: |
| 41 | + |
| 42 | +**1. Setup (skim):** `231251a6` deps swap, `b50315b1` types/schemas, `a9713e3d` EventDispatcher + transport. |
| 43 | + |
| 44 | +**2. The core rewrite (read):** `25da6658` App, `a1f1f016` AppBridge, `a5378733` react/server. This is where the design lives. |
| 45 | + |
| 46 | +**3. Making it compile (skim):** `ea3f4af5`..`a97206ca` — JSDoc fixes, casts, build config. Noise. |
| 47 | + |
| 48 | +**4. v1 parity fixes (read commit messages):** `87fee346`, `f7490c6b`, `47f4cdf3`, `be67283e` — each is "test X failed, here's the behavior the port missed." These are the gaps the test suite caught. |
| 49 | + |
| 50 | +**5. Tests + examples (skim):** `7e8ae100`, `cce8436b`, `570781e0`, `9998758b`, `4298d34a`, `b845fd7c`. |
| 51 | + |
| 52 | +**6. Upstream-fix follow-through (skim):** `953a5d81` workaround → `4e26407d` revert (typescript-sdk#1871 fixed it), `76b3ba7a` `/stdio` import. |
| 53 | + |
| 54 | +## Questions to evaluate |
| 55 | + |
| 56 | +- **Does composition cover what subclassing did?** Compare `BREAKING.md`'s "Removed inherited member" table against your mental model of what consumers actually use. The bet is: `app.request(...)` etc. were leaked implementation, not documented API. |
| 57 | +- **Is the `ui/*` ↔ standard-method split clean?** In `app-bridge.ts`, find where `ui.setRequestHandler(...)` and `server.setRequestHandler('tools/call', ...)` coexist. That's the SEP-1865 mandate (iframe = Client, host = Server proxy) made literal. |
| 58 | +- **Is `sdk-compat.ts` acceptable?** It's the one place the port admits friction. The file's own header comment explains the trade-off; typescript-sdk#1887 (type predicates) is the planned shrink. |
| 59 | +- **Wire compat:** v2 `AppBridge` keeps a `ui/initialize` handler for v1-iframe back-compat. Is that the right choice vs. a clean break? |
| 60 | + |
| 61 | +## Known gaps (intentional, documented) |
| 62 | + |
| 63 | +- `npm run test:full` includes `examples/pdf-server/server.test.ts` (~33 fails) — heavy example, deeper port out of scope. |
| 64 | +- `tsconfig.json` excludes `src/**/*.examples.ts(x)` and `docs/` — companion JSDoc snippets use removed `app.request` etc. in region bodies. Mechanical port pending. |
| 65 | +- 18 non-basic example workspaces had import paths sed-fixed but builds untested. |
| 66 | +- CI is red until SDK deps publish — `package.json` points at `file:/tmp/*.tgz`. |
0 commit comments