From 58479d76a805071b9345534090ff7fcb35882bc2 Mon Sep 17 00:00:00 2001 From: kosako Date: Sun, 14 Jun 2026 23:33:16 +0900 Subject: [PATCH 1/2] Store feedback in node:sqlite behind a swappable store interface MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The receiver kept all feedback in a single JSON array and rewrote the whole file on every change, with queries and deletes scanning the array in memory — fine for a prototype, but it degrades by the hundreds of items. Persistence now lives in server/store.js behind a small async interface (init/insert/get/list/update/delete/count) with a node:sqlite implementation; a future backend like MySQL implements the same contract without touching the receiver. The interface is async so a network-backed driver fits the same shape. A minimal portable schema (id + project/demo/status/received_at columns for filtering, the full object in a JSON column) keeps payload growth schema-free and maps cleanly to other SQL engines. On startup a legacy feedback.json is migrated once and archived; a corrupt one is set aside, preserving the old crash-safe behavior. Co-Authored-By: Claude Fable 5 --- .gitignore | 6 ++ README.en.md | 15 +++- README.md | 15 +++- server/receive.js | 141 ++++++++++++++------------------ server/store.js | 186 ++++++++++++++++++++++++++++++++++++++++++ test/receiver.test.js | 78 +++++++++++++----- 6 files changed, 335 insertions(+), 106 deletions(-) create mode 100644 server/store.js diff --git a/.gitignore b/.gitignore index 7ad7807..1a3daa3 100644 --- a/.gitignore +++ b/.gitignore @@ -1,4 +1,10 @@ server/feedback.json +server/feedback.json.migrated-* +server/feedback.json.corrupt-* +server/feedback.db +server/feedback.db-journal +server/feedback.db-wal +server/feedback.db-shm server/screenshots/ server/receiver.config.json node_modules/ diff --git a/README.en.md b/README.en.md index 129f33d..187f7ea 100644 --- a/README.en.md +++ b/README.en.md @@ -169,16 +169,23 @@ When `endpoint` is set, the widget POSTs the payload to that URL. A local receiv node server/receive.js ``` -- Accepts payload at `POST /feedback`, appending to `server/feedback.json` by default +- Accepts payload at `POST /feedback`, storing it in `server/feedback.db` (sqlite) by default - Imports download-mode JSON bundles at `POST /import`, storing them in the same inbox format - Renders an inbox of received feedback at `GET /` -- The inbox has text search plus status / kind / reviewer / source / Slack filters -- Each feedback has a triage status (`new` / `accepted` / `fixed` / `ignored`) editable from the card; statuses persist to `server/feedback.json` +- The inbox has text search plus status / kind / project / demo / reviewer / source / Slack filters +- Each feedback has a triage status (`new` / `accepted` / `fixed` / `ignored`) editable from the card; statuses persist to sqlite - `POST /feedback/:id/status` updates the status via the API (body: `{"status": "accepted"}`) +- `DELETE /feedback/:id` removes a feedback and its screenshot (also from the card's delete button) - With GitHub configured, each inbox card can create a GitHub Issue (see below) - Imports `.patchloop-feedback.json` files from the inbox UI -- Returns the raw JSON at `GET /feedback.json` +- Returns the raw JSON at `GET /feedback.json` (filterable via `?projectId=` / `?demoId=` / `?status=`) - Serves saved screenshots from `GET /screenshots/:file` + +### Storage + +Feedback is stored in the built-in `node:sqlite` (`server/feedback.db`). The backend sits behind a small async interface in `server/store.js` (`init` / `insert` / `get` / `list` / `update` / `delete` / `count`), so it can be swapped for another backend (e.g. MySQL) later without changing the receiver. + +On startup, an existing legacy `server/feedback.json` is migrated into sqlite once and archived to `feedback.json.migrated-` (or `feedback.json.corrupt-` if unreadable, starting empty). The db path is set via `FEEDBACK_DB_PATH` and the migration source via `FEEDBACK_STORE_PATH`. - Configurable via `PORT` / `HOST` env (default `127.0.0.1:4000`) - Configurable storage path via `FEEDBACK_STORE_PATH` - Configurable screenshot directory via `SCREENSHOT_DIR` diff --git a/README.md b/README.md index 3ab2a81..73a51f0 100644 --- a/README.md +++ b/README.md @@ -169,16 +169,23 @@ submit のたびに `document` で `patchloop:feedback` が発火し、`event.de node server/receive.js ``` -- `POST /feedback` で payload を受け取り、デフォルトでは `server/feedback.json` に追記します +- `POST /feedback` で payload を受け取り、デフォルトでは `server/feedback.db`（sqlite）に保存します - `POST /import` で download mode の JSON bundle を読み込み、通常の inbox と同じ形式で保存します - `GET /` で受信した feedback の一覧（inbox）を表示します -- inbox にはテキスト検索と status / kind / reviewer / source / Slack の絞り込みがあります -- 各 feedback には triage status（`new` / `accepted` / `fixed` / `ignored`）があり、card 上の select から変更できます。status は `server/feedback.json` に永続化されます +- inbox にはテキスト検索と status / kind / project / demo / reviewer / source / Slack の絞り込みがあります +- 各 feedback には triage status（`new` / `accepted` / `fixed` / `ignored`）があり、card 上の select から変更できます。status は sqlite に永続化されます - `POST /feedback/:id/status` で API からも status を更新できます（body は `{"status": "accepted"}` 形式） +- `DELETE /feedback/:id` で feedback と紐づく screenshot を削除できます（inbox の card の「削除」ボタンからも） - GitHub 連携を設定すると、inbox の各 card から GitHub Issue を作成できます（後述） - inbox UI から `.patchloop-feedback.json` を選択して import できます -- `GET /feedback.json` で raw JSON を返します +- `GET /feedback.json` で raw JSON を返します（`?projectId=` / `?demoId=` / `?status=` で絞り込み可） - `GET /screenshots/:file` で保存済み screenshot を返します + +### ストレージ + +feedback は組み込みの `node:sqlite`（`server/feedback.db`）に保存します。バックエンドは `server/store.js` の小さな非同期インターフェース（`init` / `insert` / `get` / `list` / `update` / `delete` / `count`）の背後に隔離してあり、将来 MySQL 等の別バックエンドに差し替えても receiver 本体は変更不要です。 + +起動時に旧形式の `server/feedback.json` が残っていれば一度だけ sqlite に取り込み、元ファイルは `feedback.json.migrated-` に退避します（壊れていれば `feedback.json.corrupt-` に退避して空で起動）。db ファイルのパスは `FEEDBACK_DB_PATH`、移行元の JSON は `FEEDBACK_STORE_PATH` で指定できます。 - `PORT` / `HOST` env で変更可能（デフォルトは `127.0.0.1:4000`） - `FEEDBACK_STORE_PATH` env で保存先を変更できます - `SCREENSHOT_DIR` env で screenshot 保存先を変更できます diff --git a/server/receive.js b/server/receive.js index 10c7485..d0860b9 100644 --- a/server/receive.js +++ b/server/receive.js @@ -7,6 +7,7 @@ const path = require("path"); const crypto = require("crypto"); const { truncateText, present, escapeHtml, slackEscape, formatSlackCode, formatSlackLink, formatViewport, formatTarget } = require("../shared/format.js"); +const { createStore } = require("./store.js"); const CONFIG_PATH = process.env.PATCHLOOP_RECEIVER_CONFIG || path.join(__dirname, "receiver.config.json"); const config = loadConfig(CONFIG_PATH); @@ -14,7 +15,8 @@ const configDir = path.dirname(CONFIG_PATH); const PORT = numberSetting(process.env.PORT, numberSetting(config.port, 4000)); const HOST = process.env.HOST || config.host || "127.0.0.1"; -const STORE_PATH = process.env.FEEDBACK_STORE_PATH || pathFromConfig(config.feedbackStorePath, path.join(__dirname, "feedback.json")); +const LEGACY_STORE_PATH = process.env.FEEDBACK_STORE_PATH || pathFromConfig(config.feedbackStorePath, path.join(__dirname, "feedback.json")); +const DB_PATH = process.env.FEEDBACK_DB_PATH || pathFromConfig(config.feedbackDbPath, path.join(__dirname, "feedback.db")); const MAX_BODY_BYTES = numberSetting(process.env.MAX_BODY_BYTES, numberSetting(config.maxBodyBytes, 3_000_000)); const SCREENSHOT_DIR = process.env.SCREENSHOT_DIR || pathFromConfig(config.screenshotDir, path.join(__dirname, "screenshots")); const SCREENSHOT_MAX_BYTES = numberSetting(process.env.SCREENSHOT_MAX_BYTES, numberSetting(config.screenshotMaxBytes, 1_500_000)); @@ -40,7 +42,7 @@ const FEEDBACK_STATUSES = ["new", "accepted", "fixed", "ignored"]; // so every stored item carries a version going forward. const DEFAULT_SCHEMA_VERSION = 1; -let feedback = loadFeedback(); +let store; const server = http.createServer((req, res) => { setCors(res); @@ -117,15 +119,27 @@ const server = http.createServer((req, res) => { res.end("Not Found"); }); -server.listen(PORT, HOST, () => { - console.log(`[PatchLoop receiver] listening on http://${HOST}:${PORT}`); - console.log(`[PatchLoop receiver] config file: ${config.__loaded ? CONFIG_PATH : "not loaded"}`); - console.log(`[PatchLoop receiver] feedback file: ${STORE_PATH}`); - console.log(`[PatchLoop receiver] screenshot dir: ${SCREENSHOT_DIR}`); - console.log(`[PatchLoop receiver] Slack webhook: ${SLACK_WEBHOOK_URL ? "enabled" : "disabled"}`); - console.log(`[PatchLoop receiver] Slack image mode: ${SLACK_IMAGE_MODE}`); - console.log(`[PatchLoop receiver] Slack file upload: ${SLACK_BOT_TOKEN && SLACK_UPLOAD_CHANNEL_ID ? "enabled" : "disabled"}`); - console.log(`[PatchLoop receiver] GitHub issues: ${GITHUB_CONFIGURED ? `enabled (${GITHUB_REPO})` : "disabled"}`); +// Storage is initialized (and the legacy JSON store migrated) before the +// server accepts requests, so no handler can run against an unready store. +async function start() { + store = createStore({ dbPath: DB_PATH, legacyJsonPath: LEGACY_STORE_PATH }); + await store.init(); + + server.listen(PORT, HOST, () => { + console.log(`[PatchLoop receiver] listening on http://${HOST}:${PORT}`); + console.log(`[PatchLoop receiver] config file: ${config.__loaded ? CONFIG_PATH : "not loaded"}`); + console.log(`[PatchLoop receiver] feedback db: ${DB_PATH}`); + console.log(`[PatchLoop receiver] screenshot dir: ${SCREENSHOT_DIR}`); + console.log(`[PatchLoop receiver] Slack webhook: ${SLACK_WEBHOOK_URL ? "enabled" : "disabled"}`); + console.log(`[PatchLoop receiver] Slack image mode: ${SLACK_IMAGE_MODE}`); + console.log(`[PatchLoop receiver] Slack file upload: ${SLACK_BOT_TOKEN && SLACK_UPLOAD_CHANNEL_ID ? "enabled" : "disabled"}`); + console.log(`[PatchLoop receiver] GitHub issues: ${GITHUB_CONFIGURED ? `enabled (${GITHUB_REPO})` : "disabled"}`); + }); +} + +start().catch((error) => { + console.error(`[PatchLoop receiver] failed to start: ${error.message}`); + process.exit(1); }); function setCors(res) { @@ -203,8 +217,7 @@ function handlePostFeedback(req, res) { ...(stored.integrations || {}), slack: await deliverToSlack(stored) }; - feedback.unshift(stored); - persist(); + await store.insert(stored); const slackLog = stored.integrations.slack.status === "disabled" ? "" : ` slack=${stored.integrations.slack.status}`; @@ -212,7 +225,7 @@ function handlePostFeedback(req, res) { respondJson(res, 201, { ok: true, id: payload?.id, - count: feedback.length, + count: await store.count(), slack: stored.integrations.slack }); }); @@ -243,31 +256,34 @@ function handlePostImport(req, res) { } }; - feedback.unshift(stored); - persist(); + await store.insert(stored); console.log(`[PatchLoop receiver] imported feedback id=${stored.id || "?"} comment="${truncateText(stored.comment || "", 60)}"`); respondJson(res, 201, { ok: true, id: stored.id, - count: feedback.length, + count: await store.count(), source: "import" }); }); } async function handleDeleteFeedback(req, res, id) { - const index = feedback.findIndex((entry) => entry.id === id); - if (index === -1) { + let removed; + try { + removed = await store.delete(id); + } catch (error) { + respondJson(res, 500, { ok: false, error: error.message }); + return; + } + if (!removed) { respondJson(res, 404, { ok: false, error: `Unknown feedback id: ${id}` }); return; } - const [removed] = feedback.splice(index, 1); - persist(); // Await the file removal so a 200 means the screenshot is gone too. await deleteScreenshotFile(removed.screenshot); console.log(`[PatchLoop receiver] deleted feedback id=${id}`); - respondJson(res, 200, { ok: true, id, count: feedback.length }); + respondJson(res, 200, { ok: true, id, count: await store.count() }); } // Removes the stored screenshot for a deleted feedback. Confined to @@ -294,15 +310,12 @@ function handlePostStatus(req, res, id) { return; } - const item = feedback.find((entry) => entry.id === id); - if (!item) { + const updated = await store.update(id, { status, statusUpdatedAt: new Date().toISOString() }); + if (!updated) { respondJson(res, 404, { ok: false, error: `Unknown feedback id: ${id}` }); return; } - item.status = status; - item.statusUpdatedAt = new Date().toISOString(); - persist(); respondJson(res, 200, { ok: true, id, status }); }); } @@ -314,7 +327,7 @@ function handlePostGitHubIssue(req, res, id) { return; } - const item = feedback.find((entry) => entry.id === id); + const item = await store.get(id); if (!item) { respondJson(res, 404, { ok: false, error: `Unknown feedback id: ${id}` }); return; @@ -327,8 +340,7 @@ function handlePostGitHubIssue(req, res, id) { } const github = await createGitHubIssue(item); - item.integrations = { ...(item.integrations || {}), github }; - persist(); + await store.update(id, { integrations: { ...(item.integrations || {}), github } }); console.log(`[PatchLoop receiver] github issue ${github.status} id=${id}${github.url ? ` url=${github.url}` : ""}`); if (github.status === "created") { @@ -557,14 +569,21 @@ function requireNonEmptyString(value, label) { } } -function handleGetInbox(req, res) { - res.writeHead(200, { "Content-Type": "text/html; charset=utf-8" }); - res.end(renderInbox(feedback)); +async function handleGetInbox(req, res) { + try { + const items = await store.list({}); + res.writeHead(200, { "Content-Type": "text/html; charset=utf-8" }); + res.end(renderInbox(items)); + } catch (error) { + res.writeHead(500, { "Content-Type": "text/plain; charset=utf-8" }); + res.end("Internal Server Error"); + console.warn(`[PatchLoop receiver] inbox render failed: ${error.message}`); + } } // GET /feedback.json supports ?projectId=&demoId=&status= to narrow the // export, so a shared receiver can hand each project just its own feedback. -function handleGetFeedbackJson(req, res) { +async function handleGetFeedbackJson(req, res) { let params; try { params = new URL(req.url, "http://localhost").searchParams; @@ -582,12 +601,15 @@ function handleGetFeedbackJson(req, res) { return; } - const filtered = feedback.filter((item) => - (projectId == null || (item.projectId || "") === projectId) - && (demoId == null || (item.demoId || "") === demoId) - && (status == null || feedbackStatusOf(item) === status)); - - respondJson(res, 200, filtered); + try { + const filter = {}; + if (projectId != null) filter.projectId = projectId; + if (demoId != null) filter.demoId = demoId; + if (status != null) filter.status = status; + respondJson(res, 200, await store.list(filter)); + } catch (error) { + respondJson(res, 500, { ok: false, error: error.message }); + } } function handleGetWidgetScript(req, res) { @@ -671,45 +693,6 @@ function handleGetScreenshot(req, res) { }); } -// An unreadable store must never be silently replaced: persist() rewrites the -// whole file, so starting from [] would destroy all previous feedback on the -// next write. Move the broken file aside and start fresh instead. -function loadFeedback() { - let raw; - try { - raw = fs.readFileSync(STORE_PATH, "utf8"); - } catch (error) { - if (error.code !== "ENOENT") { - console.warn(`[PatchLoop receiver] feedback store unreadable: ${error.message}`); - } - return []; - } - - try { - const parsed = JSON.parse(raw); - if (!Array.isArray(parsed)) throw new Error("store content is not an array"); - return parsed; - } catch (error) { - const backupPath = `${STORE_PATH}.corrupt-${Date.now()}`; - try { - fs.renameSync(STORE_PATH, backupPath); - console.warn(`[PatchLoop receiver] feedback store corrupt (${error.message}); backed up to ${backupPath}`); - } catch (backupError) { - console.warn(`[PatchLoop receiver] feedback store corrupt and backup failed: ${backupError.message}`); - } - return []; - } -} - -// Write-then-rename keeps the store readable even if the process dies -// mid-write; a torn direct write would corrupt the only copy. -function persist() { - fs.mkdirSync(path.dirname(STORE_PATH), { recursive: true }); - const tempPath = `${STORE_PATH}.tmp`; - fs.writeFileSync(tempPath, JSON.stringify(feedback, null, 2)); - fs.renameSync(tempPath, STORE_PATH); -} - function saveScreenshot(screenshot, id) { if (!screenshot) return null; diff --git a/server/store.js b/server/store.js new file mode 100644 index 0000000..0d22474 --- /dev/null +++ b/server/store.js @@ -0,0 +1,186 @@ +"use strict"; + +// Feedback persistence, isolated behind a small async interface so the +// backend can be swapped (sqlite now, a remote DB like MySQL later) without +// touching the receiver. The receiver only ever calls the methods documented +// in the Store contract below. +// +// Store contract (all methods async so a network-backed driver can implement +// the same shape): +// init() prepare storage; migrate a legacy JSON store once +// insert(item) persist a new feedback object +// get(id) -> item|null +// list({projectId, demoId, status}) -> item[] newest first; filters are optional +// update(id, patch) -> item|null shallow-merge patch into the stored item +// delete(id) -> item|null returns the removed item (for screenshot cleanup) +// count() -> number +// close() +// +// A new backend (e.g. createMysqlStore) just needs to implement this shape and +// be wired into createStore() below; the receiver code stays unchanged. + +const fs = require("fs"); +const path = require("path"); + +// node:sqlite is still flagged experimental and prints a warning on load. +// Swallow only that one line so the receiver console stays clean; everything +// else keeps its default handler. +const originalEmitWarning = process.emitWarning; +process.emitWarning = function (warning, ...rest) { + const text = typeof warning === "string" ? warning : warning && warning.message; + if (text && text.includes("SQLite is an experimental feature")) return; + return originalEmitWarning.call(process, warning, ...rest); +}; +const { DatabaseSync } = require("node:sqlite"); + +const VALID_STATUSES = ["new", "accepted", "fixed", "ignored"]; + +function normalizeStatus(value) { + return VALID_STATUSES.includes(value) ? value : "new"; +} + +// Columns extracted from each feedback object for indexed filtering. The full +// object always round-trips through the JSON `data` column, so adding fields to +// the payload never needs a schema change — only new filterable fields do. +function extractColumns(item) { + return { + id: String(item.id), + project_id: item.projectId == null ? null : String(item.projectId), + demo_id: item.demoId == null ? null : String(item.demoId), + status: normalizeStatus(item.status), + received_at: item.receivedAt == null ? null : String(item.receivedAt) + }; +} + +function createSqliteStore({ dbPath, legacyJsonPath }) { + let db; + + function migrateLegacyJson() { + if (!legacyJsonPath || !fs.existsSync(legacyJsonPath)) return; + + let items; + try { + const parsed = JSON.parse(fs.readFileSync(legacyJsonPath, "utf8")); + if (!Array.isArray(parsed)) throw new Error("legacy store is not an array"); + items = parsed; + } catch (error) { + // Never discard an unreadable store silently: set it aside so a human + // can recover it, then start from an empty database. + const backup = `${legacyJsonPath}.corrupt-${Date.now()}`; + try { + fs.renameSync(legacyJsonPath, backup); + console.warn(`[PatchLoop store] legacy feedback.json corrupt (${error.message}); backed up to ${backup}`); + } catch (backupError) { + console.warn(`[PatchLoop store] legacy feedback.json corrupt and backup failed: ${backupError.message}`); + } + return; + } + + // The JSON array is newest-first; insert oldest-first so seq order (and + // therefore newest-first reads) matches what the array represented. + for (const item of items.slice().reverse()) { + if (item && item.id != null) insertRow(item); + } + const archived = `${legacyJsonPath}.migrated-${Date.now()}`; + try { + fs.renameSync(legacyJsonPath, archived); + console.log(`[PatchLoop store] migrated ${items.length} feedback item(s) from ${legacyJsonPath} -> sqlite (archived to ${archived})`); + } catch (error) { + console.warn(`[PatchLoop store] migrated legacy store but could not archive it: ${error.message}`); + } + } + + function insertRow(item) { + const cols = extractColumns(item); + db.prepare( + "INSERT OR REPLACE INTO feedback (id, project_id, demo_id, status, received_at, data) VALUES (?, ?, ?, ?, ?, ?)" + ).run(cols.id, cols.project_id, cols.demo_id, cols.status, cols.received_at, JSON.stringify(item)); + } + + return { + async init() { + fs.mkdirSync(path.dirname(dbPath), { recursive: true }); + db = new DatabaseSync(dbPath); + db.exec(` + CREATE TABLE IF NOT EXISTS feedback ( + seq INTEGER PRIMARY KEY AUTOINCREMENT, + id TEXT UNIQUE NOT NULL, + project_id TEXT, + demo_id TEXT, + status TEXT, + received_at TEXT, + data TEXT NOT NULL + ) + `); + const existing = db.prepare("SELECT COUNT(*) AS n FROM feedback").get().n; + if (existing === 0) migrateLegacyJson(); + }, + + async insert(item) { + insertRow(item); + }, + + async get(id) { + const row = db.prepare("SELECT data FROM feedback WHERE id = ?").get(String(id)); + return row ? JSON.parse(row.data) : null; + }, + + async list(filter = {}) { + const clauses = []; + const params = []; + if (filter.projectId != null) { + clauses.push("project_id = ?"); + params.push(String(filter.projectId)); + } + if (filter.demoId != null) { + clauses.push("demo_id = ?"); + params.push(String(filter.demoId)); + } + if (filter.status != null) { + clauses.push("status = ?"); + params.push(String(filter.status)); + } + const where = clauses.length ? ` WHERE ${clauses.join(" AND ")}` : ""; + const rows = db.prepare(`SELECT data FROM feedback${where} ORDER BY seq DESC`).all(...params); + return rows.map((row) => JSON.parse(row.data)); + }, + + async update(id, patch) { + const current = await this.get(id); + if (!current) return null; + const updated = { ...current, ...patch }; + const cols = extractColumns(updated); + db.prepare( + "UPDATE feedback SET project_id = ?, demo_id = ?, status = ?, received_at = ?, data = ? WHERE id = ?" + ).run(cols.project_id, cols.demo_id, cols.status, cols.received_at, JSON.stringify(updated), String(id)); + return updated; + }, + + async delete(id) { + const current = await this.get(id); + if (!current) return null; + db.prepare("DELETE FROM feedback WHERE id = ?").run(String(id)); + return current; + }, + + async count() { + return db.prepare("SELECT COUNT(*) AS n FROM feedback").get().n; + }, + + async close() { + if (db) db.close(); + } + }; +} + +function createStore(config = {}) { + const backend = config.backend || "sqlite"; + if (backend === "sqlite") { + return createSqliteStore(config); + } + // Future backends (e.g. "mysql") implement the same Store contract and are + // wired in here; the receiver does not change. + throw new Error(`Unknown store backend: ${backend}`); +} + +module.exports = { createStore }; diff --git a/test/receiver.test.js b/test/receiver.test.js index df54acb..724732c 100644 --- a/test/receiver.test.js +++ b/test/receiver.test.js @@ -8,6 +8,7 @@ const net = require("node:net"); const os = require("node:os"); const path = require("node:path"); const test = require("node:test"); +const { DatabaseSync } = require("node:sqlite"); const RECEIVER_PATH = path.resolve(__dirname, "../server/receive.js"); @@ -23,7 +24,7 @@ test("POST /feedback stores valid feedback and saves screenshot data URLs", asyn assert.equal(response.body.count, 1); assert.deepEqual(response.body.slack, { status: "disabled" }); - const stored = await readStoredFeedback(receiver.storePath); + const stored = await readStoredFeedback(receiver.dbPath); assert.equal(stored.length, 1); assert.equal(stored[0].id, payload.id); assert.equal(stored[0].screenshot.status, "saved"); @@ -50,7 +51,7 @@ test("POST /feedback rejects malformed feedback payloads", async (t) => { assert.equal(response.status, 400); assert.equal(response.body.ok, false); assert.match(response.body.error, /feedback\.reviewer must not be empty/); - assert.deepEqual(await readStoredFeedback(receiver.storePath), []); + assert.deepEqual(await readStoredFeedback(receiver.dbPath), []); }); test("POST /import stores bundle feedback and strips delivery metadata", async (t) => { @@ -76,7 +77,7 @@ test("POST /import stores bundle feedback and strips delivery metadata", async ( assert.equal(response.body.id, payload.id); assert.equal(response.body.source, "import"); - const stored = await readStoredFeedback(receiver.storePath); + const stored = await readStoredFeedback(receiver.dbPath); assert.equal(stored.length, 1); assert.equal(stored[0].id, payload.id); assert.equal(stored[0].source, "import"); @@ -100,7 +101,7 @@ test("POST /import rejects unsupported bundle versions", async (t) => { assert.equal(response.status, 400); assert.equal(response.body.ok, false); assert.match(response.body.error, /Unsupported PatchLoop bundle version: 999/); - assert.deepEqual(await readStoredFeedback(receiver.storePath), []); + assert.deepEqual(await readStoredFeedback(receiver.dbPath), []); }); test("POST /feedback/:id/status updates triage status and persists it", async (t) => { @@ -113,7 +114,7 @@ test("POST /feedback/:id/status updates triage status and persists it", async (t assert.equal(response.status, 200); assert.deepEqual(response.body, { ok: true, id: payload.id, status: "accepted" }); - const stored = await readStoredFeedback(receiver.storePath); + const stored = await readStoredFeedback(receiver.dbPath); assert.equal(stored[0].status, "accepted"); assert.match(stored[0].statusUpdatedAt, /^\d{4}-\d{2}-\d{2}T/); }); @@ -151,7 +152,7 @@ test("POST /feedback/:id/github-issue creates an issue via the GitHub API", asyn assert.deepEqual(request.body.labels, ["feedback", "patchloop"]); assert.deepEqual(request.body.assignees, ["kosako"]); - const stored = await readStoredFeedback(receiver.storePath); + const stored = await readStoredFeedback(receiver.dbPath); assert.equal(stored[0].integrations.github.status, "created"); assert.equal(stored[0].integrations.github.issueNumber, 7); @@ -184,7 +185,7 @@ test("POST /feedback/:id/github-issue persists failures and requires configurati assert.equal(failed.status, 502); assert.match(failed.body.error, /Validation Failed/); - const stored = await readStoredFeedback(receiver.storePath); + const stored = await readStoredFeedback(receiver.dbPath); assert.equal(stored[0].integrations.github.status, "failed"); assert.equal(stored[0].integrations.github.statusCode, 422); }); @@ -268,23 +269,28 @@ test("POST /feedback/:id/status rejects unknown statuses and ids", async (t) => assert.equal(missing.status, 404); assert.match(missing.body.error, /Unknown feedback id/); - const stored = await readStoredFeedback(receiver.storePath); + const stored = await readStoredFeedback(receiver.dbPath); assert.equal(stored[0].status, undefined); }); -test("corrupt feedback store is backed up instead of overwritten", async (t) => { +test("corrupt legacy feedback store is backed up, not migrated, during startup", async (t) => { const tempDir = await fs.mkdtemp(path.join(os.tmpdir(), "patchloop-receiver-test-")); const storePath = path.join(tempDir, "feedback.json"); const corruptContent = '[{"id": "pl_old", "comment": "truncated...'; await fs.writeFile(storePath, corruptContent); t.after(() => fs.rm(tempDir, { recursive: true, force: true })); - const receiver = await startReceiver(t, { FEEDBACK_STORE_PATH: storePath }); + // The db lives in the same dir so we can confirm migration started empty. + const receiver = await startReceiver(t, { + FEEDBACK_STORE_PATH: storePath, + FEEDBACK_DB_PATH: path.join(tempDir, "feedback.db") + }); const payload = feedbackPayload("pl_after_corruption"); const response = await postJson(`${receiver.baseUrl}/feedback`, payload); assert.equal(response.status, 201); - const stored = await readStoredFeedback(storePath); + // Only the new item exists; the corrupt legacy rows were not imported. + const stored = await readStoredFeedback(path.join(tempDir, "feedback.db")); assert.equal(stored.length, 1); assert.equal(stored[0].id, payload.id); @@ -292,7 +298,30 @@ test("corrupt feedback store is backed up instead of overwritten", async (t) => const backup = entries.find((name) => name.startsWith("feedback.json.corrupt-")); assert.ok(backup, `expected a corrupt backup file, found: ${entries.join(", ")}`); assert.equal(await fs.readFile(path.join(tempDir, backup), "utf8"), corruptContent); - assert.ok(!entries.includes("feedback.json.tmp"), "temp file should not linger after persist"); +}); + +test("a valid legacy feedback.json is migrated into sqlite on startup", async (t) => { + const tempDir = await fs.mkdtemp(path.join(os.tmpdir(), "patchloop-receiver-test-")); + const storePath = path.join(tempDir, "feedback.json"); + const dbPath = path.join(tempDir, "feedback.db"); + // newest-first on disk, as the JSON store was written + const legacy = [feedbackPayload("pl_legacy_new"), feedbackPayload("pl_legacy_old")]; + await fs.writeFile(storePath, JSON.stringify(legacy, null, 2)); + t.after(() => fs.rm(tempDir, { recursive: true, force: true })); + + const receiver = await startReceiver(t, { FEEDBACK_STORE_PATH: storePath, FEEDBACK_DB_PATH: dbPath }); + + const stored = await readStoredFeedback(dbPath); + assert.deepEqual(stored.map((item) => item.id), ["pl_legacy_new", "pl_legacy_old"]); + + // the original is archived, not left in place to re-import + const entries = await fs.readdir(tempDir); + assert.ok(!entries.includes("feedback.json"), "legacy store should be archived after migration"); + assert.ok(entries.some((name) => name.startsWith("feedback.json.migrated-")), `expected a migrated archive, found: ${entries.join(", ")}`); + + // the API serves the migrated rows + const served = await fetch(`${receiver.baseUrl}/feedback.json`).then((r) => r.json()); + assert.deepEqual(served.map((item) => item.id), ["pl_legacy_new", "pl_legacy_old"]); }); test("GET /widget.js serves the built widget bundle", async (t) => { @@ -381,7 +410,7 @@ test("upload-only Slack config reports skipped, not failed, without a screenshot const response = await postJson(`${receiver.baseUrl}/feedback`, payload); assert.equal(response.status, 201); - const stored = await readStoredFeedback(receiver.storePath); + const stored = await readStoredFeedback(receiver.dbPath); assert.equal(stored[0].integrations.slack.status, "skipped"); }); @@ -398,7 +427,7 @@ test("schemaVersion is stored, defaulted for legacy payloads, and validated", as delete legacy.schemaVersion; await postJson(`${receiver.baseUrl}/feedback`, legacy); - const stored = await readStoredFeedback(receiver.storePath); + const stored = await readStoredFeedback(receiver.dbPath); const byId = Object.fromEntries(stored.map((item) => [item.id, item])); assert.equal(byId.pl_schema_1.schemaVersion, 1); assert.equal(byId.pl_schema_legacy.schemaVersion, 1); @@ -461,7 +490,7 @@ test("DELETE /feedback/:id removes the item and its screenshot file", async (t) await postJson(`${receiver.baseUrl}/feedback`, payload); await postJson(`${receiver.baseUrl}/feedback`, feedbackPayload("pl_delete_keep")); - const before = await readStoredFeedback(receiver.storePath); + const before = await readStoredFeedback(receiver.dbPath); const screenshotPath = before.find((item) => item.id === "pl_delete_1").screenshot.path; await fs.access(screenshotPath); // exists before delete @@ -470,7 +499,7 @@ test("DELETE /feedback/:id removes the item and its screenshot file", async (t) const body = await response.json(); assert.deepEqual(body, { ok: true, id: "pl_delete_1", count: 1 }); - const after = await readStoredFeedback(receiver.storePath); + const after = await readStoredFeedback(receiver.dbPath); assert.deepEqual(after.map((item) => item.id), ["pl_delete_keep"]); await assert.rejects(fs.access(screenshotPath), /ENOENT/); }); @@ -483,7 +512,7 @@ test("DELETE /feedback/:id returns 404 for an unknown id", async (t) => { assert.equal(response.status, 404); assert.match((await response.json()).error, /Unknown feedback id/); - const stored = await readStoredFeedback(receiver.storePath); + const stored = await readStoredFeedback(receiver.dbPath); assert.equal(stored.length, 1); }); @@ -499,6 +528,7 @@ async function startReceiver(t, extraEnv = {}) { const port = await getFreePort(); const baseUrl = `http://127.0.0.1:${port}`; const storePath = path.join(tempDir, "feedback.json"); + const dbPath = path.join(tempDir, "feedback.db"); const screenshotDir = path.join(tempDir, "screenshots"); const child = spawn(process.execPath, [RECEIVER_PATH], { @@ -507,6 +537,7 @@ async function startReceiver(t, extraEnv = {}) { HOST: "127.0.0.1", PORT: String(port), FEEDBACK_STORE_PATH: storePath, + FEEDBACK_DB_PATH: dbPath, SCREENSHOT_DIR: screenshotDir, PUBLIC_BASE_URL: baseUrl, SLACK_WEBHOOK_URL: "", @@ -534,6 +565,7 @@ async function startReceiver(t, extraEnv = {}) { logs, screenshotDir, storePath, + dbPath, tempDir }; } @@ -618,13 +650,21 @@ async function postJson(url, body) { }; } -async function readStoredFeedback(storePath) { +// Reads the sqlite store directly (newest first, matching store.list) so the +// tests can assert persisted state without going through the HTTP API. +async function readStoredFeedback(dbPath) { try { - return JSON.parse(await fs.readFile(storePath, "utf8")); + await fs.access(dbPath); } catch (error) { if (error.code === "ENOENT") return []; throw error; } + const db = new DatabaseSync(dbPath); + try { + return db.prepare("SELECT data FROM feedback ORDER BY seq DESC").all().map((row) => JSON.parse(row.data)); + } finally { + db.close(); + } } function feedbackPayload(id) { From e6dedc5af44296c624eb9aa2a332f137a8fc66af Mon Sep 17 00:00:00 2001 From: kosako Date: Sun, 14 Jun 2026 23:46:09 +0900 Subject: [PATCH 2/2] Address review on the sqlite store - Reject duplicate ids with 409 instead of INSERT OR REPLACE silently overwriting an existing row - Make update/delete atomic by reading synchronously (no await between read and write), closing the lost-update window - Treat empty query filter values as "no filter" so NULL-column rows still match, matching the inbox "all" option - Wrap the legacy migration in a transaction so an interruption rolls back and the next startup retries instead of stranding rows - Restore process.emitWarning right after requiring node:sqlite Co-Authored-By: Claude Fable 5 --- server/receive.js | 8 ++++-- server/store.js | 67 ++++++++++++++++++++++++++++++++----------- test/receiver.test.js | 17 +++++++++++ 3 files changed, 73 insertions(+), 19 deletions(-) diff --git a/server/receive.js b/server/receive.js index d0860b9..2e80be5 100644 --- a/server/receive.js +++ b/server/receive.js @@ -592,9 +592,11 @@ async function handleGetFeedbackJson(req, res) { return; } - const projectId = params.get("projectId"); - const demoId = params.get("demoId"); - const status = params.get("status"); + // Empty query values mean "no filter" (matching the inbox's "all" option), + // not "items whose field equals the empty string". + const projectId = params.get("projectId") || null; + const demoId = params.get("demoId") || null; + const status = params.get("status") || null; if (status != null && !FEEDBACK_STATUSES.includes(status)) { respondJson(res, 400, { ok: false, error: `status must be one of: ${FEEDBACK_STATUSES.join(", ")}` }); diff --git a/server/store.js b/server/store.js index 0d22474..c3febf3 100644 --- a/server/store.js +++ b/server/store.js @@ -22,9 +22,9 @@ const fs = require("fs"); const path = require("path"); -// node:sqlite is still flagged experimental and prints a warning on load. -// Swallow only that one line so the receiver console stays clean; everything -// else keeps its default handler. +// node:sqlite is still flagged experimental and prints a warning when it +// loads. Swallow only that one line, and restore the original handler right +// after the require so the override does not stay in effect process-wide. const originalEmitWarning = process.emitWarning; process.emitWarning = function (warning, ...rest) { const text = typeof warning === "string" ? warning : warning && warning.message; @@ -32,6 +32,11 @@ process.emitWarning = function (warning, ...rest) { return originalEmitWarning.call(process, warning, ...rest); }; const { DatabaseSync } = require("node:sqlite"); +process.emitWarning = originalEmitWarning; + +function isUniqueViolation(error) { + return /UNIQUE constraint failed/i.test(error && error.message); +} const VALID_STATUSES = ["new", "accepted", "fixed", "ignored"]; @@ -76,11 +81,25 @@ function createSqliteStore({ dbPath, legacyJsonPath }) { return; } - // The JSON array is newest-first; insert oldest-first so seq order (and - // therefore newest-first reads) matches what the array represented. - for (const item of items.slice().reverse()) { - if (item && item.id != null) insertRow(item); + // All-or-nothing: a migration interrupted partway would leave rows behind, + // and the next startup (seeing a non-empty table) would skip migration and + // strand the remaining legacy items. A transaction rolls back on failure so + // the legacy file stays intact and the next startup retries. + db.exec("BEGIN"); + try { + // The JSON array is newest-first; insert oldest-first so seq order (and + // therefore newest-first reads) matches what the array represented. + // OR IGNORE keeps the first of any duplicate legacy ids instead of + // aborting the whole migration. + for (const item of items.slice().reverse()) { + if (item && item.id != null) insertRow(item, { ignoreConflict: true }); + } + db.exec("COMMIT"); + } catch (error) { + db.exec("ROLLBACK"); + throw error; } + const archived = `${legacyJsonPath}.migrated-${Date.now()}`; try { fs.renameSync(legacyJsonPath, archived); @@ -90,10 +109,11 @@ function createSqliteStore({ dbPath, legacyJsonPath }) { } } - function insertRow(item) { + function insertRow(item, { ignoreConflict = false } = {}) { const cols = extractColumns(item); + const verb = ignoreConflict ? "INSERT OR IGNORE" : "INSERT"; db.prepare( - "INSERT OR REPLACE INTO feedback (id, project_id, demo_id, status, received_at, data) VALUES (?, ?, ?, ?, ?, ?)" + `${verb} INTO feedback (id, project_id, demo_id, status, received_at, data) VALUES (?, ?, ?, ?, ?, ?)` ).run(cols.id, cols.project_id, cols.demo_id, cols.status, cols.received_at, JSON.stringify(item)); } @@ -117,7 +137,19 @@ function createSqliteStore({ dbPath, legacyJsonPath }) { }, async insert(item) { - insertRow(item); + // A duplicate id is anomalous (ids are unique by construction). Reject it + // with a 409 instead of silently overwriting the existing row (data loss) + // or silently appending a second row that breaks id-based triage. + try { + insertRow(item); + } catch (error) { + if (isUniqueViolation(error)) { + const conflict = new Error(`feedback id already exists: ${item.id}`); + conflict.statusCode = 409; + throw conflict; + } + throw error; + } }, async get(id) { @@ -145,10 +177,13 @@ function createSqliteStore({ dbPath, legacyJsonPath }) { return rows.map((row) => JSON.parse(row.data)); }, + // Read and write happen without an await in between, so the whole + // read-modify-write runs synchronously on the single thread and cannot lose + // a concurrent update. A networked backend would wrap this in a transaction. async update(id, patch) { - const current = await this.get(id); - if (!current) return null; - const updated = { ...current, ...patch }; + const row = db.prepare("SELECT data FROM feedback WHERE id = ?").get(String(id)); + if (!row) return null; + const updated = { ...JSON.parse(row.data), ...patch }; const cols = extractColumns(updated); db.prepare( "UPDATE feedback SET project_id = ?, demo_id = ?, status = ?, received_at = ?, data = ? WHERE id = ?" @@ -157,10 +192,10 @@ function createSqliteStore({ dbPath, legacyJsonPath }) { }, async delete(id) { - const current = await this.get(id); - if (!current) return null; + const row = db.prepare("SELECT data FROM feedback WHERE id = ?").get(String(id)); + if (!row) return null; db.prepare("DELETE FROM feedback WHERE id = ?").run(String(id)); - return current; + return JSON.parse(row.data); }, async count() { diff --git a/test/receiver.test.js b/test/receiver.test.js index 724732c..5849788 100644 --- a/test/receiver.test.js +++ b/test/receiver.test.js @@ -523,6 +523,23 @@ test("inbox renders a delete button per card", async (t) => { assert.match(html, /data-delete-feedback data-feedback-id="pl_delete_ui"/); }); +test("POST /feedback rejects a duplicate id instead of overwriting", async (t) => { + const receiver = await startReceiver(t); + const first = feedbackPayload("pl_dup"); + first.comment = "original"; + await postJson(`${receiver.baseUrl}/feedback`, first); + + const second = feedbackPayload("pl_dup"); + second.comment = "should not overwrite"; + const response = await postJson(`${receiver.baseUrl}/feedback`, second); + assert.equal(response.status, 409); + assert.match(response.body.error, /already exists/); + + const stored = await readStoredFeedback(receiver.dbPath); + assert.equal(stored.length, 1); + assert.equal(stored[0].comment, "original"); +}); + async function startReceiver(t, extraEnv = {}) { const tempDir = await fs.mkdtemp(path.join(os.tmpdir(), "patchloop-receiver-test-")); const port = await getFreePort();