Skip to content

Commit 6e5f460

Browse files
vanceingallsclaude
andauthored
fix(studio): split the empty-read reason and stop blaming the composition (#4325)
Two residuals from the cutover-dashboard work, both found in the data the first pass shipped (0.8.61/0.8.62). `absent_or_empty` was one label over two different failures: the route answers `content: ""` both from the `optional=1` shim (nothing resolved at that path) and for a real 0-byte file. The route now marks the shim with `missing: true`, and the read path with `missing: false` so the field's PRESENCE is what tells the client this server draws the distinction — a new server's empty file would otherwise look exactly like either case from an old one. Client splits them into `absent` and `empty_file`; a server without the field still reports `absent_or_empty`, so the existing series stays honest instead of silently folding into one of the new ones. Why it is worth splitting: on 0.8.62, 75 tabs hit this class and not one of them ever landed an SDK edit afterwards — it is terminal for the tab, while the tab keeps playing back and navigating, so nothing surfaces to the user. Every measured case with a loaded tree carries `path_in_tree: true`: a file the tree lists and this read cannot get. Which of the two causes that is decides the fix, and until now the event could not say. `res.json()` at the end of the read was unguarded, so a 200 carrying HTML — an SPA fallback or a proxy answering in the route's place — rejected out of the function entirely and the effect's outer catch filed it as `stage: "open"`, which asserts the COMPOSITION failed to parse. Observed in production on 0.8.62: `Unexpected token '<', "<!-- /*!"... is not valid JSON`. That is the third label to make the same wrong claim, after the fetch-rejection split in #4240 and #4269. Now `reason: "invalid_json"` with the response's content-type. `path_in_tree` now rides all three empty-read reasons, not just the combined one, so the split keeps the signal that justified it. Tests: 4 new (absent / empty_file / combined-label fallback / non-JSON 200) plus a server test pinning `missing` on both answers. 1172 tests green across studio hooks + studio-server routes; oxlint, oxfmt, tsc clean. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent b32b9a7 commit 6e5f460

4 files changed

Lines changed: 196 additions & 17 deletions

File tree

‎packages/studio-server/src/routes/files.test.ts‎

Lines changed: 25 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -255,11 +255,35 @@ describe("registerFileRoutes", () => {
255255
const response = await app.request(
256256
"http://localhost/projects/demo/files/missing-file.txt?optional=1",
257257
);
258-
const payload = (await response.json()) as { filename?: string; content?: string };
258+
const payload = (await response.json()) as {
259+
filename?: string;
260+
content?: string;
261+
missing?: boolean;
262+
};
259263

260264
expect(response.status).toBe(200);
261265
expect(payload.filename).toBe("missing-file.txt");
262266
expect(payload.content).toBe("");
267+
expect(payload.missing).toBe(true);
268+
});
269+
270+
// The shim and a real 0-byte file both answer `content: ""`. `missing` is
271+
// the only thing separating them, and it has to be on BOTH answers — its
272+
// presence is what tells a caller this server draws the distinction at all.
273+
it("marks a real zero-byte file as present, not missing", async () => {
274+
const projectDir = createProjectDir();
275+
writeFileSync(join(projectDir, "empty.html"), "");
276+
const app = new Hono();
277+
registerFileRoutes(app, createAdapter(projectDir));
278+
279+
const response = await app.request(
280+
"http://localhost/projects/demo/files/empty.html?optional=1",
281+
);
282+
const payload = (await response.json()) as { content?: string; missing?: boolean };
283+
284+
expect(response.status).toBe(200);
285+
expect(payload.content).toBe("");
286+
expect(payload.missing).toBe(false);
263287
});
264288

265289
it("still returns 404 for other missing files", async () => {

‎packages/studio-server/src/routes/files.ts‎

Lines changed: 18 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2278,15 +2278,31 @@ export function registerFileRoutes(api: Hono, adapter: StudioApiAdapter): void {
22782278

22792279
if (!existsSync(res.absPath)) {
22802280
if (c.req.query("optional") === "1") {
2281-
return c.json({ filename: res.filePath, content: "" });
2281+
// `missing: true` separates the absent-file shim from a genuinely
2282+
// 0-byte file — both answer `content: ""`, and the caller could not
2283+
// tell them apart. That ambiguity hid the largest remaining class of
2284+
// SDK-session failures: a composition the file tree lists but this
2285+
// read answers empty for is either a placeholder nobody has written
2286+
// yet, or a path that does not resolve here at all, and those need
2287+
// different fixes. Additive, so an older client ignores it.
2288+
return c.json({ filename: res.filePath, content: "", missing: true });
22822289
}
22832290
return c.json({ error: "not found" }, 404);
22842291
}
22852292

22862293
const content = readFileSync(res.absPath);
22872294
const version = fileContentVersion(content);
22882295
c.header("ETag", version);
2289-
return c.json({ filename: res.filePath, content: content.toString("utf-8"), version });
2296+
// `missing: false` on the read path too, so its PRESENCE is what tells a
2297+
// caller this server distinguishes the two empty answers at all. Without
2298+
// it here, a real 0-byte file from a new server looks exactly like either
2299+
// case from an old one, and the split above buys nothing.
2300+
return c.json({
2301+
filename: res.filePath,
2302+
content: content.toString("utf-8"),
2303+
version,
2304+
missing: false,
2305+
});
22902306
});
22912307

22922308
// ── Write (overwrite) ──

‎packages/studio/src/hooks/useSdkSession.lifecycle.test.tsx‎

Lines changed: 78 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -440,7 +440,9 @@ describe("useSdkSession unavailable telemetry", () => {
440440
// so this is what "the composition genuinely is not there" looks like on the
441441
// wire — previously indistinguishable from a broken request. The shim and a
442442
// real 0-byte file are the same response, hence the name.
443-
it("separates a file that is not on disk", async () => {
443+
// An older server sends no `missing` field, so the combined label stays —
444+
// rather than guessing one of the two and quietly corrupting the series.
445+
it("keeps the combined label when the server does not say which empty this is", async () => {
444446
vi.stubGlobal(
445447
"fetch",
446448
vi.fn(async () => ({ ok: true, json: async () => ({ content: "" }) }) as Response),
@@ -459,6 +461,81 @@ describe("useSdkSession unavailable telemetry", () => {
459461
await act(async () => root.unmount());
460462
});
461463

464+
// `missing: true` is the route's shim — nothing resolved at that path.
465+
it("reports a file the server could not find as absent", async () => {
466+
vi.stubGlobal(
467+
"fetch",
468+
vi.fn(
469+
async () => ({ ok: true, json: async () => ({ content: "", missing: true }) }) as Response,
470+
),
471+
);
472+
const root = createRoot(document.createElement("div"));
473+
await act(async () => root.render(<Probe projectId="project-a" />));
474+
await flushAsyncEffects();
475+
476+
expect(trackMock).toHaveBeenCalledWith("sdk_session_unavailable", {
477+
stage: "read",
478+
reason: "absent",
479+
path_in_tree: null,
480+
});
481+
await act(async () => root.unmount());
482+
});
483+
484+
// `missing: false` with empty content is a real 0-byte file on disk — a
485+
// placeholder somebody created and has not written yet, not a bad path.
486+
it("reports a real zero-byte file separately from an absent one", async () => {
487+
vi.stubGlobal(
488+
"fetch",
489+
vi.fn(
490+
async () => ({ ok: true, json: async () => ({ content: "", missing: false }) }) as Response,
491+
),
492+
);
493+
const root = createRoot(document.createElement("div"));
494+
await act(async () => root.render(<Probe projectId="project-a" />));
495+
await flushAsyncEffects();
496+
497+
expect(trackMock).toHaveBeenCalledWith("sdk_session_unavailable", {
498+
stage: "read",
499+
reason: "empty_file",
500+
path_in_tree: null,
501+
});
502+
await act(async () => root.unmount());
503+
});
504+
505+
// A 200 carrying HTML is an SPA fallback or a proxy answering in the route's
506+
// place. Before this, `res.json()` rejected outside any catch and the outer
507+
// catch filed it as `stage: "open"` — blaming the user's composition for a
508+
// response the composition had nothing to do with.
509+
it("reports a non-JSON 200 as a read failure, not a composition parse failure", async () => {
510+
vi.stubGlobal(
511+
"fetch",
512+
vi.fn(
513+
async () =>
514+
({
515+
ok: true,
516+
headers: { get: () => "text/html; charset=utf-8" },
517+
json: async () => {
518+
throw new SyntaxError(`Unexpected token '<', "<!-- /*!"... is not valid JSON`);
519+
},
520+
}) as unknown as Response,
521+
),
522+
);
523+
const root = createRoot(document.createElement("div"));
524+
await act(async () => root.render(<Probe projectId="project-a" />));
525+
await flushAsyncEffects();
526+
527+
expect(trackMock).toHaveBeenCalledWith("sdk_session_unavailable", {
528+
stage: "read",
529+
reason: "invalid_json",
530+
content_type: "text/html; charset=utf-8",
531+
});
532+
expect(trackMock).not.toHaveBeenCalledWith(
533+
"sdk_session_unavailable",
534+
expect.objectContaining({ stage: "open" }),
535+
);
536+
await act(async () => root.unmount());
537+
});
538+
462539
// The graveyard-refuted fix's proposed next step: instrument, don't act.
463540
// `path_in_tree` separates "genuinely not in the loaded tree" from
464541
// "concurrent/duplicate open" without deciding anything on Studio's behalf.

‎packages/studio/src/hooks/useSdkSession.ts‎

Lines changed: 75 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -35,16 +35,46 @@ import { addExternalFileReloadListener } from "./externalFileReloadBus";
3535
* before it left the browser — CSP `connect-src`, Private Network Access, an
3636
* extension rewriting `fetch` (rejects near-instantly, tab stays visible and
3737
* alive). Telemetry before this change could not tell the two apart.
38+
*
39+
* `absent` and `empty_file` split what `absent_or_empty` could not: the route
40+
* answers `content: ""` both for a file it cannot find (the `optional=1` shim)
41+
* and for a real 0-byte one, so the single label covered a composition nobody
42+
* has written yet AND a path that does not resolve on this server. The route
43+
* now marks the shim with `missing: true`. A server without that field still
44+
* lands in `absent_or_empty`, so the old series stays honest rather than
45+
* silently folding into one of the new ones.
46+
*
47+
* Measured on 0.8.62 before the split: 75 tabs hit this class and not one of
48+
* them ever landed an SDK edit afterwards — it is terminal for the tab, while
49+
* the tab itself keeps playing back and navigating, so nothing surfaces.
50+
*
51+
* `invalid_json` is a 200 whose body is not JSON at all — in production, an
52+
* HTML page (`Unexpected token '<', "<!-- /*!"...`), i.e. an SPA fallback or a
53+
* proxy answering in the route's place. `res.json()` rejected outside any
54+
* catch until now, so it escaped this function and the effect's outer catch
55+
* filed it as `stage: "open"` — which asserts the COMPOSITION failed to parse.
56+
* Third label to make that same wrong claim, after `network` (#4240) and the
57+
* fetch-rejection split; this one is the response shape, not the composition.
3858
*/
3959
type ProjectFileReadFailure =
4060
| { ok: false; reason: "unsafe_path" }
4161
| { ok: false; reason: "http_error"; status: number; why?: string }
4262
| { ok: false; reason: "missing_content" }
4363
| { ok: false; reason: "absent_or_empty" }
64+
| { ok: false; reason: "absent" }
65+
| { ok: false; reason: "empty_file" }
66+
| { ok: false; reason: "invalid_json"; contentType: string }
4467
| { ok: false; reason: "network"; elapsedMs: number; hidden: boolean };
4568

4669
type ProjectFileReadResult = { ok: true; content: string } | ProjectFileReadFailure;
4770

71+
/** The three ways a 200 can carry no composition — old combined label plus its split. */
72+
const EMPTY_READ_REASONS = new Set<ProjectFileReadFailure["reason"]>([
73+
"absent_or_empty",
74+
"absent",
75+
"empty_file",
76+
]);
77+
4878
/**
4979
* Record a read that produced no usable content, and answer which project — if
5080
* any — the failure identifies as unreachable.
@@ -73,12 +103,15 @@ function reportReadFailure(
73103
reason: read.reason,
74104
...(read.reason === "http_error" ? { status: read.status, why: read.why } : {}),
75105
...(read.reason === "network" ? { elapsed_ms: read.elapsedMs, hidden: read.hidden } : {}),
76-
// Only meaningful for `absent_or_empty` — the graveyard-refuted "clear
106+
...(read.reason === "invalid_json" ? { content_type: read.contentType } : {}),
107+
// Only meaningful for the empty-read reasons — the graveyard-refuted "clear
77108
// activeCompPath when it's not in the tree" fix's proposed next step,
78109
// scoped to instrumentation only. `null` while the tree hasn't loaded
79110
// yet: a false-negative there would read as "genuinely absent" when it is
80-
// really "haven't looked".
81-
...(read.reason === "absent_or_empty" ? { path_in_tree: pathInTree } : {}),
111+
// really "haven't looked". Carried on all three so the split keeps the
112+
// signal that made it worth splitting: every measured case so far is
113+
// `path_in_tree: true`, a file the tree lists and this read cannot get.
114+
...(EMPTY_READ_REASONS.has(read.reason) ? { path_in_tree: pathInTree } : {}),
82115
});
83116
if (read.reason !== "http_error") return null;
84117
return read.status === 404 ? projectId : null;
@@ -118,6 +151,30 @@ async function httpReadFailureWhy(res: Response): Promise<string | undefined> {
118151
return typeof body?.why === "string" ? body.why : undefined;
119152
}
120153

154+
interface ProjectFileBody {
155+
content?: string;
156+
/** Present only from a server that separates its shim from a real 0-byte file. */
157+
missing?: boolean;
158+
}
159+
160+
/**
161+
* Decide what a 200 with a JSON body actually delivered.
162+
*
163+
* `optional=1` answers a missing file with 200 + `content: ""`, so a non-string
164+
* content is a response shape we did not expect, not absence. An empty string
165+
* parses into a session with no elements, which declines every edit wholesale —
166+
* not a session worth opening — and `missing` is the route saying which of the
167+
* two empties it was. Without that field (older server) the combined label
168+
* stands rather than guessing one and corrupting the series.
169+
*/
170+
function classifyProjectFileBody(data: ProjectFileBody): ProjectFileReadResult {
171+
if (typeof data.content !== "string") return { ok: false, reason: "missing_content" };
172+
if (data.content !== "") return { ok: true, content: data.content };
173+
if (data.missing === true) return { ok: false, reason: "absent" };
174+
if (data.missing === false) return { ok: false, reason: "empty_file" };
175+
return { ok: false, reason: "absent_or_empty" };
176+
}
177+
121178
/**
122179
* Read a project file's content (optional read — a missing file is not an
123180
* error). Replaces the removed SDK http adapter's `read()` — the only thing
@@ -146,16 +203,21 @@ async function readProjectFileOptional(
146203
const why = await httpReadFailureWhy(res);
147204
return { ok: false, reason: "http_error", status: res.status, ...(why ? { why } : {}) };
148205
}
149-
const data = (await res.json()) as { content?: string };
150-
// `optional=1` answers a missing file with 200 + `content: ""`, so a
151-
// non-string here means a response shape we did not expect, not absence.
152-
if (typeof data.content !== "string") return { ok: false, reason: "missing_content" };
153-
// An empty body parses into a session with no elements, which declines every
154-
// edit wholesale — not a session worth opening. The absent-file shim and a
155-
// genuinely 0-byte file are the same 200 on the wire and cannot be told
156-
// apart here, hence the name; for a composition it is always the former.
157-
if (data.content === "") return { ok: false, reason: "absent_or_empty" };
158-
return { ok: true, content: data.content };
206+
// A 200 that is not JSON is a different failure from a JSON body missing its
207+
// field: it means something other than this route answered — an SPA fallback
208+
// or a proxy. Left unguarded, it rejected out of this function entirely and
209+
// the effect's outer catch blamed the composition. See the type's comment.
210+
let data: ProjectFileBody;
211+
try {
212+
data = (await res.json()) as ProjectFileBody;
213+
} catch {
214+
return {
215+
ok: false,
216+
reason: "invalid_json",
217+
contentType: res.headers.get("content-type") ?? "",
218+
};
219+
}
220+
return classifyProjectFileBody(data);
159221
}
160222

161223
/**

0 commit comments

Comments
 (0)