From 1b71a3bd2678ed38aadb336f88316da529a0aaae Mon Sep 17 00:00:00 2001 From: Atila Fassina Date: Tue, 7 Apr 2026 19:35:07 +0200 Subject: [PATCH 1/3] refactor: use HTTP status code descriptions instead of hardcoded error strings Replace hardcoded error messages like "List failed" with standard HTTP status descriptions from node:http STATUS_CODES for consistent error responses. Co-authored-by: Isaac Signed-off-by: Atila Fassina --- packages/appkit/src/plugins/files/plugin.ts | 46 +++++++++++++++---- .../files/tests/plugin.integration.test.ts | 8 ++-- .../src/plugins/files/tests/plugin.test.ts | 16 +++---- 3 files changed, 49 insertions(+), 21 deletions(-) diff --git a/packages/appkit/src/plugins/files/plugin.ts b/packages/appkit/src/plugins/files/plugin.ts index e745b347..78cbfdd1 100644 --- a/packages/appkit/src/plugins/files/plugin.ts +++ b/packages/appkit/src/plugins/files/plugin.ts @@ -1,3 +1,4 @@ +import { STATUS_CODES } from "node:http"; import { Readable } from "node:stream"; import { ApiError } from "@databricks/sdk-experimental"; import type express from "express"; @@ -448,7 +449,10 @@ export class FilesPlugin extends Plugin { if (!result.ok) { res .status(result.status) - .json({ error: "List failed", plugin: this.name }); + .json({ + error: STATUS_CODES[result.status] ?? "Unknown Error", + plugin: this.name, + }); return; } res.json(result.data); @@ -486,7 +490,10 @@ export class FilesPlugin extends Plugin { if (!result.ok) { res .status(result.status) - .json({ error: "Read failed", plugin: this.name }); + .json({ + error: STATUS_CODES[result.status] ?? "Unknown Error", + plugin: this.name, + }); return; } res.type("text/plain").send(result.data); @@ -552,7 +559,10 @@ export class FilesPlugin extends Plugin { if (!response.ok) { res .status(response.status) - .json({ error: `${label} failed`, plugin: this.name }); + .json({ + error: STATUS_CODES[response.status] ?? "Unknown Error", + plugin: this.name, + }); return; } @@ -633,7 +643,10 @@ export class FilesPlugin extends Plugin { if (!result.ok) { res .status(result.status) - .json({ error: "Exists check failed", plugin: this.name }); + .json({ + error: STATUS_CODES[result.status] ?? "Unknown Error", + plugin: this.name, + }); return; } res.json({ exists: result.data }); @@ -671,7 +684,10 @@ export class FilesPlugin extends Plugin { if (!result.ok) { res .status(result.status) - .json({ error: "Metadata fetch failed", plugin: this.name }); + .json({ + error: STATUS_CODES[result.status] ?? "Unknown Error", + plugin: this.name, + }); return; } res.json(result.data); @@ -709,7 +725,10 @@ export class FilesPlugin extends Plugin { if (!result.ok) { res .status(result.status) - .json({ error: "Preview failed", plugin: this.name }); + .json({ + error: STATUS_CODES[result.status] ?? "Unknown Error", + plugin: this.name, + }); return; } res.json(result.data); @@ -809,7 +828,10 @@ export class FilesPlugin extends Plugin { ); res .status(result.status) - .json({ error: "Upload failed", plugin: this.name }); + .json({ + error: STATUS_CODES[result.status] ?? "Unknown Error", + plugin: this.name, + }); return; } @@ -864,7 +886,10 @@ export class FilesPlugin extends Plugin { if (!result.ok) { res .status(result.status) - .json({ error: "Create directory failed", plugin: this.name }); + .json({ + error: STATUS_CODES[result.status] ?? "Unknown Error", + plugin: this.name, + }); return; } @@ -911,7 +936,10 @@ export class FilesPlugin extends Plugin { if (!result.ok) { res .status(result.status) - .json({ error: "Delete failed", plugin: this.name }); + .json({ + error: STATUS_CODES[result.status] ?? "Unknown Error", + plugin: this.name, + }); return; } diff --git a/packages/appkit/src/plugins/files/tests/plugin.integration.test.ts b/packages/appkit/src/plugins/files/tests/plugin.integration.test.ts index e0117cdc..cbd5a529 100644 --- a/packages/appkit/src/plugins/files/tests/plugin.integration.test.ts +++ b/packages/appkit/src/plugins/files/tests/plugin.integration.test.ts @@ -567,7 +567,7 @@ describe("Files Plugin Integration", () => { expect(response.status).toBe(500); const data = (await response.json()) as { error: string; plugin: string }; - expect(data.error).toBe("Metadata fetch failed"); + expect(data.error).toBe("Internal Server Error"); expect(data.plugin).toBe("files"); }); @@ -583,7 +583,7 @@ describe("Files Plugin Integration", () => { expect(response.status).toBe(500); const data = (await response.json()) as { error: string; plugin: string }; - expect(data.error).toBe("List failed"); + expect(data.error).toBe("Internal Server Error"); expect(data.plugin).toBe("files"); }); @@ -602,7 +602,7 @@ describe("Files Plugin Integration", () => { error: string; plugin: string; }; - expect(data.error).toBe("Metadata fetch failed"); + expect(data.error).toBe("Internal Server Error"); expect(data.plugin).toBe("files"); }); @@ -622,7 +622,7 @@ describe("Files Plugin Integration", () => { error: string; plugin: string; }; - expect(data.error).toBe("Create directory failed"); + expect(data.error).toBe("Internal Server Error"); expect(data.plugin).toBe("files"); }); }); diff --git a/packages/appkit/src/plugins/files/tests/plugin.test.ts b/packages/appkit/src/plugins/files/tests/plugin.test.ts index 27b55cd5..99e08b8c 100644 --- a/packages/appkit/src/plugins/files/tests/plugin.test.ts +++ b/packages/appkit/src/plugins/files/tests/plugin.test.ts @@ -674,7 +674,7 @@ describe("FilesPlugin", () => { expect(res.status).toHaveBeenCalledWith(500); expect(res.json).toHaveBeenCalledWith( expect.objectContaining({ - error: "List failed", + error: "Internal Server Error", plugin: "files", }), ); @@ -698,7 +698,7 @@ describe("FilesPlugin", () => { expect(res.status).toHaveBeenCalledWith(500); expect(res.json).toHaveBeenCalledWith( - expect.objectContaining({ error: "Read failed" }), + expect.objectContaining({ error: "Internal Server Error" }), ); }); @@ -722,7 +722,7 @@ describe("FilesPlugin", () => { expect(res.status).toHaveBeenCalledWith(500); expect(res.json).toHaveBeenCalledWith( - expect.objectContaining({ error: "Exists check failed" }), + expect.objectContaining({ error: "Internal Server Error" }), ); }); @@ -746,7 +746,7 @@ describe("FilesPlugin", () => { expect(res.status).toHaveBeenCalledWith(500); expect(res.json).toHaveBeenCalledWith( - expect.objectContaining({ error: "Metadata fetch failed" }), + expect.objectContaining({ error: "Internal Server Error" }), ); }); @@ -768,7 +768,7 @@ describe("FilesPlugin", () => { expect(res.status).toHaveBeenCalledWith(500); expect(res.json).toHaveBeenCalledWith( - expect.objectContaining({ error: "Download failed" }), + expect.objectContaining({ error: "Internal Server Error" }), ); }); @@ -791,7 +791,7 @@ describe("FilesPlugin", () => { expect(res.status).toHaveBeenCalledWith(500); expect(res.json).toHaveBeenCalledWith( - expect.objectContaining({ error: "Create directory failed" }), + expect.objectContaining({ error: "Internal Server Error" }), ); }); @@ -835,7 +835,7 @@ describe("FilesPlugin", () => { await handlerPromise; const errorBody = res.json.mock.calls[0][0]; - expect(errorBody.error).toBe("List failed"); + expect(errorBody.error).toBe("Internal Server Error"); expect(errorBody.error).not.toContain("secret"); expect(errorBody.error).not.toContain("internal"); }); @@ -876,7 +876,7 @@ describe("FilesPlugin", () => { expect(signalWasAborted).toBe(true); expect(res.status).toHaveBeenCalledWith(500); expect(res.json).toHaveBeenCalledWith( - expect.objectContaining({ error: "List failed" }), + expect.objectContaining({ error: "Internal Server Error" }), ); }); From 971abe2d1a53d31be3c7910c077be6af6b3e8994 Mon Sep 17 00:00:00 2001 From: Atila Fassina Date: Wed, 8 Apr 2026 12:46:56 +0200 Subject: [PATCH 2/3] chore: dry status error responses in files plugin Co-authored-by: Isaac Signed-off-by: Atila Fassina --- packages/appkit/src/plugins/files/plugin.ts | 70 +++++---------------- 1 file changed, 16 insertions(+), 54 deletions(-) diff --git a/packages/appkit/src/plugins/files/plugin.ts b/packages/appkit/src/plugins/files/plugin.ts index 78cbfdd1..1c57d082 100644 --- a/packages/appkit/src/plugins/files/plugin.ts +++ b/packages/appkit/src/plugins/files/plugin.ts @@ -425,6 +425,13 @@ export class FilesPlugin extends Plugin { res.status(500).json({ error: fallbackMessage, plugin: this.name }); } + private _sendStatusError(res: express.Response, status: number): void { + res.status(status).json({ + error: STATUS_CODES[status] ?? "Unknown Error", + plugin: this.name, + }); + } + private async _handleList( req: express.Request, res: express.Response, @@ -447,12 +454,7 @@ export class FilesPlugin extends Plugin { ); if (!result.ok) { - res - .status(result.status) - .json({ - error: STATUS_CODES[result.status] ?? "Unknown Error", - plugin: this.name, - }); + this._sendStatusError(res, result.status); return; } res.json(result.data); @@ -488,12 +490,7 @@ export class FilesPlugin extends Plugin { ); if (!result.ok) { - res - .status(result.status) - .json({ - error: STATUS_CODES[result.status] ?? "Unknown Error", - plugin: this.name, - }); + this._sendStatusError(res, result.status); return; } res.type("text/plain").send(result.data); @@ -557,12 +554,7 @@ export class FilesPlugin extends Plugin { }, settings); if (!response.ok) { - res - .status(response.status) - .json({ - error: STATUS_CODES[response.status] ?? "Unknown Error", - plugin: this.name, - }); + this._sendStatusError(res, response.status); return; } @@ -641,12 +633,7 @@ export class FilesPlugin extends Plugin { ); if (!result.ok) { - res - .status(result.status) - .json({ - error: STATUS_CODES[result.status] ?? "Unknown Error", - plugin: this.name, - }); + this._sendStatusError(res, result.status); return; } res.json({ exists: result.data }); @@ -682,12 +669,7 @@ export class FilesPlugin extends Plugin { ); if (!result.ok) { - res - .status(result.status) - .json({ - error: STATUS_CODES[result.status] ?? "Unknown Error", - plugin: this.name, - }); + this._sendStatusError(res, result.status); return; } res.json(result.data); @@ -723,12 +705,7 @@ export class FilesPlugin extends Plugin { ); if (!result.ok) { - res - .status(result.status) - .json({ - error: STATUS_CODES[result.status] ?? "Unknown Error", - plugin: this.name, - }); + this._sendStatusError(res, result.status); return; } res.json(result.data); @@ -826,12 +803,7 @@ export class FilesPlugin extends Plugin { path, contentLength ?? 0, ); - res - .status(result.status) - .json({ - error: STATUS_CODES[result.status] ?? "Unknown Error", - plugin: this.name, - }); + this._sendStatusError(res, result.status); return; } @@ -884,12 +856,7 @@ export class FilesPlugin extends Plugin { ); if (!result.ok) { - res - .status(result.status) - .json({ - error: STATUS_CODES[result.status] ?? "Unknown Error", - plugin: this.name, - }); + this._sendStatusError(res, result.status); return; } @@ -934,12 +901,7 @@ export class FilesPlugin extends Plugin { ); if (!result.ok) { - res - .status(result.status) - .json({ - error: STATUS_CODES[result.status] ?? "Unknown Error", - plugin: this.name, - }); + this._sendStatusError(res, result.status); return; } From 9770eb11ad9febdffe8c73d4c3daa17573258430 Mon Sep 17 00:00:00 2001 From: Atila Fassina Date: Wed, 8 Apr 2026 12:49:29 +0200 Subject: [PATCH 3/3] fix: standardize stream error to use _sendStatusError helper Addresses Copilot review comment about mixed error formats in the read/download stream handler. Co-authored-by: Isaac Signed-off-by: Atila Fassina --- packages/appkit/src/plugins/files/plugin.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/packages/appkit/src/plugins/files/plugin.ts b/packages/appkit/src/plugins/files/plugin.ts index 1c57d082..9344af85 100644 --- a/packages/appkit/src/plugins/files/plugin.ts +++ b/packages/appkit/src/plugins/files/plugin.ts @@ -590,9 +590,7 @@ export class FilesPlugin extends Plugin { nodeStream.on("error", (err) => { logger.error("Stream error during %s: %O", opts.mode, err); if (!res.headersSent) { - res - .status(500) - .json({ error: `${label} failed`, plugin: this.name }); + this._sendStatusError(res, 500); } else { res.destroy(); }