feat(KB-251): complete Step 1 — core types and store updates for AI title generation
This commit is contained in:
@@ -25,6 +25,7 @@ async function api<T = unknown>(path: string, opts: RequestInit = {}): Promise<T
|
||||
// Check if response is JSON before attempting to parse
|
||||
const contentType = res.headers.get("content-type") ?? "";
|
||||
const isJson = contentType.includes("application/json");
|
||||
const isHtml = contentType.includes("text/html");
|
||||
|
||||
if (!res.ok) {
|
||||
// For error responses, read as text first
|
||||
@@ -40,6 +41,11 @@ async function api<T = unknown>(path: string, opts: RequestInit = {}): Promise<T
|
||||
}
|
||||
}
|
||||
|
||||
// HTML error response (SPA fallback case) - show user-friendly message
|
||||
if (isHtml || text.trim().startsWith("<!DOCTYPE") || text.trim().startsWith("<html")) {
|
||||
throw new Error("Server returned an unexpected response. Please refresh and try again.");
|
||||
}
|
||||
|
||||
// Non-JSON error response: use status text with truncated body preview
|
||||
const textPreview = text.length > 100 ? text.slice(0, 100) + "..." : text;
|
||||
const message = textPreview
|
||||
@@ -50,8 +56,15 @@ async function api<T = unknown>(path: string, opts: RequestInit = {}): Promise<T
|
||||
|
||||
// For successful responses, parse as JSON if content-type indicates JSON
|
||||
if (!isJson) {
|
||||
// Unexpected non-JSON success response
|
||||
// Unexpected non-JSON success response (likely HTML from SPA fallback)
|
||||
const text = await res.text();
|
||||
|
||||
// Check if it looks like HTML
|
||||
if (isHtml || text.trim().startsWith("<!DOCTYPE") || text.trim().startsWith("<html")) {
|
||||
throw new Error("Server returned an unexpected response. Please refresh and try again.");
|
||||
}
|
||||
|
||||
// Other non-JSON response
|
||||
const textPreview = text.length > 100 ? text.slice(0, 100) + "..." : text;
|
||||
throw new Error(`Unexpected response format: expected JSON, got ${contentType || "unknown"}${textPreview ? ` (Response: ${textPreview})` : ""}`);
|
||||
}
|
||||
|
||||
113
packages/dashboard/src/server.test.ts
Normal file
113
packages/dashboard/src/server.test.ts
Normal file
@@ -0,0 +1,113 @@
|
||||
import { describe, it, expect, vi, beforeEach } from "vitest";
|
||||
import express from "express";
|
||||
import http from "node:http";
|
||||
import { createServer } from "./server.js";
|
||||
import type { TaskStore } from "@kb/core";
|
||||
|
||||
function createMockStore(overrides: Partial<TaskStore> = {}): TaskStore {
|
||||
return {
|
||||
getTask: vi.fn(),
|
||||
listTasks: vi.fn().mockResolvedValue([]),
|
||||
createTask: vi.fn(),
|
||||
moveTask: vi.fn(),
|
||||
updateTask: vi.fn(),
|
||||
deleteTask: vi.fn(),
|
||||
mergeTask: vi.fn(),
|
||||
archiveTask: vi.fn(),
|
||||
unarchiveTask: vi.fn(),
|
||||
getSettings: vi.fn().mockResolvedValue({}),
|
||||
updateSettings: vi.fn(),
|
||||
logEntry: vi.fn().mockResolvedValue(undefined),
|
||||
getAgentLogs: vi.fn().mockResolvedValue([]),
|
||||
addSteeringComment: vi.fn(),
|
||||
updatePrInfo: vi.fn().mockResolvedValue(undefined),
|
||||
updateIssueInfo: vi.fn().mockResolvedValue(undefined),
|
||||
getRootDir: vi.fn().mockReturnValue("/fake/root"),
|
||||
on: vi.fn(),
|
||||
off: vi.fn(),
|
||||
...overrides,
|
||||
} as unknown as TaskStore;
|
||||
}
|
||||
|
||||
/** Helper: send GET and return { status, body, headers } */
|
||||
async function GET(app: express.Express, path: string): Promise<{ status: number; body: unknown; headers: http.IncomingHttpHeaders }> {
|
||||
return new Promise((resolve, reject) => {
|
||||
const server = app.listen(0, () => {
|
||||
const addr = server.address() as { port: number };
|
||||
http.get(`http://127.0.0.1:${addr.port}${path}`, (res) => {
|
||||
let data = "";
|
||||
res.on("data", (chunk) => (data += chunk));
|
||||
res.on("end", () => {
|
||||
server.close();
|
||||
try {
|
||||
resolve({ status: res.statusCode!, body: JSON.parse(data), headers: res.headers });
|
||||
} catch {
|
||||
resolve({ status: res.statusCode!, body: data, headers: res.headers });
|
||||
}
|
||||
});
|
||||
}).on("error", (err) => { server.close(); reject(err); });
|
||||
});
|
||||
});
|
||||
}
|
||||
|
||||
describe("API Error Handling Middleware", () => {
|
||||
let store: TaskStore;
|
||||
|
||||
beforeEach(() => {
|
||||
store = createMockStore();
|
||||
});
|
||||
|
||||
describe("404 handler for unmatched API routes", () => {
|
||||
it("returns JSON 404 for unmatched API routes", async () => {
|
||||
const app = createServer(store);
|
||||
const res = await GET(app, "/api/nonexistent/route");
|
||||
|
||||
expect(res.status).toBe(404);
|
||||
expect(res.body).toEqual({ error: "Not found" });
|
||||
expect(res.headers["content-type"]).toContain("application/json");
|
||||
});
|
||||
|
||||
it("returns JSON 404 for unmatched API paths under known routes", async () => {
|
||||
const app = createServer(store);
|
||||
const res = await GET(app, "/api/tasks/nonexistent/path");
|
||||
|
||||
expect(res.status).toBe(404);
|
||||
expect(res.body).toEqual({ error: "Not found" });
|
||||
expect(res.headers["content-type"]).toContain("application/json");
|
||||
});
|
||||
});
|
||||
|
||||
describe("Error handler for route failures", () => {
|
||||
it("returns JSON 500 when a route handler throws an error", async () => {
|
||||
// Create a store that throws an error for listTasks
|
||||
const failingStore = createMockStore({
|
||||
listTasks: vi.fn().mockRejectedValue(new Error("Database connection failed")),
|
||||
});
|
||||
|
||||
const app = createServer(failingStore);
|
||||
const res = await GET(app, "/api/tasks");
|
||||
|
||||
expect(res.status).toBe(500);
|
||||
expect(res.body).toEqual({ error: "Internal server error" });
|
||||
expect(res.headers["content-type"]).toContain("application/json");
|
||||
});
|
||||
});
|
||||
|
||||
describe("SPA fallback behavior", () => {
|
||||
it("does not return HTML for API 404s", async () => {
|
||||
const app = createServer(store);
|
||||
const res = await GET(app, "/api/unknown-endpoint");
|
||||
|
||||
// Should NOT get HTML (the SPA fallback returns HTML)
|
||||
expect(res.status).toBe(404);
|
||||
expect(typeof res.body).toBe("object"); // JSON object
|
||||
expect(res.body).toHaveProperty("error");
|
||||
expect(res.headers["content-type"]).toContain("application/json");
|
||||
// Verify we didn't get HTML
|
||||
if (typeof res.body === "string") {
|
||||
expect(res.body).not.toContain("<!DOCTYPE html>");
|
||||
expect(res.body).not.toContain("<html");
|
||||
}
|
||||
});
|
||||
});
|
||||
});
|
||||
@@ -169,6 +169,25 @@ export function createServer(store: TaskStore, options?: ServerOptions): ReturnT
|
||||
// REST API
|
||||
app.use("/api", createApiRoutes(store, options));
|
||||
|
||||
// API 404 Handler - Return JSON for unmatched API routes (instead of falling through to SPA)
|
||||
app.use("/api", (_req: express.Request, res: express.Response) => {
|
||||
res.status(404).json({ error: "Not found" });
|
||||
});
|
||||
|
||||
// API Error Handling Middleware - MUST be after API routes but before SPA fallback
|
||||
// This ensures API errors return JSON instead of falling through to the SPA fallback (which returns HTML)
|
||||
// eslint-disable-next-line @typescript-eslint/no-unused-vars
|
||||
app.use("/api", (err: Error, _req: express.Request, res: express.Response, _next: express.NextFunction) => {
|
||||
console.error("[api:error]", err);
|
||||
|
||||
// Ensure we send a JSON response even if headers already sent (though this is a edge case)
|
||||
if (res.headersSent) {
|
||||
return;
|
||||
}
|
||||
|
||||
res.status(500).json({ error: "Internal server error" });
|
||||
});
|
||||
|
||||
// SPA fallback
|
||||
app.get("/{*splat}", (_req, res) => {
|
||||
res.sendFile(join(clientDir, "index.html"));
|
||||
|
||||
Reference in New Issue
Block a user