fix: the API emitted undefined optionals as null, and the UI couldn't decode them (v0.3.2)
`Schema.optional(X)` means `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 when the client decodes it. Two
user-visible failures shipped in 0.3.1:
- GET /api/status on a never-synced host returned `"lastSync":null`, so the
Overview page could not decode its own status — i.e. every fresh install
until the first successful sync.
- POST /api/detect on a box with any emulator installed returned
`"contested":null` / `"appId":null`, so the Emulators page's Detect button
could not decode the result. It only looked fine because a value that has
round-tripped through cache.json has those keys ABSENT rather than present
-and-undefined, and absent keys encode correctly.
Nothing on this wire is `undefined` any more. EngineStatus.{lastSync,lastReport,
detectedAt} are `Schema.NullOr` and the engine sends explicit nulls. The
DetectedEmulator fields use `Schema.optional(Schema.NullOr(...))` instead —
tolerant of absent, undefined and null — because they also live in cache.json,
and requiring them would fail the whole cache decode on upgrade and silently
discard every art verdict.
plugin/test/wire.test.ts pins it: it drives the real makeApi handler stack
through toWebHandler and decodes the bytes with the shared contract schemas.
Against the old schema all five of its assertions fail; a unit test of either
side in isolation passes while the product is broken.
Also: the SSE feed now emits the current status on subscribe rather than only on
the next engine transition, so a freshly-opened console page is correct
immediately instead of stale until something happens.
This commit is contained in:
+29
-8
@@ -1,7 +1,23 @@
|
|||||||
// Domain DTOs shared by the plugin server and the UI — Schemas are the source of truth,
|
// 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.
|
// 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";
|
import { Schema } from "effect";
|
||||||
|
|
||||||
|
/** Absent | undefined | null on the way in, `null` on the way out — see the trap above. */
|
||||||
|
const nullish = <S extends Schema.Top>(schema: S) =>
|
||||||
|
Schema.optional(Schema.NullOr(schema));
|
||||||
|
|
||||||
/** Host OS as the launch/quoting domain sees it (macOS hosts use the linux lane). */
|
/** Host OS as the launch/quoting domain sees it (macOS hosts use the linux lane). */
|
||||||
export const Os = Schema.Literals(["linux", "windows"]);
|
export const Os = Schema.Literals(["linux", "windows"]);
|
||||||
export type Os = typeof Os.Type;
|
export type Os = typeof Os.Type;
|
||||||
@@ -51,13 +67,16 @@ export const DetectedEmulator = Schema.Struct({
|
|||||||
name: Schema.String,
|
name: Schema.String,
|
||||||
template: Schema.String,
|
template: Schema.String,
|
||||||
supportsArchives: Schema.Boolean,
|
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,
|
exeToken: Schema.String,
|
||||||
exePath: Schema.optional(Schema.String),
|
exePath: nullish(Schema.String),
|
||||||
appId: Schema.optional(Schema.String),
|
appId: nullish(Schema.String),
|
||||||
via: Schema.Literals(["path", "file", "flatpak"]),
|
via: Schema.Literals(["path", "file", "flatpak"]),
|
||||||
coresDir: Schema.optional(Schema.String),
|
coresDir: nullish(Schema.String),
|
||||||
cores: Schema.optional(Schema.Array(Schema.String)),
|
cores: nullish(Schema.Array(Schema.String)),
|
||||||
});
|
});
|
||||||
export type DetectedEmulator = typeof DetectedEmulator.Type;
|
export type DetectedEmulator = typeof DetectedEmulator.Type;
|
||||||
|
|
||||||
@@ -97,9 +116,11 @@ export const EngineStatus = Schema.Struct({
|
|||||||
os: Os,
|
os: Os,
|
||||||
artProvider: Schema.NullOr(Schema.String),
|
artProvider: Schema.NullOr(Schema.String),
|
||||||
syncing: Schema.Boolean,
|
syncing: Schema.Boolean,
|
||||||
lastSync: Schema.optional(LastSync),
|
// Assembled fresh by the engine on every read and never persisted, so these can be
|
||||||
lastReport: Schema.optional(SyncReport),
|
// strict: an explicit null on the wire, never an absent or undefined key.
|
||||||
detectedAt: Schema.optional(Schema.Number),
|
lastSync: Schema.NullOr(LastSync),
|
||||||
|
lastReport: Schema.NullOr(SyncReport),
|
||||||
|
detectedAt: Schema.NullOr(Schema.Number),
|
||||||
paths: Schema.Struct({
|
paths: Schema.Struct({
|
||||||
dir: Schema.String,
|
dir: Schema.String,
|
||||||
config: Schema.String,
|
config: Schema.String,
|
||||||
|
|||||||
+2
-2
@@ -1,9 +1,9 @@
|
|||||||
{
|
{
|
||||||
"name": "@punktfunk/plugin-rom-manager",
|
"name": "@punktfunk/plugin-rom-manager",
|
||||||
"version": "0.3.1",
|
"version": "0.3.2",
|
||||||
"private": false,
|
"private": false,
|
||||||
"type": "module",
|
"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",
|
"license": "MIT OR Apache-2.0",
|
||||||
"homepage": "https://git.unom.io/unom/punktfunk-plugin-rom-manager",
|
"homepage": "https://git.unom.io/unom/punktfunk-plugin-rom-manager",
|
||||||
"repository": {
|
"repository": {
|
||||||
|
|||||||
@@ -212,9 +212,10 @@ function make(): Effect.Effect<
|
|||||||
os,
|
os,
|
||||||
artProvider: selectArtProvider(config)?.id ?? null,
|
artProvider: selectArtProvider(config)?.id ?? null,
|
||||||
syncing: engineStatus.syncing,
|
syncing: engineStatus.syncing,
|
||||||
lastSync: engineStatus.lastSync,
|
// Explicit nulls, never undefined — see the trap note in contract/domain.ts.
|
||||||
lastReport: engineStatus.lastReport,
|
lastSync: engineStatus.lastSync ?? null,
|
||||||
detectedAt: c.detect?.at,
|
lastReport: engineStatus.lastReport ?? null,
|
||||||
|
detectedAt: c.detect?.at ?? null,
|
||||||
paths: {
|
paths: {
|
||||||
dir: nodePath.dirname(cfg.path),
|
dir: nodePath.dirname(cfg.path),
|
||||||
config: cfg.path,
|
config: cfg.path,
|
||||||
@@ -228,7 +229,14 @@ function make(): Effect.Effect<
|
|||||||
preview: computeWith(false),
|
preview: computeWith(false),
|
||||||
detect,
|
detect,
|
||||||
status,
|
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;
|
} satisfies RomSyncService;
|
||||||
});
|
});
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -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<SyncReport>;
|
||||||
|
|
||||||
|
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<typeof RomConfigSchema>;
|
||||||
|
|
||||||
|
const cache = {
|
||||||
|
get: Effect.succeed({ art: {} }),
|
||||||
|
modify: () => Effect.succeed(undefined),
|
||||||
|
update: () => Effect.void,
|
||||||
|
path: "/s/cache.json",
|
||||||
|
} as unknown as CacheStore<typeof CacheSchema>;
|
||||||
|
|
||||||
|
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<unknown> => {
|
||||||
|
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<string, unknown>;
|
||||||
|
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<Record<string, unknown>>;
|
||||||
|
};
|
||||||
|
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<string, unknown>;
|
||||||
|
expect(encoded).toHaveProperty("lastSync");
|
||||||
|
});
|
||||||
|
});
|
||||||
@@ -120,7 +120,7 @@ export const statusEventsAtom: Atom.Atom<ReadonlyArray<StatusEvent>> =
|
|||||||
if (!prev.syncing && frame.syncing) push("Sync started");
|
if (!prev.syncing && frame.syncing) push("Sync started");
|
||||||
if (prev.syncing && !frame.syncing) {
|
if (prev.syncing && !frame.syncing) {
|
||||||
const report = frame.lastReport;
|
const report = frame.lastReport;
|
||||||
if (report === undefined) {
|
if (report === null) {
|
||||||
push("Sync finished");
|
push("Sync finished");
|
||||||
} else {
|
} else {
|
||||||
push(
|
push(
|
||||||
@@ -136,7 +136,7 @@ export const statusEventsAtom: Atom.Atom<ReadonlyArray<StatusEvent>> =
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
if (
|
if (
|
||||||
frame.detectedAt !== undefined &&
|
frame.detectedAt !== null &&
|
||||||
frame.detectedAt !== prev.detectedAt
|
frame.detectedAt !== prev.detectedAt
|
||||||
) {
|
) {
|
||||||
push("Emulator detection finished");
|
push("Emulator detection finished");
|
||||||
|
|||||||
@@ -295,11 +295,16 @@ export const statusFor = (scenario: Scenario): EngineStatus => {
|
|||||||
const report = reportFor(scenario, configFor(scenario));
|
const report = reportFor(scenario, configFor(scenario));
|
||||||
switch (scenario) {
|
switch (scenario) {
|
||||||
case "firstRun":
|
case "firstRun":
|
||||||
|
// Explicit nulls, exactly as the server sends them — a fixture that omitted
|
||||||
|
// these would no longer resemble the real wire shape.
|
||||||
return {
|
return {
|
||||||
rootsConfigured: 0,
|
rootsConfigured: 0,
|
||||||
os: "linux",
|
os: "linux",
|
||||||
artProvider: null,
|
artProvider: null,
|
||||||
syncing: false,
|
syncing: false,
|
||||||
|
lastSync: null,
|
||||||
|
lastReport: null,
|
||||||
|
detectedAt: null,
|
||||||
paths: PATHS,
|
paths: PATHS,
|
||||||
};
|
};
|
||||||
case "syncing":
|
case "syncing":
|
||||||
|
|||||||
@@ -174,14 +174,13 @@ const DetectedList = ({
|
|||||||
contested
|
contested
|
||||||
</Badge>
|
</Badge>
|
||||||
) : null}
|
) : null}
|
||||||
{row.detected?.cores !== undefined &&
|
{(row.detected?.cores?.length ?? 0) > 0 ? (
|
||||||
row.detected.cores.length > 0 ? (
|
|
||||||
<Badge variant="outline" size="sm">
|
<Badge variant="outline" size="sm">
|
||||||
{row.detected.cores.length} cores
|
{row.detected?.cores?.length} cores
|
||||||
</Badge>
|
</Badge>
|
||||||
) : null}
|
) : null}
|
||||||
</div>
|
</div>
|
||||||
{row.detected?.exePath !== undefined ? (
|
{row.detected?.exePath ? (
|
||||||
<p className="mt-0.5 truncate font-mono text-xs text-muted-foreground">
|
<p className="mt-0.5 truncate font-mono text-xs text-muted-foreground">
|
||||||
{row.detected.exePath}
|
{row.detected.exePath}
|
||||||
</p>
|
</p>
|
||||||
|
|||||||
@@ -72,7 +72,7 @@ const FirstRun = ({ navigate }: PageProps) => (
|
|||||||
);
|
);
|
||||||
|
|
||||||
const lastSyncHint = (status: EngineStatus): string => {
|
const lastSyncHint = (status: EngineStatus): string => {
|
||||||
if (status.lastSync === undefined) return "never synced";
|
if (status.lastSync === null) return "never synced";
|
||||||
const minutes = Math.max(
|
const minutes = Math.max(
|
||||||
0,
|
0,
|
||||||
Math.round((Date.now() - status.lastSync.at) / 60_000),
|
Math.round((Date.now() - status.lastSync.at) / 60_000),
|
||||||
@@ -117,7 +117,7 @@ const SyncButton = ({ status }: { status: EngineStatus }) => {
|
|||||||
|
|
||||||
const Warnings = ({ status }: { status: EngineStatus }) => {
|
const Warnings = ({ status }: { status: EngineStatus }) => {
|
||||||
const report = status.lastReport;
|
const report = status.lastReport;
|
||||||
if (report === undefined) return null;
|
if (report === null) return null;
|
||||||
const count = report.warnings.length + (report.overWarn ? 1 : 0);
|
const count = report.warnings.length + (report.overWarn ? 1 : 0);
|
||||||
if (count === 0) return null;
|
if (count === 0) return null;
|
||||||
return (
|
return (
|
||||||
|
|||||||
Reference in New Issue
Block a user