From 621cb1428511ef5137c5012b0af8602b1654d4b6 Mon Sep 17 00:00:00 2001 From: Adnaan Badr Date: Sun, 19 Jul 2026 23:23:37 +0000 Subject: [PATCH 1/2] fix(upload): warn when marked fields cannot travel on a chunked upload MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Form fields only ever travel on the multipart path. The chunked WebSocket transport carries none — the server builds that action's context from an empty map (mount.go:3116) — so a field marked lvt-upload-with on a Volume/Direct upload never reaches upload__complete, with nothing in the markup to suggest why. Warn at the point of surprise instead. The mark is an explicit request that the field travel, so a chunked upload from a form that marks fields is worth saying out loud; a form that marks nothing gets no warning, since a message on every chunked upload would train people to ignore it. Not a regression: mount.go:3116 has always passed an empty map, so this path never carried fields. What changed is that #452 made field-carrying an explicit contract, and a contract that quietly doesn't hold on one transport is worth surfacing. Refs livetemplate/livetemplate#508 Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01Ui2cwpeGkrUfRt8rh2FgGG --- tests/upload-handler.test.ts | 73 ++++++++++++++++++++++++++++++++++++ upload/upload-handler.ts | 26 +++++++++++++ 2 files changed, 99 insertions(+) diff --git a/tests/upload-handler.test.ts b/tests/upload-handler.test.ts index 6d2a0a3..478eefb 100644 --- a/tests/upload-handler.test.ts +++ b/tests/upload-handler.test.ts @@ -416,6 +416,79 @@ describe("UploadHandler", () => { } }); + // Fires a chunked (WebSocket) auto-upload from the input named "doc" inside + // the given form markup and returns whatever console.warn was called with. + const chunkedUploadWarnings = async (formHTML: string): Promise => { + const warn = jest.spyOn(console, "warn").mockImplementation(() => {}); + const chunkedHandler = new UploadHandler(mockSendMessage); + + const form = document.createElement("form"); + form.innerHTML = formHTML; + const input = form.querySelector('input[name="doc"]') as HTMLInputElement; + + await chunkedHandler.startUpload( + "doc", + [createMockFile("scan.png", "imgdata", "image/png")], + input + ); + await chunkedHandler.handleUploadStartResponse({ + upload_name: "doc", + entries: [ + { + entry_id: "entry-1", + client_name: "scan.png", + valid: true, + auto_upload: true, + }, + ], + }); + await jest.runAllTimersAsync(); + + const messages = warn.mock.calls.map((c) => String(c[0])); + warn.mockRestore(); + return messages; + }; + + it("warns that marked fields cannot travel on a chunked upload (#508)", async () => { + const warnings = await chunkedUploadWarnings( + '' + + '' + ); + + // The chunked transport carries no form fields, so the mark is silently + // inert — say so rather than leaving an empty value in the handler with + // nothing in the markup to explain it. + const relevant = warnings.filter((m) => m.includes("lvt-upload-with")); + expect(relevant).toHaveLength(1); + expect(relevant[0]).toContain("id"); + expect(relevant[0]).toContain("chunked"); + }); + + it("stays quiet on a chunked upload when no field is marked", async () => { + const warnings = await chunkedUploadWarnings( + '' + + '' + ); + + // Nothing marked means nothing was promised, so there is nothing to warn + // about — a warning on every chunked upload would train people to ignore it. + expect(warnings.filter((m) => m.includes("lvt-upload-with"))).toHaveLength(0); + }); + + it("stays quiet on a proxied upload, where marked fields do travel", async () => { + const warn = jest.spyOn(console, "warn").mockImplementation(() => {}); + await proxiedUploadFormData( + '' + + '' + ); + + // Proxied is always multipart, so the fields arrive and the warning would + // be plain wrong. + const messages = warn.mock.calls.map((c) => String(c[0])); + warn.mockRestore(); + expect(messages.filter((m) => m.includes("lvt-upload-with"))).toHaveLength(0); + }); + it("previews locally without uploading when mode is preview", async () => { const createObjectURL = jest .fn() diff --git a/upload/upload-handler.ts b/upload/upload-handler.ts index 4690b4c..dde488e 100644 --- a/upload/upload-handler.ts +++ b/upload/upload-handler.ts @@ -618,6 +618,30 @@ export class UploadHandler { this.previewUrls.clear(); } + /** + * Warn when a form marks fields with lvt-upload-with but the upload is going + * out over the chunked WebSocket transport, which carries no form fields — the + * server builds that action's context from an empty map. Without this the + * fields simply never arrive and the handler sees empty values, with nothing in + * the markup to suggest why. Tracked as livetemplate/livetemplate#508. + */ + private warnIfMarkedFieldsCannotTravel(entry: UploadEntry): void { + const form = entry.sourceInput?.form; + if (!form) return; + const marked = Array.from(form.elements) + .filter((el) => el.hasAttribute("lvt-upload-with")) + .map((el) => (el as HTMLInputElement).name) + .filter(Boolean); + if (marked.length === 0) return; + console.warn( + `Upload "${entry.uploadName}" is using the chunked WebSocket transport, ` + + `which does not carry form fields — ${marked.join(", ")} marked ` + + `lvt-upload-with will not reach the upload_${entry.uploadName}_complete ` + + `handler. Fields travel on the multipart path only (Proxied uploads, and ` + + `Volume/Direct when the socket is down). See livetemplate/livetemplate#508.` + ); + } + /** * Upload file in chunks via WebSocket */ @@ -625,6 +649,8 @@ export class UploadHandler { const { file, id } = entry; let offset = 0; + this.warnIfMarkedFieldsCannotTravel(entry); + // Create abort controller for cancellation entry.abortController = new AbortController(); From e71b92ecabe7381ea82ec9463bcf2ed4021c1a8b Mon Sep 17 00:00:00 2001 From: Adnaan Badr Date: Mon, 20 Jul 2026 00:16:27 +0000 Subject: [PATCH 2/2] fix(upload): cover Direct too, and correct the remedy text MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review caught a factual error in the first pass: it treated Volume and Direct as the same case. They are not. case "direct" routes to uploadExternal, which PUTs straight to storage and completes with a metadata-only message. It touches neither the chunked nor the multipart path, so marked fields reach a Direct handler in no socket state — while Volume does deliver them on its multipart fallback. Two consequences, both fixed here: The warning claimed fields travel "Volume/Direct when the socket is down", which is a remedy that does not work for Direct. Someone following it would drop the socket and still find empty values — worse than no diagnostic, since a wrong one spends the reader's trust. The text is now per-transport: Volume points at the multipart fallback, Direct points at controller state. Direct had no warning at all, despite being the mode with the more complete failure. uploadExternal now warns too. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01Ui2cwpeGkrUfRt8rh2FgGG --- tests/upload-handler.test.ts | 46 ++++++++++++++++++++++++++++++++++++ upload/upload-handler.ts | 40 +++++++++++++++++++++---------- 2 files changed, 74 insertions(+), 12 deletions(-) diff --git a/tests/upload-handler.test.ts b/tests/upload-handler.test.ts index 478eefb..324ab9e 100644 --- a/tests/upload-handler.test.ts +++ b/tests/upload-handler.test.ts @@ -489,6 +489,52 @@ describe("UploadHandler", () => { expect(messages.filter((m) => m.includes("lvt-upload-with"))).toHaveLength(0); }); + it("warns on a Direct upload, which never carries fields on any path (#508)", async () => { + const warn = jest.spyOn(console, "warn").mockImplementation(() => {}); + const mockUploader = { upload: jest.fn().mockResolvedValue(undefined) }; + const directHandler = new UploadHandler(mockSendMessage); + directHandler.registerUploader("mock", mockUploader); + + const form = document.createElement("form"); + form.innerHTML = + '' + + ''; + const input = form.querySelector('input[name="doc"]') as HTMLInputElement; + + await directHandler.startUpload( + "doc", + [createMockFile("scan.png", "imgdata", "image/png")], + input + ); + await directHandler.handleUploadStartResponse({ + upload_name: "doc", + entries: [ + { + entry_id: "entry-1", + client_name: "scan.png", + valid: true, + auto_upload: true, + mode: "direct", + external: { uploader: "mock", url: "https://cdn.example/scan.png" }, + }, + ], + }); + await jest.runAllTimersAsync(); + + const messages = warn.mock.calls.map((c) => String(c[0])); + warn.mockRestore(); + + // Direct PUTs straight to storage and completes with a metadata-only + // message, so marked fields reach the handler on NO path — not even with + // the socket down, which is the multipart fallback Volume gets and Direct + // does not. The remedy has to differ accordingly. + const relevant = messages.filter((m) => m.includes("lvt-upload-with")); + expect(relevant).toHaveLength(1); + expect(relevant[0]).toContain("direct-to-storage"); + expect(relevant[0]).toContain("controller state"); + expect(relevant[0]).not.toContain("when the socket is down"); + }); + it("previews locally without uploading when mode is preview", async () => { const createObjectURL = jest .fn() diff --git a/upload/upload-handler.ts b/upload/upload-handler.ts index dde488e..a3aad2a 100644 --- a/upload/upload-handler.ts +++ b/upload/upload-handler.ts @@ -358,6 +358,8 @@ export class UploadHandler { entry: UploadEntry, meta: ExternalUploadMeta ): Promise { + this.warnIfMarkedFieldsCannotTravel(entry, "direct-to-storage"); + try { const uploader = this.uploaders.get(meta.uploader); if (!uploader) { @@ -619,13 +621,22 @@ export class UploadHandler { } /** - * Warn when a form marks fields with lvt-upload-with but the upload is going - * out over the chunked WebSocket transport, which carries no form fields — the - * server builds that action's context from an empty map. Without this the - * fields simply never arrive and the handler sees empty values, with nothing in - * the markup to suggest why. Tracked as livetemplate/livetemplate#508. + * Warn when a form marks fields with lvt-upload-with but the upload is leaving + * by a transport that carries no form fields. Only the multipart POST carries + * them: the chunked WebSocket path sends bytes and entry ids, and the Direct + * path PUTs straight to storage and completes with a metadata-only message — + * in both cases the server builds the action's context from an empty map. + * Without this the fields simply never arrive and the handler sees empty + * values, with nothing in the markup to suggest why. + * + * transport names the path taken, since the remedy differs: a Volume upload + * does carry fields when it falls back to multipart, while Direct never does + * on any path. Tracked as livetemplate/livetemplate#508. */ - private warnIfMarkedFieldsCannotTravel(entry: UploadEntry): void { + private warnIfMarkedFieldsCannotTravel( + entry: UploadEntry, + transport: "chunked WebSocket" | "direct-to-storage" + ): void { const form = entry.sourceInput?.form; if (!form) return; const marked = Array.from(form.elements) @@ -633,12 +644,17 @@ export class UploadHandler { .map((el) => (el as HTMLInputElement).name) .filter(Boolean); if (marked.length === 0) return; + const remedy = + transport === "chunked WebSocket" + ? `Fields travel on the multipart path only — Proxied uploads, and this ` + + `field when the socket is down.` + : `Fields travel on the multipart path only, which Direct never uses; ` + + `read the context from controller state instead.`; console.warn( - `Upload "${entry.uploadName}" is using the chunked WebSocket transport, ` + - `which does not carry form fields — ${marked.join(", ")} marked ` + - `lvt-upload-with will not reach the upload_${entry.uploadName}_complete ` + - `handler. Fields travel on the multipart path only (Proxied uploads, and ` + - `Volume/Direct when the socket is down). See livetemplate/livetemplate#508.` + `Upload "${entry.uploadName}" is using the ${transport} transport, which ` + + `does not carry form fields — ${marked.join(", ")} marked lvt-upload-with ` + + `will not reach the upload_${entry.uploadName}_complete handler. ` + + `${remedy} See livetemplate/livetemplate#508.` ); } @@ -649,7 +665,7 @@ export class UploadHandler { const { file, id } = entry; let offset = 0; - this.warnIfMarkedFieldsCannotTravel(entry); + this.warnIfMarkedFieldsCannotTravel(entry, "chunked WebSocket"); // Create abort controller for cancellation entry.abortController = new AbortController();