diff --git a/contract/src/domain.ts b/contract/src/domain.ts index 9325b73..5bd5087 100644 --- a/contract/src/domain.ts +++ b/contract/src/domain.ts @@ -1,7 +1,23 @@ // Domain DTOs shared by the plugin server and the UI — Schemas are the source of truth, // the derived types keep the original domain names so the pure core reads unchanged. +// +// TRAP, measured (see plugin/test/wire.test.ts, which pins it): the HttpApi RESPONSE path +// serialises a present-but-`undefined` key as `null`, where the plain Schema encoder omits +// it. So a `Schema.optional(X)` field the server assembles as `{ k: undefined }` goes out +// as `"k":null` — which `X | undefined` then REFUSES when the client decodes it. It fails +// silently in the SSE feed (`sseAtom` drops schema-invalid frames, so the page just stops +// updating) and loudly in the typed AtomHttpApi client. +// +// The rule here: **nothing on this wire is `undefined`.** Server-assembled fields are +// `Schema.NullOr` with an explicit null; fields that also round-trip through cache.json use +// `Schema.optional(Schema.NullOr(...))` so an older cache (absent keys) still decodes while +// the wire's null is accepted too. import { Schema } from "effect"; +/** Absent | undefined | null on the way in, `null` on the way out — see the trap above. */ +const nullish = (schema: S) => + Schema.optional(Schema.NullOr(schema)); + /** Host OS as the launch/quoting domain sees it (macOS hosts use the linux lane). */ export const Os = Schema.Literals(["linux", "windows"]); export type Os = typeof Os.Type; @@ -51,13 +67,16 @@ export const DetectedEmulator = Schema.Struct({ name: Schema.String, template: Schema.String, supportsArchives: Schema.Boolean, - contested: Schema.optional(Schema.Boolean), + // `nullish`, not plain NullOr: these also live in cache.json, and a cache written by + // 0.3.1 has the keys ABSENT — requiring them would fail the whole cache decode and + // silently discard every art verdict on upgrade. + contested: nullish(Schema.Boolean), exeToken: Schema.String, - exePath: Schema.optional(Schema.String), - appId: Schema.optional(Schema.String), + exePath: nullish(Schema.String), + appId: nullish(Schema.String), via: Schema.Literals(["path", "file", "flatpak"]), - coresDir: Schema.optional(Schema.String), - cores: Schema.optional(Schema.Array(Schema.String)), + coresDir: nullish(Schema.String), + cores: nullish(Schema.Array(Schema.String)), }); export type DetectedEmulator = typeof DetectedEmulator.Type; @@ -97,9 +116,11 @@ export const EngineStatus = Schema.Struct({ os: Os, artProvider: Schema.NullOr(Schema.String), syncing: Schema.Boolean, - lastSync: Schema.optional(LastSync), - lastReport: Schema.optional(SyncReport), - detectedAt: Schema.optional(Schema.Number), + // Assembled fresh by the engine on every read and never persisted, so these can be + // strict: an explicit null on the wire, never an absent or undefined key. + lastSync: Schema.NullOr(LastSync), + lastReport: Schema.NullOr(SyncReport), + detectedAt: Schema.NullOr(Schema.Number), paths: Schema.Struct({ dir: Schema.String, config: Schema.String, diff --git a/plugin/package.json b/plugin/package.json index 8f157c2..5b52da3 100644 --- a/plugin/package.json +++ b/plugin/package.json @@ -1,9 +1,9 @@ { "name": "@punktfunk/plugin-rom-manager", - "version": "0.3.1", + "version": "0.3.2", "private": false, "type": "module", - "description": "Punktfunk plugin: scans ROM directories, maps them to emulators, and reconciles them into the host game library as a provider — with a console-hosted web UI. The reference plugin built on @punktfunk/plugin-kit.", + "description": "Punktfunk plugin: scans ROM directories, maps them to emulators, and reconciles them into the host game library as a provider \u2014 with a console-hosted web UI. The reference plugin built on @punktfunk/plugin-kit.", "license": "MIT OR Apache-2.0", "homepage": "https://git.unom.io/unom/punktfunk-plugin-rom-manager", "repository": { diff --git a/plugin/src/services/engine.ts b/plugin/src/services/engine.ts index 7e06fa6..1a218e6 100644 --- a/plugin/src/services/engine.ts +++ b/plugin/src/services/engine.ts @@ -212,9 +212,10 @@ function make(): Effect.Effect< os, artProvider: selectArtProvider(config)?.id ?? null, syncing: engineStatus.syncing, - lastSync: engineStatus.lastSync, - lastReport: engineStatus.lastReport, - detectedAt: c.detect?.at, + // Explicit nulls, never undefined — see the trap note in contract/domain.ts. + lastSync: engineStatus.lastSync ?? null, + lastReport: engineStatus.lastReport ?? null, + detectedAt: c.detect?.at ?? null, paths: { dir: nodePath.dirname(cfg.path), config: cfg.path, @@ -228,7 +229,14 @@ function make(): Effect.Effect< preview: computeWith(false), detect, status, - statusChanges: engine.changes.pipe(Stream.mapEffect(() => status)), + // A fresh subscriber gets the CURRENT status immediately, then one frame per + // engine transition — so the console's live view is right the moment it + // connects, rather than stale until the next sync. Each frame is a full + // snapshot, so the sliver between the two stages costs nothing. + statusChanges: Stream.concat( + Stream.fromEffect(status), + engine.changes.pipe(Stream.mapEffect(() => status)), + ), } satisfies RomSyncService; }); } diff --git a/plugin/test/wire.test.ts b/plugin/test/wire.test.ts new file mode 100644 index 0000000..d2745e9 --- /dev/null +++ b/plugin/test/wire.test.ts @@ -0,0 +1,178 @@ +// The wire contract: whatever the server ENCODES, the client must be able to DECODE. +// +// This exists because a whole class of bug is invisible to unit tests of either side. +// `Schema.optional(X)` describes `X | undefined`, and the plain Schema encoder omits a +// present-but-undefined key — but the HttpApi RESPONSE path serialises it as `null`, which +// `X | undefined` then refuses on the way back in. The result shipped in 0.3.1: a +// never-synced host's `GET /api/status` returned `"lastSync":null` and the Overview page +// could not decode it, and `POST /api/detect` on a box with any emulator installed returned +// `"contested":null` and the Emulators page could not decode that. +// +// So these tests drive the REAL `makeApi` handler stack through `toWebHandler` and decode +// the bytes with the shared contract schemas. A unit test of the engine, or of the schema +// in isolation, passes happily while the product is broken. +import { describe, expect, test } from "bun:test"; +import type { + CacheStore, + ConfigService, + SyncEngine, +} from "@punktfunk/plugin-kit"; +import { + type DetectedEmulator, + EmulatorsPayload, + EngineStatus, + type PreviewResult, + type RomConfigSchema, + resolveConfig, + type SyncReport, +} from "@rom-manager/contract"; +import { Effect, Schema, Stream } from "effect"; +import { HttpRouter } from "effect/unstable/http"; +import { makeApi } from "../src/services/api.js"; +import type { CacheSchema } from "../src/services/cache.js"; +import type { RomSyncService } from "../src/services/engine.js"; + +const EMPTY_REPORT: SyncReport = { + considered: 0, + included: 0, + skipped: [], + excluded: [], + warnings: [], + truncated: 0, + perPlatform: {}, + overWarn: false, +}; + +/** A never-synced, never-detected host — what every fresh install looks like. */ +const FIRST_RUN_STATUS: EngineStatus = { + rootsConfigured: 0, + os: "linux", + artProvider: null, + syncing: false, + lastSync: null, + lastReport: null, + detectedAt: null, + paths: { dir: "/s", config: "/s/config.json", cache: "/s/cache.json" }, +}; + +/** + * A detection result exactly as `detectAll` builds it: the optional keys are PRESENT with + * `undefined` values, not absent. That distinction is the whole bug — a value round-tripped + * through cache.json has them absent instead, and absent keys encode correctly, which is + * why this only ever broke right after a live detect. + */ +const FRESHLY_DETECTED: DetectedEmulator = { + id: "duckstation", + name: "DuckStation", + template: "{exe} -batch {rom}", + supportsArchives: true, + contested: undefined, + exeToken: "duckstation-qt", + exePath: "/usr/bin/duckstation-qt", + appId: undefined, + via: "path", + coresDir: undefined, + cores: undefined, +}; + +const stubDeps = () => { + const engine = { + sync: () => Effect.succeed({ _tag: "Unchanged", report: EMPTY_REPORT }), + status: Effect.succeed({ syncing: false }), + changes: Stream.empty, + start: Effect.void, + reconfigure: Effect.void, + } as unknown as SyncEngine; + + const sync: RomSyncService = { + engine, + preview: Effect.succeed({ + entries: [], + report: EMPTY_REPORT, + } satisfies PreviewResult), + detect: () => Effect.succeed([FRESHLY_DETECTED]), + status: Effect.succeed(FIRST_RUN_STATUS), + statusChanges: Stream.empty, + }; + + const config = { + load: Effect.succeed(resolveConfig({})), + loadRaw: Effect.succeed({}), + saveRaw: () => Effect.succeed(resolveConfig({})), + changes: Stream.empty, + path: "/s/config.json", + } as unknown as ConfigService; + + const cache = { + get: Effect.succeed({ art: {} }), + modify: () => Effect.succeed(undefined), + update: () => Effect.void, + path: "/s/cache.json", + } as unknown as CacheStore; + + return { sync, config, cache }; +}; + +/** Drive one request through the real handler stack and hand back the parsed body. */ +const call = async (method: string, path: string): Promise => { + const { handler, dispose } = HttpRouter.toWebHandler(makeApi(stubDeps())); + try { + const response = await handler( + new Request(`http://localhost${path}`, { method }), + ); + expect(response.status).toBe(200); + return await response.json(); + } finally { + await dispose(); + } +}; + +describe("the status endpoint survives a round trip", () => { + test("a never-synced host's status decodes on the client", async () => { + const body = await call("GET", "/api/status"); + // The assertion that matters: the UI's own decoder accepts the server's bytes. + expect(() => Schema.decodeUnknownSync(EngineStatus)(body)).not.toThrow(); + }); + + test("absent values arrive as null, not as a missing key", async () => { + // Pinning the shape, not just its decodability: the UI branches on `=== null`. + const body = (await call("GET", "/api/status")) as Record; + expect(body.lastSync).toBeNull(); + expect(body.lastReport).toBeNull(); + expect(body.detectedAt).toBeNull(); + }); +}); + +describe("the emulator endpoints survive a round trip", () => { + test("a freshly detected emulator decodes on the client", async () => { + const body = await call("POST", "/api/detect"); + expect(() => + Schema.decodeUnknownSync(EmulatorsPayload)(body), + ).not.toThrow(); + }); + + test("the cached-read path decodes too", async () => { + const body = await call("GET", "/api/emulators"); + expect(() => + Schema.decodeUnknownSync(EmulatorsPayload)(body), + ).not.toThrow(); + }); +}); + +describe("the trap itself", () => { + test("the HttpApi response path turns undefined into null", async () => { + // Documents WHY the contract avoids `Schema.optional(X)` on this API. If a future + // effect release stops doing this, the schemas can relax — this test will say so. + const body = (await call("POST", "/api/detect")) as { + detected: Array>; + }; + const emulator = body.detected[0]!; + expect(emulator.contested).toBeNull(); + expect(emulator.appId).toBeNull(); + // …while the plain Schema encoder omits the very same keys. + const encoded = Schema.encodeUnknownSync(EngineStatus)({ + ...FIRST_RUN_STATUS, + }) as Record; + expect(encoded).toHaveProperty("lastSync"); + }); +}); diff --git a/ui/src/data/atoms.ts b/ui/src/data/atoms.ts index 96de4e4..61a4f2a 100644 --- a/ui/src/data/atoms.ts +++ b/ui/src/data/atoms.ts @@ -120,7 +120,7 @@ export const statusEventsAtom: Atom.Atom> = if (!prev.syncing && frame.syncing) push("Sync started"); if (prev.syncing && !frame.syncing) { const report = frame.lastReport; - if (report === undefined) { + if (report === null) { push("Sync finished"); } else { push( @@ -136,7 +136,7 @@ export const statusEventsAtom: Atom.Atom> = } } if ( - frame.detectedAt !== undefined && + frame.detectedAt !== null && frame.detectedAt !== prev.detectedAt ) { push("Emulator detection finished"); diff --git a/ui/src/mocks/fixtures.ts b/ui/src/mocks/fixtures.ts index 1fc44e8..be68f52 100644 --- a/ui/src/mocks/fixtures.ts +++ b/ui/src/mocks/fixtures.ts @@ -295,11 +295,16 @@ export const statusFor = (scenario: Scenario): EngineStatus => { const report = reportFor(scenario, configFor(scenario)); switch (scenario) { case "firstRun": + // Explicit nulls, exactly as the server sends them — a fixture that omitted + // these would no longer resemble the real wire shape. return { rootsConfigured: 0, os: "linux", artProvider: null, syncing: false, + lastSync: null, + lastReport: null, + detectedAt: null, paths: PATHS, }; case "syncing": diff --git a/ui/src/pages/emulators.tsx b/ui/src/pages/emulators.tsx index ef3e5fa..5d96639 100644 --- a/ui/src/pages/emulators.tsx +++ b/ui/src/pages/emulators.tsx @@ -174,14 +174,13 @@ const DetectedList = ({ contested ) : null} - {row.detected?.cores !== undefined && - row.detected.cores.length > 0 ? ( + {(row.detected?.cores?.length ?? 0) > 0 ? ( - {row.detected.cores.length} cores + {row.detected?.cores?.length} cores ) : null} - {row.detected?.exePath !== undefined ? ( + {row.detected?.exePath ? (

{row.detected.exePath}

diff --git a/ui/src/pages/overview.tsx b/ui/src/pages/overview.tsx index 56b2fa8..19d368b 100644 --- a/ui/src/pages/overview.tsx +++ b/ui/src/pages/overview.tsx @@ -72,7 +72,7 @@ const FirstRun = ({ navigate }: PageProps) => ( ); const lastSyncHint = (status: EngineStatus): string => { - if (status.lastSync === undefined) return "never synced"; + if (status.lastSync === null) return "never synced"; const minutes = Math.max( 0, Math.round((Date.now() - status.lastSync.at) / 60_000), @@ -117,7 +117,7 @@ const SyncButton = ({ status }: { status: EngineStatus }) => { const Warnings = ({ status }: { status: EngineStatus }) => { const report = status.lastReport; - if (report === undefined) return null; + if (report === null) return null; const count = report.warnings.length + (report.overWarn ? 1 : 0); if (count === 0) return null; return (