Codebase optimization review: TODO list + P0/P1 bounds fixes

OPTIMIZATION_TODOS.md

@@ -1,78 +1,104 @@
 # Optimization TODO list
 
-Full codebase review after the SQLite event-bus migration (`7915613`).
-Priorities: **P0** = correctness / unbounded growth; **P1** = hot-path performance;
-**P2** = medium impact; **P3** = polish / future.
+Full codebase review after floating terminal/browser panels (`f542b35`)
+and prior SQLite/event-bus work. Priorities: **P0** = correctness /
+unbounded growth; **P1** = hot-path performance; **P2** = medium impact;
+**P3** = polish / future.
 
 Stub crates (`retcon-worktrees`, `retcon-browser`, `retcon-checkpoints`,
 `retcon-filesystem`, `retcon-index`, `retcon-platform`, `retcon-permissions`,
-`retcon-diagnostics`, `retcon-updater`) and empty TypeScript packages have no
-meaningful optimization surface yet.
+`retcon-diagnostics`, `retcon-updater`) and empty TypeScript/Dart packages
+have no meaningful optimization surface yet.
 
 ---
 
 ## Done in this PR
 
-- [x] **P0** Cap browser-service console/network buffers with a ring buffer
-  (`apps/browser-service/src/browser.ts`)
-- [x] **P0** Remove timed-out / closed browser RPC entries from `pending`
-  (`crates/retcon-core/src/spikes/browser.rs`)
-- [x] **P0** Cap RPC frame size on read (`crates/retcon-core/src/frame.rs`,
-  `server.rs`, `spikes/browser.rs`)
-- [x] **P0** Periodic SQLite event retention (`crates/retcon-core/src/event.rs`)
-- [x] **P0** Evict finished jobs from memory + DB (`crates/retcon-core/src/jobs.rs`)
-- [x] **P0** Graceful browser-service shutdown before kill (`spikes/browser.rs`)
-- [x] **P0** Bound Git command output (`crates/retcon-git/src/lib.rs`)
-- [x] **P1** Don't hold event-bus mutex across SQLite insert (`event.rs` writer thread)
-- [x] **P1** Batch / coalesce terminal output events (`spikes/terminal.rs`)
-- [x] **P1** Move SQLite work off Tokio worker threads (`database.rs` `read_async`,
-  `artifacts.rs` async helpers)
-- [x] **P1** Add status indexes for recovery queries (migration `0003`)
-- [x] **P1** Enforce log pagination / tail in `browser.logs` (`browser.ts`)
-- [x] **P1** Default screenshot to viewport (`browser.ts`)
-- [x] **P1** Combine Git status into one process (`retcon-git/src/lib.rs`)
-- [x] **P2** Browser proc lock held across stdin writes — writer actor split (`browser.rs`)
-- [x] **P2** Clear stale `BrowserHandle.proc` + drain pending on reader exit (`browser.rs`)
-- [x] **P2** Artifact retention hash indexes (migration `0003`)
-- [x] **P2** Run artifact disk walks in `spawn_blocking` (`artifacts.rs`)
-- [x] **P2** Skip full rehash on duplicate artifact store (`artifacts.rs`)
-- [x] **P2** Binary-search event replay (`event.rs`)
-- [x] **P2** `Arc<EventEnvelope>` to cut clone cost on emit (`event.rs`)
-- [x] **P2** `prepare_cached` for hot statements (`event.rs`, `jobs.rs`)
-- [x] **P2** `PRAGMA quick_check` on normal open; full check for diagnostics (`database.rs`)
-- [x] **P2** `SQLITE_OPEN_NO_MUTEX` when Rust serializes the connection (`database.rs`)
-- [x] **P2** Tune `cache_size` / `mmap_size` / `wal_autocheckpoint` (`database.rs`)
-- [x] **P2** Debounce job persistence; avoid Debug-format enum names (`jobs.rs`)
-- [x] **P2** Don't hold terminal registry lock while killing (`spikes/terminal.rs`)
-- [x] **P2** Cache / async shell detection (`spikes/terminal.rs`)
-- [x] **P2** Cache agent/provider detection (`retcon-agents`)
-- [x] **P2** Await `child.wait()` instead of 250ms poll (`spikes/agent.rs`)
-- [x] **P2** Cache schema version after open (`state.rs`)
-- [x] **P2** Lazy-load event replay pages from DB at startup (`event.rs`)
-- [x] **P2** Pass browser temp profile dir via `launchPersistentContext` (`browser.ts`)
-- [x] **P2** Stdout backpressure on browser-service event emits (`main.ts`)
-- [x] **P2** Spawn `bun` directly (no `cmd /C` shell) on Windows (`spikes/browser.rs`)
-- [x] **P3** Deduplicate console/network stream vs buffer shipping (`browser.ts` — network
-  responses now land in the ring buffer)
-- [x] **P3** Method-aware concurrency for browser-service stdio RPC (`main.ts`)
-- [x] **P3** Cache log level at module load (`logging.ts`)
-- [x] **P3** Website asset generation: single Sharp pipeline + `Promise.all` for icons
-- [x] **P3** Website CLS: explicit image dimensions; defer Tally iframe (`index.astro`)
-- [x] **P3** CI-generate PWA icons referenced by `site.webmanifest` (`package.json` build)
+- [x] **P0** Cap RPC frame reads with chunked `fill_buf` (no unbounded
+  `read_until`) — `crates/retcon-core/src/frame.rs`
+- [x] **P0** Cap Git stderr; kill child on stdout/stderr overflow —
+  `crates/retcon-git/src/lib.rs`
+- [x] **P0** Bound event persist queue (`sync_channel` + `try_send`) —
+  `crates/retcon-core/src/event.rs`
+- [x] **P0** Bound terminal output channel + drop-on-full —
+  `crates/retcon-core/src/spikes/terminal.rs`
+- [x] **P0** Fix agent cancel deadlock (don't hold mutex across `wait`) —
+  `crates/retcon-core/src/spikes/agent.rs`
+- [x] **P0** Cap browser-service stdin RPC line size —
+  `apps/browser-service/src/main.ts`
+- [x] **P0** Cap browser console/network/text field bytes —
+  `apps/browser-service/src/browser.ts`
+- [x] **P1** Real stdio concurrency for status/logs vs mutating methods —
+  `apps/browser-service/src/main.ts`
+- [x] **P1** Don't emit `browser.crashed` on intentional close —
+  `apps/browser-service/src/browser.ts`
+- [x] **P1** Default `browser.logs` to latest tail —
+  `apps/browser-service/src/browser.ts`
+- [x] **P1** Workspace restore race / dispose guard —
+  `apps/desktop/lib/src/workspace.dart`
+- [x] **P1** Fix float/close singleton tab replacement bug —
+  `apps/desktop/lib/src/workspace.dart`
+- [x] **P1** Reuse existing floating terminal/browser panels —
+  `apps/desktop/lib/src/workspace.dart`
+- [x] **P1** Wire tab switching + cap closed-panel history —
+  `apps/desktop/lib/src/workspace.dart`
 
 ---
 
-## Completed schema work
+## Still open
 
-- [x] **P3** UUID-as-BLOB schema migration (`retcon-storage` migration `0004`)
-  - Rebuilds v1-v3 databases transactionally, converts every UUID column to compact
-    16-byte BLOBs, recreates indexes, and updates active repository/core bindings.
+### P1
+
+- [ ] **P1** Dedicated job-persistence writer actor (replace sync DB writes
+  / any `block_in_place` on async paths) — `crates/retcon-core/src/jobs.rs`
+- [ ] **P1** Move event prune / lifecycle maintain / artifact cleanup off
+  async request paths (`spawn_blocking` or worker) — `event.rs`,
+  `lifecycle.rs`
+- [ ] **P1** Drain completed RPC `JoinSet` tasks in accept loop; cap
+  concurrent connections — `crates/retcon-core/src/server.rs`
+- [ ] **P1** Browser reader cleanup generation ID (don't clear a newly
+  started process) — `crates/retcon-core/src/spikes/browser.rs`
+- [ ] **P1** Enforce outbound frame size (`MAX_FRAME_BYTES`); align Git
+  output cap with protocol (or page large diffs as artifacts) —
+  `server.rs`, `retcon-git`, `retcon-protocol`
+
+### P2
+
+- [ ] **P2** `jobs.list` ordered index or SQL pagination (avoid clone+sort
+  of full map) — `crates/retcon-core/src/jobs.rs`
+- [ ] **P2** Paginate `SessionRepository::list_for_project` —
+  `crates/retcon-storage/src/repositories.rs`
+- [ ] **P2** Artifact retention: stream/`EXISTS` instead of full hash set —
+  `crates/retcon-storage/src/artifacts.rs`
+- [ ] **P2** `CREATE INDEX … ON agent_events(created_at)` + scheduled age
+  prune (not in emit path) — migrations + `event.rs`
+- [ ] **P2** Attach Playwright listeners to all pages (`context.on("page")`)
+  — `apps/browser-service/src/browser.ts`
+- [ ] **P2** Floating panel instance IDs + `ValueKey` for stateful bodies —
+  `apps/desktop/lib/src/workspace.dart`
+- [ ] **P2** Serialize/coalesce workspace layout persistence —
+  `apps/desktop/lib/src/workspace.dart`
+- [ ] **P2** Fixed-index ring buffer for browser logs (vs `splice`) —
+  `apps/browser-service/src/browser.ts`
+- [ ] **P2** BLOB-native fresh schema (skip TEXT→rebuild on new DBs)
+
+### P3
+
+- [ ] **P3** Website asset generation mtime/hash short-circuit —
+  `apps/website/scripts/generate-brand-assets.mjs`
+- [ ] **P3** Guard redundant focus `setState` in design-system controls —
+  `packages/retcon-design-system/lib/src/components/controls.dart`
+- [ ] **P3** CI Playwright cache + path filters
+- [x] **P3** Update storage/core tests for schema v4 UUID BLOBs
+  (`database.rs`, `artifacts.rs`, `recovery.rs`, `lifecycle.rs`)
 
 ---
 
-## Suggested order of attack (completed)
+## Suggested order of attack
 
-1. Frame-size caps + event retention + job eviction (bounds) ✅
-2. Event writer off async path + terminal coalesce (throughput) ✅
-3. Recovery indexes + Git/browser evidence polish (latency) ✅
-4. Storage pragma / statement-cache tuning (incremental) ✅
+1. Job writer actor + prune off emit path (async latency)
+2. RPC JoinSet drain + connection cap + outbound frame limit
+3. Browser process generation + multi-page listeners
+4. Jobs/sessions pagination + event `created_at` index
+5. Desktop panel instance IDs + save coalescing
+6. Website/CI polish

apps/browser-service/src/browser.ts

@@ -5,6 +5,10 @@ import { type Browser, type BrowserContext, type Page, chromium } from "playwrig
 
 /** Retained console/network evidence per session (ring buffer). */
 export const MAX_LOG_ENTRIES = 1_000;
+/** Per-field text/url byte budget for buffered evidence. */
+export const MAX_LOG_FIELD_CHARS = 4_096;
+/** Cap for `browser.action` text extraction responses. */
+export const MAX_ACTION_TEXT_CHARS = 64_000;
 
 export interface BrowserEvent {
   type: string;
@@ -31,9 +35,16 @@ export function pushCapped<T>(buffer: T[], entry: T, max: number): void {
   }
 }
 
+/** Truncate oversized strings so count-capped buffers stay byte-bounded. */
+export function truncateField(value: string, max = MAX_LOG_FIELD_CHARS): string {
+  if (value.length <= max) return value;
+  return `${value.slice(0, max)}…[truncated ${value.length - max} chars]`;
+}
+
 function sliceLogs<T>(entries: T[], query: LogQuery = {}): T[] {
-  const offset = Math.max(0, query.offset ?? 0);
   const limit = Math.min(Math.max(1, query.limit ?? 100), MAX_LOG_ENTRIES);
+  const defaultOffset = Math.max(0, entries.length - limit);
+  const offset = Math.max(0, query.offset ?? defaultOffset);
   return entries.slice(offset, offset + limit);
 }
 
@@ -42,13 +53,15 @@ export class ManagedBrowser {
   private context: BrowserContext | undefined;
   private page: Page | undefined;
   private profile: string | undefined;
+  private closing = false;
   private readonly consoleEntries: Record<string, unknown>[] = [];
   private readonly networkEntries: Record<string, unknown>[] = [];
 
   constructor(private readonly emit: (event: BrowserEvent) => void) {}
 
   async launch(): Promise<Record<string, unknown>> {
     if (this.context) return { alreadyRunning: true };
+    this.closing = false;
     this.profile = await mkdtemp(join(tmpdir(), "retcon-browser-"));
     const executablePath = process.env.RETCON_CHROMIUM_PATH;
     this.context = await chromium.launchPersistentContext(this.profile, {
@@ -57,31 +70,13 @@ export class ManagedBrowser {
       ...(executablePath ? { executablePath } : {}),
     });
     this.browser = this.context.browser() ?? undefined;
-    this.context.on("close", () => this.emit({ type: "browser.crashed", payload: {} }));
-    this.page = this.context.pages()[0] ?? (await this.context.newPage());
-    this.page.on("console", (message) => {
-      const entry = {
-        type: message.type(),
-        text: message.text(),
-        timestamp: new Date().toISOString(),
-      };
-      pushCapped(this.consoleEntries, entry, MAX_LOG_ENTRIES);
-      this.emit({ type: "browser.console", payload: entry });
-    });
-    this.page.on("request", (request) => {
-      const entry = {
-        method: request.method(),
-        url: request.url(),
-        resourceType: request.resourceType(),
-      };
-      pushCapped(this.networkEntries, entry, MAX_LOG_ENTRIES);
-      this.emit({ type: "browser.request", payload: entry });
-    });
-    this.page.on("response", (response) => {
-      const entry = { status: response.status(), url: response.url() };
-      pushCapped(this.networkEntries, entry, MAX_LOG_ENTRIES);
-      this.emit({ type: "browser.response", payload: entry });
+    this.context.on("close", () => {
+      if (!this.closing) {
+        this.emit({ type: "browser.crashed", payload: {} });
+      }
     });
+    this.page = this.context.pages()[0] ?? (await this.context.newPage());
+    this.attachPageListeners(this.page);
     return { launched: true, profile: this.profile };
   }
 
@@ -121,24 +116,31 @@ export class ManagedBrowser {
       case "press":
         await page.locator(selector).press(String(params.value ?? "Enter"));
         break;
-      case "text":
-        return { text: await page.locator(selector).innerText() };
+      case "text": {
+        const text = await page.locator(selector).innerText();
+        return { text: truncateField(text, MAX_ACTION_TEXT_CHARS) };
+      }
       default:
         throw new Error(`unsupported action: ${String(params.action)}`);
     }
     return { completed: true };
   }
 
   logs(query: LogQuery = {}): Record<string, unknown> {
+    const limit = Math.min(Math.max(1, query.limit ?? 100), MAX_LOG_ENTRIES);
+    const consoleOffset =
+      query.offset ?? Math.max(0, this.consoleEntries.length - limit);
+    const networkOffset =
+      query.offset ?? Math.max(0, this.networkEntries.length - limit);
     return {
-      console: sliceLogs(this.consoleEntries, query),
-      network: sliceLogs(this.networkEntries, query),
+      console: sliceLogs(this.consoleEntries, { ...query, offset: consoleOffset, limit }),
+      network: sliceLogs(this.networkEntries, { ...query, offset: networkOffset, limit }),
       totals: {
         console: this.consoleEntries.length,
         network: this.networkEntries.length,
       },
-      offset: query.offset ?? 0,
-      limit: query.limit ?? 100,
+      offset: query.offset ?? consoleOffset,
+      limit,
     };
   }
 
@@ -150,6 +152,7 @@ export class ManagedBrowser {
   }
 
   async close(): Promise<Record<string, unknown>> {
+    this.closing = true;
     const context = this.context;
     this.browser = undefined;
     this.context = undefined;
@@ -160,6 +163,32 @@ export class ManagedBrowser {
     return { closed: true };
   }
 
+  private attachPageListeners(page: Page): void {
+    page.on("console", (message) => {
+      const entry = {
+        type: message.type(),
+        text: truncateField(message.text()),
+        timestamp: new Date().toISOString(),
+      };
+      pushCapped(this.consoleEntries, entry, MAX_LOG_ENTRIES);
+      this.emit({ type: "browser.console", payload: entry });
+    });
+    page.on("request", (request) => {
+      const entry = {
+        method: request.method(),
+        url: truncateField(request.url()),
+        resourceType: request.resourceType(),
+      };
+      pushCapped(this.networkEntries, entry, MAX_LOG_ENTRIES);
+      this.emit({ type: "browser.request", payload: entry });
+    });
+    page.on("response", (response) => {
+      const entry = { status: response.status(), url: truncateField(response.url()) };
+      pushCapped(this.networkEntries, entry, MAX_LOG_ENTRIES);
+      this.emit({ type: "browser.response", payload: entry });
+    });
+  }
+
   private requirePage(): Page {
     if (!this.page) throw new Error("managed browser is not running");
     return this.page;