diff --git a/.claude/mcp.json b/.claude/mcp.json index b4ee8749..41a4ceb1 100644 --- a/.claude/mcp.json +++ b/.claude/mcp.json @@ -2,7 +2,7 @@ "mcpServers": { "chrome-devtools": { "command": "npx", - "args": ["-y", "chrome-devtools-mcp@latest", "--browser-url=http://127.0.0.1:9222"] + "args": ["-y", "chrome-devtools-mcp@latest", "--browser-url=http://127.0.0.1:9223"] }, "posthog": { "type": "http", diff --git a/.claude/skills/clear-caches b/.claude/skills/clear-caches new file mode 120000 index 00000000..7ab56e9b --- /dev/null +++ b/.claude/skills/clear-caches @@ -0,0 +1 @@ +../../skills/clear-caches \ No newline at end of file diff --git a/.claude/skills/electron-devtools-testing b/.claude/skills/electron-devtools-testing new file mode 120000 index 00000000..87be48c6 --- /dev/null +++ b/.claude/skills/electron-devtools-testing @@ -0,0 +1 @@ +../../skills/electron-devtools-testing \ No newline at end of file diff --git a/.claude/skills/gh-local b/.claude/skills/gh-local new file mode 120000 index 00000000..010c600d --- /dev/null +++ b/.claude/skills/gh-local @@ -0,0 +1 @@ +../../skills/gh-local \ No newline at end of file diff --git a/.claude/skills/github-cli b/.claude/skills/github-cli new file mode 120000 index 00000000..2cd2c1bf --- /dev/null +++ b/.claude/skills/github-cli @@ -0,0 +1 @@ +../../skills/github-cli \ No newline at end of file diff --git a/.claude/skills/review-code/SKILL.md b/.claude/skills/review-code/SKILL.md deleted file mode 100644 index 39ff0ff2..00000000 --- a/.claude/skills/review-code/SKILL.md +++ /dev/null @@ -1,485 +0,0 @@ ---- -name: review-code -description: Pre-commit code review for production-critical issues. Use when reviewing staged changes, before committing, or when asked to review code for bugs and consistency issues. -allowed-tools: Bash, Read, Glob, Grep, Edit, Task, LSP -argument-hint: "[base-branch]" ---- - -Review staged changes for production-critical issues before committing. This catches the kinds of bugs that CI review bots find — type safety violations, IPC contract mismatches, missing references, and frontend/backend inconsistencies. Uses 14 parallel review agents for comprehensive coverage. - -## Scope - -Review the same diff GitHub would show on a PR: all changes on the current branch relative to `origin/main`. This uses the merge base so it only includes changes introduced by this branch, not unrelated commits on main. - -The user may specify a different base branch via $ARGUMENTS (e.g., `/review-code origin/develop`). - -## Process - -### Step 1: Gather Context - -1. Fetch latest remote state: `git fetch origin main` -2. Compute the merge base: `git merge-base origin/main HEAD` -3. Get changed files: `git diff --name-only ` -4. Get the full diff: `git diff ` -5. Categorize the changed files by boundary: main process (`src/main/`), renderer (`src/renderer/`), preload (`src/preload/`), shared types (`src/shared/`), database (`src/main/db/`), tests (`tests/`), agents (`src/agents*/`, `src/extensions*/`). - -### Step 2: Read Full File Context - -For every changed file, read the **full current contents** in parallel (not just the diff hunks). The diff shows what changed, but many issues — missing hook dependencies, stale closures, inconsistent patterns — require surrounding context to detect. - -Also read closely related files in parallel based on file type: -- **React components**: read the store, hooks, and IPC callers it imports from -- **IPC handlers**: read the preload method AND the renderer-side caller AND the DB functions it calls -- **DB functions**: read the schema AND all callers -- **Types/schemas**: grep for all importers -- **Tests**: read the source file AND the test config (`playwright.config.ts` for E2E) - -See `project-specific.md` File Reading Hints for additional codebase-specific file relationships. - -Also read `review-patterns.md` and `project-specific.md` from this skill directory and distribute relevant sections to each agent. - -Use Glob and Grep to find related files efficiently. Issue all file reads in a single batch of parallel Read calls. - -### Step 3: Run 14 Parallel Review Agents - -Create a team with `TeamCreate` (team name: `code-review`), then spawn all 14 agents simultaneously as teammates. Each agent receives the diff, the list of changed files, and the full file contents from Step 2, and returns a list of issues with confidence scores (0-100). Only launch agents whose categories are relevant to the changed files — skip agents that have nothing to check. Run all agents in the background and collect results as they complete. - ---- - -**Agent 1 — Type Safety & References** - -What it checks: -- `any` types introduced in diff (CLAUDE.md violation) -- Type assertions (`as`) that bypass type checking — prefer type guards -- All new imports reference real exports (read source file to verify symbol exists) -- Unused imports added in diff -- Run `npx tsc --noEmit` and report errors in changed files - -See `project-specific.md` Agent 1 section for codebase-specific checks. - -Detection heuristics: -``` -grep for: `as any`, `: any`, `as unknown as`, `(window as any)` -``` - ---- - -**Agent 2 — IPC Contract Consistency** - -There are 5 boundaries that must stay in sync: - -1. **Renderer → Preload**: `window.api.X()` → verify `X` exists in preload -2. **Preload → Main**: `ipcRenderer.invoke('channel:name')` → verify `ipcMain.handle('channel:name')` exists -3. **Main → Preload**: new IPC handler → verify preload exposes it -4. **Types across layers**: preload returns `Promise` → verify renderer checks `.success` before `.data` -5. **Shared types**: changes to shared types → grep all consumers - -See `project-specific.md` Agent 2 section for codebase-specific checks and known pitfalls. - -Detection heuristics: -``` -grep for: ipcMain.handle\( in changed files → verify preload has matching invoke -grep for: window.api\. in renderer → verify exists in preload -grep for: mainWindow.webContents.send\( → verify listener in renderer -``` - ---- - -**Agent 3 — Database & Data Integrity** - -What it checks: -- SQL column names match schema definitions -- DB functions return shapes match all callers -- Missing null/undefined checks on async data -- snake_case (DB) → camelCase (renderer) conversion for new fields -- `INSERT OR REPLACE` resetting flags that should be preserved — use `ON CONFLICT DO UPDATE` with explicit column handling -- `ON CONFLICT` clauses: verify they preserve important fields AND reset fields that should change -- `LIKE '%pattern%'` matching unintended rows — use JSON functions or exact equality -- COALESCE in UPDATE/INSERT preserving stale values after regeneration -- Account scoping: queries must include `accountId` WHERE clause - -See `project-specific.md` Agent 3 section for codebase-specific checks and known pitfalls. - -Detection heuristics: -``` -grep for: INSERT OR REPLACE → verify not resetting flags -grep for: LIKE '%.*%' → verify not matching unintended patterns -grep for: COALESCE → verify not preserving stale values -grep for: UPDATE|DELETE|SELECT without accountId in WHERE -``` - ---- - -**Agent 4 — React Patterns & State Management** - -What it checks: -- **Missing hook dependencies**: For EVERY `useCallback`, `useEffect`, `useMemo`, `useLayoutEffect` in changed files: verify ALL referenced variables are in the dep array -- State that should be derived — flag `useEffect` that syncs one state to another -- Object/array literals inline in JSX (re-render triggers) -- Server state should use react-query, not useState -- **Rules of Hooks violation**: hooks called after conditional early returns — hook count MUST be the same on every render -- **useRef vs useState confusion**: if a value needs to trigger re-render, it must be useState -- **iframe.onload timing**: handler MUST be set BEFORE src is assigned -- **Timer cleanup**: always clear previous timer before setting new one, and clear on component unmount -- **Ref mutations during render body**: writing to refs during render breaks React Concurrent Mode -- **useLayoutEffect vs useEffect ordering**: useLayoutEffect runs BEFORE useEffect in the same render -- **Dependency array boolean expression vs value**: `[arr.length > 0]` should be `[arr.length]` -- **Component reused across data contexts**: when a component is rendered for different data (e.g., different selected items) without remounting, ALL local state must be reset on context switch -- **Parallel code paths for same action**: if an action exists as both a button handler and a keyboard shortcut (or in multiple components), verify they behave identically - -See `project-specific.md` Agent 4 section for codebase-specific checks. - -Detection heuristics: -``` -grep for: useCallback|useEffect|useMemo|useLayoutEffect → read full function body → verify deps -grep for: useRef.*useState.*return null|return → check hook ordering around returns -grep for: iframe.*onload → check if set before src -grep for: setTimeout|setInterval → check for clearTimeout in cleanup -grep for: dangerouslySetInnerHTML → check for DOMPurify (cross-ref with Agent 8) -``` - ---- - -**Agent 5 — Test Quality & Infrastructure** (only if test files changed) - -What it checks: -- Meaningful assertions (not just "it doesn't throw") -- Brittle string-matching assertions (`toContain` on JSX source strings) -- Click elements without visibility/existence checks first -- Error-case tests for async code paths -- Shared module-level state requires `test.describe.configure({ mode: 'serial' })` -- **Virtualizer viewport-bound assertions**: `tanstack-virtual` renders only visible rows — count assertions are viewport-bound -- **Platform-specific shortcuts**: test uses `Meta+k` but CI is Linux (should use `Ctrl+k`) -- **Screenshot directory may not exist**: verify `mkdirSync` with `{ recursive: true }` -- **Test contradicts implementation** -- **Unconditional click on missing element** -- **SIGKILL on already-exited process** - -See `project-specific.md` Agent 5 section for codebase-specific checks. - ---- - -**Agent 6 — Async Logic & Race Conditions** - -What it checks: -- **UI update before API completion**: handlers that update UI state BEFORE awaiting the API call -- **Stale state after await**: reading state variable after an await gap -- **Processing flag not reset**: any flag set to `true` before async op MUST be reset in `finally` block -- **Re-entrancy**: async handlers need guard (`if (processing) return`) -- **Zustand getState() race**: captured state reference may be stale after await gap -- **requestIdleCallback/setTimeout lifecycle**: must store handle, cancel on unmount -- **useEffect ordering**: useLayoutEffect runs before useEffect -- **Stale CSRF/headers in retry loops** -- **Missing await on async function** -- **Promise that never resolves**: cancel() that doesn't resolve completion promise -- **Retry matching user cancellation**: retryable-error check matching user-initiated abort signals - -See `project-specific.md` Agent 6 section for codebase-specific checks and known pitfalls. - -Detection heuristics: -``` -grep for: isProcessing|isLoading = true → verify finally block sets false -grep for: getState\(\) → check for await between get and use -grep for: requestIdleCallback|setTimeout → verify handle stored and cancelled -grep for: async.*\{[^}]*\} without await in caller -``` - ---- - -**Agent 7 — Error Handling & Resource Leaks** - -What it checks: -- **Floating promises**: async handler without .catch() -- **Unhandled promise rejections** -- **Processing/loading flags not reset on error** -- **Event listeners without cleanup** -- **Unbounded Maps/Sets**: must have cleanup mechanism -- **Module-level state persisting across context switches** -- **Conversation/history accumulation without limit** -- **Timer handles not returned for cancellation** -- **removeAllListeners() too broad** -- **Circular object in JSON.stringify** -- **Error handler on streams** -- **Silent failures**: catch blocks that swallow errors -- **Double status emission on failure** - -See `project-specific.md` Agent 7 section for codebase-specific checks and known pitfalls. - -Detection heuristics: -``` -grep for: \.then\( without \.catch\( -grep for: addEventListener|\.on\( in React → verify cleanup return -grep for: new Map|new Set at module level → check for cleanup -grep for: catch.*\{[^}]*\} → check if error is surfaced to user -grep for: removeAllListeners\(\) → verify scope -grep for: JSON\.stringify → check for circular reference risk -``` - ---- - -**Agent 8 — Security & Input Validation** - -What it checks: -- **XSS via dangerouslySetInnerHTML** without DOMPurify.sanitize() -- **Path traversal**: `path.join(base, userInput)` without `path.basename()` -- **postMessage validation**: must validate event.origin and event.source -- **HTML injection from headers**: display names in HTML templates without escaping -- **SQL injection**: user input in SQL without parameterized queries -- **LIKE with user input** matching unintended rows -- **Credential exposure** in source code, console.log, error messages, URL parameters -- **CSRF ordering**: nonce/state must be verified BEFORE exchanging auth code -- **Electron security**: no nodeIntegration: true, no disabled contextIsolation -- **base64url → base64 padding**: must add `=` padding -- **Sensitive data in logs** -- **HTML entity escaping**: user text interpolated into HTML without escaping - -See `project-specific.md` Agent 8 section for codebase-specific XSS surfaces and known pitfalls. - -Detection heuristics: -``` -grep for: dangerouslySetInnerHTML → verify DOMPurify.sanitize wraps content -grep for: path\.join.*filename|path\.join.*name → verify path.basename -grep for: addEventListener.*message → verify origin check -grep for: LIKE.*\$\{|LIKE.*\+.*\+ → verify parameterized -grep for: postMessage.*\* → verify specific origin -grep for: API_KEY|SECRET|TOKEN|CREDENTIAL → verify not in source -grep for: \.replace\(/-/g → verify = padding added -``` - ---- - -**Agent 9 — Data Loss & Field Preservation** - -What it checks: -- **Missing spread in object construction**: verify ALL existing fields are spread when updating individual fields -- **COALESCE defeating intended updates**: passing values that make `IS NOT NULL` true for fields that should remain unchanged -- **Partial failure without rollback**: multi-step operations where later steps fail, leaving inconsistent state -- **Object property overwritten by subsequent reassignment** -- **useRef for form fields capturing stale values** -- **Regex dropping valid characters**: patterns that silently drop `+` or other valid RFC characters from addresses -- **Boolean check on collection, action on specific item**: `array.some(predicate)` returns true but subsequent action targets a different item than the one that matched - -See `project-specific.md` Agent 9 section for codebase-specific field lists. - -Detection heuristics: -``` -grep for: { body:.*subject: without ...existing spread -grep for: COALESCE → verify NULL handling -``` - ---- - -**Agent 10 — Cross-Account & Multi-Context Safety** - -What it checks: -- **Cache keys missing context scope**: keys using only item ID without context/account prefix -- **Event listeners not filtering by context**: handlers update state without checking if event is for current context -- **Selection not cleared on context switch**: batch selection carries across contexts -- **Module-level Sets shared across contexts**: Set tracking "already processed" prevents processing same ID in different context -- **DB queries missing context scoping**: queries without WHERE clause for context -- **`accounts[0]` instead of active**: using first item instead of currently selected for context-dependent operations -- **Hardcoded default context ID** - -See `project-specific.md` Agent 10 section for codebase-specific checks and known pitfalls. - -Detection heuristics: -``` -grep for: new Map|new Set → check if keys include context scope -grep for: threadId|emailId as standalone key → verify context prefix -grep for: accounts\[0\] → verify this is intentional -grep for: WHERE.*threadId|WHERE.*email_id without context scope -grep for: addEventListener|\.on\( → verify context filter -``` - ---- - -**Agent 11 — Email/RFC Compliance & String Handling** - -What it checks: -- **RFC 5322 display name quoting**: names with special chars MUST be quoted -- **base64url → base64 padding** -- **MIME header construction** via string concat skips proper escaping -- **"LastName, FirstName" address parsing**: naive `.split(",")` corrupts addresses -- **Email address case sensitivity inconsistency** -- **Angle brackets in non-email context** triggering false HTML detection -- **RFC 2822 quote handling**: escaped quotes inside quoted strings -- **Gmail wildcard queries**: quotes break glob patterns -- **Duplicate React keys from addresses** -- **HTML tag stripping via regex** destroying legitimate content -- **Markdown false positives** -- **Pre-formatted addresses breaking dedup** -- **Trailing quote in extracted names** - -See `project-specific.md` Agent 11 section for codebase-specific checks. - -Detection heuristics: -``` -grep for: \$\{.*name.*\}.*<\$\{.*email → verify RFC 5322 quoting -grep for: replace\(/-/g.*replace\(/_/g → verify = padding -grep for: \.split\(.*,.*\) on email headers → verify handles quoted commas -grep for: toLowerCase\(\) in email comparisons → verify consistency -grep for: <[^>]+> for tag stripping → verify not matching legitimate content -``` - ---- - -**Agent 12 — Build, Packaging & Electron** - -What it checks: -- **__dirname in packaged app**: relative paths may not resolve correctly -- **Module-level app.getPath()**: called at import time, before app is ready → crashes in tests -- **Config path migration** without migrating existing files -- **removeAllListeners() too broad** -- **Packaged macOS PATH** -- **Notarization completeness**: both ZIP and DMG must be notarized and stapled -- **Icon path resolution** - -See `project-specific.md` Agent 12 section for codebase-specific checks and known pitfalls. - -Detection heuristics: -``` -grep for: __dirname in main process → verify packaged path handling -grep for: app\.getPath at module level → verify not at import time -grep for: removeAllListeners\(\) → verify not too broad -grep for: asar.*unpack → verify minimal glob -``` - ---- - -**Agent 13 — Concurrency, Deduplication & Performance** - -What it checks: -- **Cross-collection deduplication**: checking one queue but not another for duplicates -- **Sequential API calls in loops when independent** -- **Timer churn from effects**: effect recreates timers on every state change -- **Re-entrancy without guard** -- **Off-by-one in retry logic** -- **Infinite recursion in queue processing**: queue re-calls itself when no items processable -- **Cache key missing fields**: key doesn't include all inputs that affect the cached value -- **Deleting from Map during iteration**: may skip entries per ECMAScript spec -- **isRunning flag reset in clear()**: flag reset without stopping the running operation - -See `project-specific.md` Agent 13 section for codebase-specific checks and known pitfalls. - -Detection heuristics: -``` -grep for: queue|backlog|pending → verify dedup across all collections -grep for: for.*of.*await → check if calls can be parallelized -grep for: setInterval|setTimeout in effects → verify not churning -grep for: processQueue|processNext → verify no recursion risk -``` - ---- - -**Agent 14 — Agent/AI Integration** (only if agent/AI files changed) - -What it checks: -- **Config propagation to workers**: settings changes must reach utility process workers -- **Duplicate terminal events**: done/complete event emitted from multiple sources -- **AbortController without signal wiring**: controller created but signal never passed to API call -- **Permission gate not wired**: gate class exists but tool executor never calls it -- **Empty accounts/context creating invalid state**: agent gets empty context → contaminated data -- **Fragile JSON extraction**: regex-based JSON parsing fails on braces inside string values - -See `project-specific.md` Agent 14 section for codebase-specific checks and known pitfalls. - -Detection heuristics: -``` -grep for: yield.*done|emit.*done → verify single terminal event -grep for: abortController|AbortController → verify signal passed to SDK -``` - ---- - -### Step 4: Cross-Agent Deduplication - -After all agents complete, deduplicate findings where multiple agents flagged the same issue from different angles. Keep the highest-confidence version and note which agents agreed (agreement between agents increases effective confidence by 10 points). - -### Step 5: Systemic Pattern Detection - -Look for patterns across individual findings: -- If Agent 6 (async) finds a race condition AND Agent 9 (data loss) finds missing rollback on the same handler → escalate to Critical -- If Agent 9 (data loss) finds missing fields AND Agent 2 (IPC) finds the same field missing across the IPC boundary → escalate to Critical -- If multiple agents independently flag the same file → flag it as a "high-risk file" in the report - -### Step 6: Score and Filter - -Each agent assigns a confidence score (0-100) to each finding: - -| Score | Meaning | -|-------|---------| -| 0-25 | Likely false positive, or pre-existing issue not introduced by this diff | -| 25-50 | Might be real but could be a nitpick or unlikely in practice | -| 50-75 | Real issue but low severity or narrow impact | -| 75-89 | Verified real issue that will likely cause problems in production | -| 90-100 | Confirmed critical — will definitely cause a bug, crash, or security vulnerability | - -**Only report issues scored 75 or above.** - -Scoring adjustments: -- **Cross-agent agreement bonus**: +10 when 2+ agents flag the same issue -- **Known pitfall match bonus**: +15 when finding matches a pattern from `project-specific.md` (these are confirmed historical bugs) -- **Regression penalty**: If the diff reintroduces a pattern that was previously fixed in a past PR, auto-score 95+ - -### False Positive Filters - -Do NOT flag these — they are the most common false positives from CI review bots: - -- Pre-existing issues on unchanged lines (only review what the diff touches) -- Formatting/style issues (linters catch these) -- Missing test coverage (unless a complex code path has zero tests) -- Type errors that `tsc` would catch (report `tsc` errors in a separate section, don't duplicate) -- Intentional `any` or `as` with an explanatory comment or lint-ignore -- Changes in behavior that are clearly intentional from the commit context -- Suggestions to use a different library/pattern when the existing one works correctly -- CLAUDE.md rules explicitly overridden by a code comment -- General code quality (documentation, naming style) unless explicitly required in CLAUDE.md -- API call count concerns for methods called < 10x per user session -- Suggestions to extract utilities for code that appears only twice - -See `project-specific.md` False Positive Filters for codebase-specific filters. - -### Step 7: Report - -Group findings in this format: - -``` -## Code Review Results - -### Critical (score 90-100) -[findings with exact file:line, what's wrong, production impact, suggested fix] - -### Important (score 75-89) -[findings] - -### Systemic Patterns -[patterns detected across multiple findings] - -### High-Risk Files -[files flagged by 3+ agents] - -### Type Errors (from tsc) -[if any] - -Found X critical, Y important issues across Z files. Reviewed by 14 agents. -``` - -For each finding: -1. **File and line** — exact location in the diff -2. **What's wrong** — concise description -3. **Why it matters** — how this manifests as a production bug -4. **Suggested fix** — concrete code change, not vague guidance - -If no issues scored 75+: "No high-confidence issues found. Reviewed [N] files across [categories]." - -### Step 8: Fix Issues - -After presenting the report, act on the findings: - -**Simple fixes** (single-line changes, missing null checks, adding a variable to a dependency array, clearing state in an existing useLayoutEffect, adding `path.basename()`) — just fix them directly. No need to ask or plan. - -**Non-trivial fixes** (architectural changes, new error handling paths, refactoring async flow, adding rollback logic to optimistic updates) — enter plan mode first. Present the plan with the specific issues being addressed, the files that will change, and the approach for each fix. Wait for approval before making changes. - -**Type errors from `tsc`** — fix directly if the fix is obvious from the error message. If the error reveals a deeper type design problem, plan first. - -After all fixes are applied, re-run `npx tsc --noEmit` to verify no new type errors were introduced by the fixes. If the fixes touched IPC boundaries, re-run the relevant IPC contract checks from Agent 2 to confirm consistency. diff --git a/.claude/skills/review-code/project-specific.md b/.claude/skills/review-code/project-specific.md deleted file mode 100644 index e2ebad4b..00000000 --- a/.claude/skills/review-code/project-specific.md +++ /dev/null @@ -1,155 +0,0 @@ -# Project-Specific Review Context - -Architecture-specific context for the Exo mail client. Each review agent should consult its section for project context that goes beyond the general patterns in SKILL.md. The general patterns already cover the principles — this file tells you WHERE those principles apply in this codebase and which areas have historically been fragile. - ---- - -## Agent 1 — Type Safety - -Key type boundaries to verify: -- `IpcResponse` generic params must match handler return types (handlers may return `T | null`) -- `window.api` types are declared inline in multiple components — check consistency against `src/preload/index.ts` -- `ComposeAttachment` shape differs between preload and shared types with no compile-time check -- Zod schemas in `src/shared/types.ts` must stay in sync with TypeScript interfaces - ---- - -## Agent 2 — IPC Contract - -This is the #1 bug source. ~100+ IPC channels but `IpcChannels` type covers only ~12. - -- **Demo mode**: Every IPC handler needs `if (useFakeData)` early return or e2e tests crash -- **Event coverage**: Main emits `sync:new-emails`, `sync:emails-removed`, `sync:emails-updated`, `sync:status-change`, `sync:action-failed`, `sync:action-succeeded` — each needs a matching `App.tsx` listener -- **Response shapes**: Some handlers return `{ data }`, others `{ queued: true }` — consumers must handle both -- **camelCase/snake_case**: `DashboardEmail` (camelCase) ↔ `AnalysisResult` (snake_case), converted in IPC handlers. New fields need conversion in both directions. -- **Settings propagation**: Changing API key/model must reset cached Anthropic clients and propagate to utility process workers - ---- - -## Agent 3 — Database & Data Integrity - -- `label_ids` stored as JSON string ↔ `DashboardEmail.labelIds: string[]` — serialization must be explicit -- Store state in `src/renderer/store/index.ts` must match IPC handler return shapes -- `saveDraftAndSync` passing empty options defeats COALESCE (`? IS NOT NULL` becomes true → clears CC/BCC) -- `clearInboxPendingDrafts` has order-dependent JOINs — call order matters - ---- - -## Agent 4 — React Patterns & State Management - -- **Optimistic update + pending queue**: Established pattern for email actions — new actions should follow it -- **IPC event listeners**: New events need listeners in `App.tsx` useEffect block -- **EmailDetail state reset**: New local state MUST be added to the useLayoutEffect that resets on thread switch (repeatedly flagged by both Greptile and Devin) -- **expandedMessages**: When email IDs change (optimistic → real), Maps/Sets tracking expanded state must update -- **Editor-specific**: Tab+autocomplete stale blur, Shift+Tab shiftKey, Escape key plugin exit, stale position in paste/drop async handlers - ---- - -## Agent 5 — Test Quality - -- `launchElectronApp()` must receive `{ workerIndex: testInfo.workerIndex }` for DB isolation -- electron-store config changes in tests need cleanup between workers - ---- - -## Agent 6 — Async Logic & Race Conditions - -Key functions/patterns to watch: -- `removeEmails|clearSelected|setViewMode` BEFORE await — the #1 async bug in this codebase -- Compare button handlers against `useKeyboardShortcuts.ts` implementations (shortcuts are typically correct, button handlers diverge) -- `useAppStore.getState()` captured before async gap — re-read after await -- `runAgent(...)` and `waitForCompletion()` — missing await / unresolved promises -- `isRetryableError` matching user-initiated 'aborted' — cancelled ops get retried - ---- - -## Agent 7 — Error Handling & Resource Leaks - -- `window.api.on` without matching `window.api.off` in cleanup — project-specific IPC listener pattern -- `{ success: true, data: { success: false } }` nesting — outer success misleads consumers - ---- - -## Agent 8 — Security - -XSS surfaces specific to this app: email body, quoted content, signature preview, agent tool output — all need DOMPurify. -- MCP server config env vars could override `ANTHROPIC_API_KEY` -- Access tokens stored without encryption alongside encrypted refresh tokens - ---- - -## Agent 9 — Data Loss & Field Preservation - -This is the #2 bug category. The core issue: draft operations that don't spread existing fields. - -- **Historically lost fields**: `cc`, `bcc`, `calendaringResult`, `attachments`, `agentTaskId`, `subject` — any code touching `saveDraft`, `createDraft`, `sendMessage`, `queueToOutbox` must preserve all of these -- **`agent_task_id`**: Should be preserved on edit, only cleared on full regeneration -- **Outbox table schema**: Must have columns for all composition fields -- **`extractReplyAllCc` regex**: Drops addresses with `+` character - ---- - -## Agent 10 — Cross-Account Safety - -- Cache keys must use `${accountId}:${threadId}` format -- IPC event handlers must filter by `accountId` -- `selectedThreadIds` must clear on account switch -- Use `currentAccountId` from store, not `accounts[0]` -- Demo mode: DB uses `me@example.com` but handler returns `demo@example.com` -- Type confusion: `Account` vs `AccountRecord` vs `AccountInfo` -- Unsnoozed return times map grows unbounded - ---- - -## Agent 11 — Email/RFC Compliance - -Project-specific functions to watch: -- `hasRichFormatting` — missing `

` tag check -- `isHtml` — false positives from angle brackets in plain text -- "On ... wrote:" attribution parser — multi-line handling - ---- - -## Agent 12 — Build & Packaging - -- `asar.unpack` glob `**/node_modules/@anthropic-ai/**` is too broad — only runtime files needed -- DER signature parsing uses hardcoded byte offsets (fragile) -- API key written to temp file creates security window -- Migration `copyFile` catch swallows non-ENOENT errors - ---- - -## Agent 13 — Concurrency & Performance - -Project-specific hotspots: -- `processAllPending` and `processAnalysis` both queue the same items → dedup needed -- In-flight agents not cancelled before regenerate → both run simultaneously -- Manual operations bypass `MAX_CONCURRENT` pool -- `getConfig()` called on every invocation instead of caching -- `emailId:theme` cache key ignores body content changes -- Sent thread computation runs on every render (not memoized) - ---- - -## Agent 14 — Agent/AI Integration - -- `persistTaskEvents` hardcodes status instead of using parameter -- `cancelAll()` scope too broad -- `save_memory` tool uses `accounts[0]` instead of active account -- Worker provider list empty on first task (init race) -- Stale Anthropic clients after settings change -- Running agents not cancelled on prompt/settings change -- Orphaned agent traces in DB after cancellation - ---- - -## False Positive Filters - -- Dark mode styling issues (many false positives historically) - ---- - -## File Reading Hints - -- **Services**: read the prefetch service if it queues work -- **Agents/tools**: read the coordinator, worker, and renderer-side agent state diff --git a/.claude/skills/review-code/review-patterns.md b/.claude/skills/review-code/review-patterns.md deleted file mode 100644 index 064c4dfa..00000000 --- a/.claude/skills/review-code/review-patterns.md +++ /dev/null @@ -1,1855 +0,0 @@ -# Generalized Review Patterns - -Patterns organized by review agent category. Each pattern includes detection heuristics, production impact, and fix templates. Patterns are derived from confirmed bugs across 150+ PRs in this codebase, generalized for reuse. - ---- - -## Section 1: Type Safety & References (Agent 1) - -### Pattern: Implicit `any` from Untyped Import -- **Category**: TypeScript / Type Safety -- **What to look for**: `import X from 'untyped-module'` where the module has no `@types/` package or local `.d.ts` -- **Why it's a bug**: TypeScript treats all values as `any`, hiding type errors throughout the consumer. Violates CLAUDE.md "NEVER use `any`" rule. -- **The correct pattern**: -```ts -// BAD: no types available -import parser from 'some-untyped-lib'; -parser.parse(data); // parser is any, data errors hidden - -// GOOD: add declaration -// src/types/some-untyped-lib.d.ts -declare module 'some-untyped-lib' { - export function parse(data: string): ParsedResult; -} -``` -- **Confidence heuristic**: HIGH if the import is used in new code with property access or passed to typed functions. MEDIUM if only used in one place with obvious semantics. - ---- - -### Pattern: Type Assertion Bypassing Null Check -- **Category**: TypeScript / Null Safety -- **What to look for**: `value as SomeType` without prior `null`/`undefined` check, especially when value comes from async/optional source -- **Why it's a bug**: Runtime null access crash when value is actually null/undefined. The assertion silences TypeScript but doesn't prevent the runtime error. -- **The correct pattern**: -```ts -// BAD -const email = getEmail(id) as Email; -console.log(email.subject); // crash if null - -// GOOD -const email = getEmail(id); -if (!email) { - throw new Error(`Email ${id} not found`); -} -email.subject; // TypeScript knows non-null here -``` -- **Confidence heuristic**: HIGH (90) if value comes from DB query, Map.get(), or async IPC call. MEDIUM if value is from a guaranteed source like a filtered array. - ---- - -### Pattern: IpcResponse Generic Mismatch -- **Category**: TypeScript / IPC -- **What to look for**: Handler returns `T | null` but caller expects `IpcResponse` with guaranteed `.data` property access -- **Why it's a bug**: Crash on `.data` access when result is null. Confirmed from PRs #43, #44. -- **The correct pattern**: -```ts -// BAD: caller assumes data exists -const result = await window.api.getAnalysis(id); -setAnalysis(result.data); // crash when data is null - -// GOOD: check before access -const result = await window.api.getAnalysis(id); -if (result.success && result.data) { - setAnalysis(result.data); -} -``` -- **Confidence heuristic**: HIGH (95) when handler has nullable return path. Check the handler implementation to verify. - ---- - -### Pattern: Divergent Window.api Type Declarations -- **Category**: TypeScript / Interface Drift -- **What to look for**: `declare global { interface Window { api: { ... } } }` in component files that overlap with but differ from `src/preload/index.ts` -- **Why it's a bug**: Component's type declaration drifts from actual preload implementation. TypeScript says method exists but preload doesn't expose it, or vice versa. -- **The correct pattern**: -```ts -// BAD: component declares its own window.api shape -declare global { - interface Window { - api: { - getEmails: () => Promise; // different return type than preload - }; - } -} - -// GOOD: import types from shared declaration -import type { PreloadApi } from '../preload/types'; -// or use typeof preloadApi -``` -- **Confidence heuristic**: MEDIUM when shapes overlap. HIGH when the shapes actually diverge on a method the component calls. - ---- - -### Pattern: Zod Schema / TypeScript Interface Drift -- **Category**: TypeScript / Validation -- **What to look for**: Changes to a Zod schema in `types.ts` without matching TypeScript interface update, or vice versa -- **Why it's a bug**: Runtime validation (Zod) passes but TypeScript compile fails, or TypeScript compiles but runtime validation rejects valid data. -- **The correct pattern**: -```ts -// BAD: separate Zod schema and interface -const EmailSchema = z.object({ subject: z.string(), priority: z.number() }); -interface Email { subject: string; priority: string; } // mismatch: number vs string - -// GOOD: derive type from Zod -const EmailSchema = z.object({ subject: z.string(), priority: z.number() }); -type Email = z.infer; -``` -- **Confidence heuristic**: HIGH if both exist in the same file or module. MEDIUM if they're in different files but clearly represent the same entity. - ---- - -### Pattern: Unused Import from Diff -- **Category**: TypeScript / Dead Code -- **What to look for**: New import added in diff but symbol never referenced in the changed file -- **Why it's a bug**: Dead code at minimum. May indicate a forgotten usage — the developer intended to use the import but didn't. -- **The correct pattern**: Remove unused import, or add the intended usage. -- **Confidence heuristic**: MEDIUM for utility imports. HIGH if the import is a service or handler that was clearly meant to be called. - ---- - -## Section 2: IPC Contract Consistency (Agent 2) - -### Pattern: Missing Preload Bridge Method -- **Category**: Electron / IPC -- **What to look for**: New `ipcMain.handle('channel:name')` in `src/main/ipc/*.ipc.ts` without corresponding `ipcRenderer.invoke('channel:name')` in `src/preload/index.ts` -- **Why it's a bug**: Handler is unreachable from renderer — dead code or half-implemented feature. -- **The correct pattern**: -```ts -// main/ipc/drafts.ipc.ts -ipcMain.handle('drafts:refine', async (_, args) => { ... }); - -// preload/index.ts — MUST add this -refineDraft: (args: RefineArgs) => ipcRenderer.invoke('drafts:refine', args), -``` -- **Confidence heuristic**: HIGH (90) if the handler is newly added in the diff. LOW if it's a handler used only by main-process code. - ---- - -### Pattern: Missing Demo Mode Branch -- **Category**: Electron / Testing -- **What to look for**: New IPC handler without `if (useFakeData)` early return -- **Why it's a bug**: Tests and demo mode crash or make real API calls. Confirmed across PRs #22, #124. -- **The correct pattern**: -```ts -// BAD: no demo mode handling -ipcMain.handle('analysis:run', async (_, emailId) => { - const result = await analyzer.analyze(emailId); - return { success: true, data: result }; -}); - -// GOOD: demo mode early return -ipcMain.handle('analysis:run', async (_, emailId) => { - if (useFakeData) { - return { success: true, data: mockAnalysis }; - } - const result = await analyzer.analyze(emailId); - return { success: true, data: result }; -}); -``` -- **Confidence heuristic**: HIGH (90) for any new handler that calls external APIs or DB. - ---- - -### Pattern: IPC Event Without Renderer Listener -- **Category**: Electron / IPC Events -- **What to look for**: `mainWindow.webContents.send('event-name', data)` without matching `window.api.on('event-name', callback)` listener in `App.tsx` -- **Why it's a bug**: Main process fires event but no one handles it — silent failure. Data updates never reach the UI. -- **The correct pattern**: -```ts -// main process emits -mainWindow.webContents.send('sync:new-emails', { accountId, emails }); - -// App.tsx MUST have listener with cleanup -useEffect(() => { - const cleanup = window.api.on('sync:new-emails', (data) => { - addEmails(data.emails); - }); - return cleanup; -}, []); -``` -- **Confidence heuristic**: HIGH if the event carries data that should update UI state. - ---- - -### Pattern: Settings Change Not Propagating -- **Category**: Electron / Configuration -- **What to look for**: Settings change IPC handler that stores new value but doesn't invalidate cached service instances -- **Why it's a bug**: Cached clients continue using stale config (API keys, model names). Confirmed from PR #146: changing API key didn't reset Anthropic clients. -- **The correct pattern**: -```ts -// BAD: store new key but keep old client -ipcMain.handle('settings:set-api-key', async (_, key) => { - config.set('apiKey', key); - return { success: true }; -}); - -// GOOD: reset cached instances -ipcMain.handle('settings:set-api-key', async (_, key) => { - config.set('apiKey', key); - analyzerInstance = null; // force re-creation with new key - draftGenInstance = null; - return { success: true }; -}); -``` -- **Confidence heuristic**: HIGH (90) when the setting affects a cached service. MEDIUM for UI-only settings. - ---- - -### Pattern: Queued Response Not Handled -- **Category**: Electron / IPC Response -- **What to look for**: IPC response with `{ queued: true }` flag but consumer only checks `.data` -- **Why it's a bug**: Consumer treats queued operation as completed, showing incorrect UI state. -- **The correct pattern**: -```ts -// BAD -const result = await window.api.sendDraft(draft); -if (result.success) { - showToast('Sent!'); // wrong: it's queued, not sent -} - -// GOOD -const result = await window.api.sendDraft(draft); -if (result.success && result.queued) { - showToast('Queued for sending'); -} else if (result.success) { - showToast('Sent!'); -} -``` -- **Confidence heuristic**: HIGH if the handler returns queued responses (check for offline/queue logic). - ---- - -## Section 3: Database & Data Integrity (Agent 3) - -### Pattern: INSERT OR REPLACE Resetting Flags -- **Category**: SQLite / Data Integrity -- **What to look for**: `INSERT OR REPLACE INTO` on table with boolean/flag columns (e.g., `dismissed`, `is_read`, `is_starred`) -- **Why it's a bug**: All non-specified columns reset to defaults (usually 0/NULL). User's dismissed flag, read status, etc. silently lost. Confirmed in PR #22. -- **The correct pattern**: -```sql --- BAD: resets dismissed to 0 -INSERT OR REPLACE INTO analyses (email_id, needs_reply, dismissed) -VALUES (?, ?, 0); - --- GOOD: preserve existing flag -INSERT INTO analyses (email_id, needs_reply, dismissed) -VALUES (?, ?, 0) -ON CONFLICT(email_id) DO UPDATE SET - needs_reply = excluded.needs_reply, - dismissed = CASE WHEN analyses.dismissed = 1 THEN 1 ELSE excluded.dismissed END; -``` -- **Confidence heuristic**: HIGH (95) when the table has user-mutable flags. Check schema.ts for boolean columns. - ---- - -### Pattern: LIKE Substring False Match -- **Category**: SQLite / Query Correctness -- **What to look for**: `LIKE '%"VALUE"%'` on JSON string columns -- **Why it's a bug**: Matches any value containing the substring. `LIKE '%"SENT"%'` matches `"SENTIMENT"`, `"UNSENT"`, etc. Confirmed in PRs #36, #75, #79, #105. -- **The correct pattern**: -```sql --- BAD: matches SENTIMENT, UNSENT, etc. -WHERE label_ids LIKE '%"SENT"%' - --- GOOD: use json_each for exact match -WHERE EXISTS (SELECT 1 FROM json_each(label_ids) WHERE value = 'SENT') - --- GOOD (simpler but less robust): exact JSON element match -WHERE label_ids LIKE '%"SENT"%' AND label_ids NOT LIKE '%"UNSENT"%' -``` -- **Confidence heuristic**: HIGH (90) when querying JSON columns. Check if the search value is a substring of other valid values. - ---- - -### Pattern: COALESCE Preserving Stale Values -- **Category**: SQLite / Update Logic -- **What to look for**: `COALESCE(?, column)` in UPDATE/INSERT where the column should be reset on regeneration -- **Why it's a bug**: Passing NULL preserves old value instead of clearing it. Stale `gmail_draft_id` preserved after regeneration (PR #133). `saveDraftAndSync` always passing options defeating COALESCE (PR #139). -- **The correct pattern**: -```sql --- BAD: COALESCE keeps stale gmail_draft_id on regeneration -UPDATE drafts SET - body = ?, - gmail_draft_id = COALESCE(?, gmail_draft_id) -WHERE id = ?; - --- GOOD: explicit NULL when regenerating -UPDATE drafts SET - body = ?, - gmail_draft_id = NULL -- clear on regeneration -WHERE id = ?; -``` -- **Confidence heuristic**: HIGH if the operation is a regeneration/reset. MEDIUM for optional field updates where COALESCE is intentional. - ---- - -### Pattern: Missing accountId in WHERE -- **Category**: SQLite / Multi-Account -- **What to look for**: SELECT/UPDATE/DELETE on per-account tables without `WHERE accountId = ?` -- **Why it's a bug**: Returns or modifies data from all accounts — cross-account data leaks. Confirmed in PRs #22, #34. -- **The correct pattern**: -```sql --- BAD: no account scoping -SELECT * FROM emails WHERE thread_id = ?; - --- GOOD: scoped to account -SELECT * FROM emails WHERE thread_id = ? AND account_id = ?; -``` -- **Confidence heuristic**: HIGH (95) for any query on `emails`, `analyses`, `drafts`, `sender_profiles`, or `sync_state` tables. - ---- - -### Pattern: Column Name Mismatch with Schema -- **Category**: SQLite / Schema -- **What to look for**: SQL column name in query that doesn't exist in `schema.ts` table definition -- **Why it's a bug**: Query fails at runtime with "no such column" error. -- **The correct pattern**: Cross-reference every column name in new SQL against `src/main/db/schema.ts`. Update both if renaming. -- **Confidence heuristic**: HIGH (95) — immediate runtime error. Always verify against schema. - ---- - -### Pattern: JSON Column Without Serialization -- **Category**: SQLite / Serialization -- **What to look for**: Reading or writing a JSON column (e.g., `label_ids`, `attachments`) without `JSON.parse`/`JSON.stringify` -- **Why it's a bug**: Stores `"[object Object]"` on write, or reads raw JSON string as value on read. -- **The correct pattern**: -```ts -// Write -db.run('INSERT INTO emails (label_ids) VALUES (?)', JSON.stringify(labelIds)); - -// Read -const row = db.get('SELECT label_ids FROM emails WHERE id = ?', id); -const labelIds: string[] = JSON.parse(row.label_ids || '[]'); -``` -- **Confidence heuristic**: HIGH if the column stores arrays or objects. Check schema.ts for TEXT columns that hold structured data. - ---- - -## Section 4: React Patterns & State Management (Agent 4) - -### Pattern: Stale Closure in Async Hook Callback -- **Category**: React / Async -- **What to look for**: `useCallback` with async body referencing a state variable that's not in the dependency array -- **Why it's a bug**: Callback captures stale state value at closure creation time. Sends wrong data or makes wrong decision. Confirmed from PR #40: `composeAttachments` missing from `handleSend` deps. -- **The correct pattern**: -```tsx -// BAD: attachments stale in closure -const handleSend = useCallback(async () => { - await sendEmail({ body, attachments }); // attachments is stale -}, [body]); // missing attachments - -// GOOD: all referenced state in deps -const handleSend = useCallback(async () => { - await sendEmail({ body, attachments }); -}, [body, attachments]); - -// ALTERNATIVE: useRef for values that change often -const attachmentsRef = useRef(attachments); -useEffect(() => { attachmentsRef.current = attachments; }, [attachments]); -``` -- **Confidence heuristic**: HIGH (90) when the missing dep is used in an API call or state update. MEDIUM for deps used only in logging. - ---- - -### Pattern: Rules of Hooks Violation -- **Category**: React / Hooks -- **What to look for**: Hook call (`useState`, `useRef`, `useCallback`, `useEffect`, etc.) appearing after a conditional `return` statement -- **Why it's a bug**: React crashes with "Rendered fewer hooks than expected" on the conditional path. Confirmed in PR #41. -- **The correct pattern**: -```tsx -// BAD: hooks after conditional return -function EmailDetail({ email }: Props) { - if (!email) return ; - const [draft, setDraft] = useState(''); // CRASH: fewer hooks on null path - // ... -} - -// GOOD: all hooks before any conditional returns -function EmailDetail({ email }: Props) { - const [draft, setDraft] = useState(''); - if (!email) return ; - // ... -} -``` -- **Confidence heuristic**: HIGH (95) — immediate crash on the conditional path. - ---- - -### Pattern: useRef for Values That Affect Render -- **Category**: React / State -- **What to look for**: `useRef` storing a value that appears in JSX output or determines what's rendered -- **Why it's a bug**: Changing ref value doesn't trigger re-render — user sees stale data. Confirmed in PR #58. -- **The correct pattern**: -```tsx -// BAD: ref change won't re-render -const selectedTab = useRef('inbox'); -return

{selectedTab.current === 'inbox' ? : }
; - -// GOOD: useState triggers re-render -const [selectedTab, setSelectedTab] = useState('inbox'); -return
{selectedTab === 'inbox' ? : }
; -``` -- **Confidence heuristic**: HIGH if the ref value flows into JSX or a conditional rendering path. - ---- - -### Pattern: Missing State Reset on Context Switch -- **Category**: React / State Lifecycle -- **What to look for**: New `useState` in `EmailDetail` (or similar reused component) without corresponding reset in the `useLayoutEffect` that runs on thread/email switch -- **Why it's a bug**: State from previous email bleeds into new email view. User sees stale errors, loading states, or draft data. Confirmed pattern from PRs #17, #44, #95, #96. -- **The correct pattern**: -```tsx -// When adding new state: -const [newFeatureState, setNewFeatureState] = useState(false); - -// MUST add reset to existing useLayoutEffect -useLayoutEffect(() => { - setDraftError(null); - setIsLoading(false); - setNewFeatureState(false); // <-- add this -}, [threadId]); -``` -- **Confidence heuristic**: HIGH (90) for any new state in EmailDetail. Check that the reset useLayoutEffect includes the new setter. - ---- - -### Pattern: Timer Without Cleanup -- **Category**: React / Effects -- **What to look for**: `setTimeout`/`setInterval` in `useEffect` without storing handle and returning cleanup function -- **Why it's a bug**: Timer fires after unmount — setState on unmounted component, memory leak, potential crash. Confirmed in PRs #117, #122, #139. -- **The correct pattern**: -```tsx -// BAD: no cleanup -useEffect(() => { - setTimeout(() => setStatus('done'), 3000); -}, []); - -// GOOD: cleanup on unmount -useEffect(() => { - const timer = setTimeout(() => setStatus('done'), 3000); - return () => clearTimeout(timer); -}, []); -``` -- **Confidence heuristic**: HIGH if the timer triggers state updates. MEDIUM for timers that only log. - ---- - -### Pattern: Dependency Array Boolean Collapse -- **Category**: React / Effects -- **What to look for**: `[someArray.length > 0]` or `[!!value]` in dependency array -- **Why it's a bug**: Effect doesn't re-run when value changes between truthy values (e.g., length 1 → 5 both collapse to `true`). Confirmed in PR #19. -- **The correct pattern**: -```tsx -// BAD: effect runs on false→true and true→false only -useEffect(() => { ... }, [emails.length > 0]); - -// GOOD: effect runs on actual count changes -useEffect(() => { ... }, [emails.length]); -``` -- **Confidence heuristic**: MEDIUM — only a bug if the effect should re-run on value changes, not just presence changes. - ---- - -### Pattern: Object Literal in JSX Props -- **Category**: React / Performance -- **What to look for**: `` or `options={[...]}` inline in JSX -- **Why it's a bug**: Creates new reference on every render — unnecessary child re-renders if child uses React.memo or shouldComponentUpdate. -- **The correct pattern**: -```tsx -// BAD: new object every render - - -// GOOD: stable reference -const rowStyle = useMemo(() => ({ padding: 8, margin: 4 }), []); - - -// GOOD: module-level constant if no dynamic values -const ROW_STYLE = { padding: 8, margin: 4 } as const; -``` -- **Confidence heuristic**: LOW-MEDIUM — only impactful if the child does expensive rendering or the parent renders frequently. - ---- - -### Pattern: Ref Mutation During Render -- **Category**: React / Concurrent Mode -- **What to look for**: `someRef.current = value` in the render function body (not inside `useEffect` or `useLayoutEffect`) -- **Why it's a bug**: Breaks React Concurrent Mode. Ref may be mutated during aborted renders, leaving inconsistent state. Confirmed in PR #147. -- **The correct pattern**: -```tsx -// BAD: mutation during render -function Component({ value }) { - const ref = useRef(value); - ref.current = value; // mutation during render body - - // GOOD: mutation in effect - useEffect(() => { - ref.current = value; - }, [value]); -} -``` -- **Confidence heuristic**: HIGH in React 18+ with concurrent features enabled. - ---- - -### Pattern: iframe.onload After src Assignment -- **Category**: React / DOM Timing -- **What to look for**: `iframe.src = url; iframe.onload = handler;` — src assigned before onload handler -- **Why it's a bug**: iframe may load synchronously (cached content) before handler is set — onload never fires. Confirmed in PR #39. -- **The correct pattern**: -```ts -// BAD: handler set after src -iframe.src = emailContentUrl; -iframe.onload = () => adjustHeight(); - -// GOOD: handler set before src -iframe.onload = () => adjustHeight(); -iframe.src = emailContentUrl; -``` -- **Confidence heuristic**: HIGH (90) — especially for cached/local content. - ---- - -### Pattern: Escape Key Handler Without Proper Cleanup -- **Category**: React / Events -- **What to look for**: Event handler for Escape key that calls `preventDefault()` unconditionally -- **Why it's a bug**: Prevents other Escape handlers (dropdowns, modals) from working. Confirmed in PR #119. -- **The correct pattern**: -```ts -// BAD: unconditional prevent -const handleKeyDown = (e: KeyboardEvent) => { - if (e.key === 'Escape') { - e.preventDefault(); // blocks ALL other Escape handlers - closePanel(); - } -}; - -// GOOD: only preventDefault when actually consuming -const handleKeyDown = (e: KeyboardEvent) => { - if (e.key === 'Escape' && isPanelOpen) { - e.preventDefault(); - closePanel(); - } -}; -``` -- **Confidence heuristic**: MEDIUM — depends on how many Escape handlers coexist. - ---- - -## Section 5: Test Quality & Infrastructure (Agent 5) - -### Pattern: Missing workerIndex for DB Isolation -- **Category**: Testing / Isolation -- **What to look for**: `launchElectronApp()` call without `{ workerIndex: testInfo.workerIndex }` -- **Why it's a bug**: Tests share database file — cross-test contamination, flaky failures. Confirmed pattern in PR #46. -- **The correct pattern**: -```ts -// BAD: shared DB -test.beforeAll(async () => { - app = await launchElectronApp(); -}); - -// GOOD: per-worker DB -test.beforeAll(async ({ }, testInfo) => { - app = await launchElectronApp({ workerIndex: testInfo.workerIndex }); -}); -``` -- **Confidence heuristic**: HIGH (90) for any test that reads/writes DB state. - ---- - -### Pattern: Platform-Specific Keyboard Shortcut in Test -- **Category**: Testing / Cross-Platform -- **What to look for**: `Meta+k`, `Cmd+` in test assertions on a CI system that runs Linux -- **Why it's a bug**: Meta/Cmd doesn't work on Linux — test always fails on CI. Confirmed in PR #32. -- **The correct pattern**: -```ts -// BAD: Mac-only shortcut -await page.keyboard.press('Meta+k'); - -// GOOD: platform-aware -const modifier = process.platform === 'darwin' ? 'Meta' : 'Control'; -await page.keyboard.press(`${modifier}+k`); -``` -- **Confidence heuristic**: HIGH if CI runs on Linux. - ---- - -### Pattern: Assertion on Virtualized Row Count -- **Category**: Testing / Virtual Lists -- **What to look for**: `expect(rows).toHaveLength(totalItems)` with tanstack-virtual or any virtualizer -- **Why it's a bug**: Virtualizer only renders visible rows — count depends on viewport height, not total items. Confirmed in PR #147. -- **The correct pattern**: -```ts -// BAD: assumes all rows rendered -const rows = page.locator('.email-row'); -await expect(rows).toHaveCount(50); // fails: only 15 visible - -// GOOD: assert visible subset -await expect(rows.first()).toBeVisible(); -await expect(page.getByText('Showing 50 emails')).toBeVisible(); -``` -- **Confidence heuristic**: HIGH if using tanstack-virtual or any windowed list. - ---- - -### Pattern: Click Without Visibility Check -- **Category**: Testing / Robustness -- **What to look for**: `await page.click('selector')` without prior visibility assertion -- **Why it's a bug**: Clicks on non-existent or hidden element — confusing test failure message. Confirmed in PR #38. -- **The correct pattern**: -```ts -// BAD: confusing failure -await page.click('.draft-button'); - -// GOOD: clear failure message -const draftBtn = page.locator('.draft-button'); -await expect(draftBtn).toBeVisible(); -await draftBtn.click(); -``` -- **Confidence heuristic**: MEDIUM — often works but fragile in dynamic UIs. - ---- - -### Pattern: SIGKILL on Already-Exited Process -- **Category**: Testing / Cleanup -- **What to look for**: `process.kill(pid, 'SIGKILL')` in test cleanup without checking if process still running -- **Why it's a bug**: Throws ESRCH error — test cleanup fails — subsequent tests broken. Confirmed in PR #119. -- **The correct pattern**: -```ts -// BAD: crashes if already exited -process.kill(pid, 'SIGKILL'); - -// GOOD: check first -try { - process.kill(pid, 0); // check if alive (signal 0) - process.kill(pid, 'SIGKILL'); -} catch { - // already exited, nothing to do -} -``` -- **Confidence heuristic**: MEDIUM — depends on whether the process lifecycle is deterministic. - ---- - -## Section 6: Async Logic & Race Conditions (Agent 6) - -### Pattern: UI Update Before API Completion -- **Category**: Async / Race Condition -- **What to look for**: State mutations like `removeEmails(ids)`, `clearSelection()`, `setViewMode()` BEFORE `await apiCall()` -- **Why it's a bug**: If API fails, UI is out of sync — items appear removed/changed but aren't. This is the #1 bug pattern in this codebase. Confirmed in PRs #26, #28, #34, #45. -- **The correct pattern**: -```ts -// BAD: UI updated before API completes -const handleArchive = async (ids: string[]) => { - removeEmails(ids); // UI updates immediately - await gmail.archive(ids); // if this fails, emails are gone from UI -}; - -// GOOD: await API first -const handleArchive = async (ids: string[]) => { - await gmail.archive(ids); - removeEmails(ids); // only update UI on success -}; -``` -- **Confidence heuristic**: HIGH (95) when the API call can fail (network, rate limits). Always flag this pattern. - ---- - -### Pattern: Processing Flag Without Finally Block -- **Category**: Async / State Consistency -- **What to look for**: `isLoading = true; await ...; isLoading = false;` without `finally` block -- **Why it's a bug**: Error skips the reset — loading/processing state stuck forever. Confirmed in PRs #20, #96, #130, #151. -- **The correct pattern**: -```ts -// BAD: stuck on error -setIsLoading(true); -await fetchData(); -setIsLoading(false); - -// GOOD: always resets -setIsLoading(true); -try { - await fetchData(); -} finally { - setIsLoading(false); -} -``` -- **Confidence heuristic**: HIGH (90) for any flag set before an async operation. Check for `finally`. - ---- - -### Pattern: Stale Zustand State After Await -- **Category**: Async / State -- **What to look for**: `const state = useAppStore.getState()` followed by `await` and then reading from `state` -- **Why it's a bug**: State object is a snapshot — may have changed during the await gap. Confirmed in PRs #75, #84, #124, #127, #142. -- **The correct pattern**: -```ts -// BAD: stale snapshot -const state = useAppStore.getState(); -await someAsyncOperation(); -const emails = state.emails; // stale! - -// GOOD: re-read after await -await someAsyncOperation(); -const freshState = useAppStore.getState(); -const emails = freshState.emails; -``` -- **Confidence heuristic**: HIGH if state is mutated concurrently (e.g., background sync running). - ---- - -### Pattern: Missing Re-entrancy Guard -- **Category**: Async / Concurrency -- **What to look for**: Async handler callable from UI (button click, IPC) without `if (isProcessing) return` guard -- **Why it's a bug**: Rapid clicks trigger duplicate concurrent operations — duplicate API calls, double state updates. Confirmed in PRs #124, #151. -- **The correct pattern**: -```ts -// BAD: no guard -const handleGenerate = async () => { - const draft = await generateDraft(emailId); - setDraft(draft); -}; - -// GOOD: ref-based guard -const isProcessing = useRef(false); -const handleGenerate = async () => { - if (isProcessing.current) return; - isProcessing.current = true; - try { - const draft = await generateDraft(emailId); - setDraft(draft); - } finally { - isProcessing.current = false; - } -}; -``` -- **Confidence heuristic**: MEDIUM-HIGH depending on operation cost. HIGH for API calls, MEDIUM for local-only operations. - ---- - -### Pattern: Retry Matching User Cancellation -- **Category**: Async / Abort Handling -- **What to look for**: `isRetryableError` or retry logic that matches 'abort' or 'cancel' strings -- **Why it's a bug**: User-initiated cancel gets retried instead of stopping. Confirmed in PR #99. -- **The correct pattern**: -```ts -// BAD: retries user cancellation -function isRetryable(err: Error) { - return err.message.includes('aborted') || err.message.includes('timeout'); -} - -// GOOD: exclude user cancellation -function isRetryable(err: Error) { - if (err.name === 'AbortError') return false; // user cancelled - return err.message.includes('timeout') || err.message.includes('rate_limit'); -} -``` -- **Confidence heuristic**: HIGH if abort controller is used in the same code path. - ---- - -### Pattern: Missing Await on Async Call -- **Category**: Async / Correctness -- **What to look for**: Async function call without `await` keyword where the result or side effect is needed -- **Why it's a bug**: Downstream code runs before async setup completes — race condition. Confirmed in PR #124. -- **The correct pattern**: -```ts -// BAD: initialization races -initializeService(); // forgot await -const result = service.process(data); // service not ready - -// GOOD -await initializeService(); -const result = service.process(data); -``` -- **Confidence heuristic**: HIGH if the function's result or side effect is needed by subsequent code. - ---- - -### Pattern: Promise That Never Resolves -- **Category**: Async / Deadlock -- **What to look for**: Cancel/abort path that doesn't resolve or reject the associated promise -- **Why it's a bug**: `await promise` hangs forever after cancellation. Confirmed in PR #124. -- **The correct pattern**: -```ts -// BAD: cancel doesn't resolve promise -let resolve: () => void; -const promise = new Promise(r => { resolve = r; }); -function cancel() { - abortController.abort(); // forgets to resolve promise -} - -// GOOD: cancel resolves the promise -function cancel() { - abortController.abort(); - resolve(); // unblock any awaiter -} -``` -- **Confidence heuristic**: HIGH (90) when there's a cancel/abort path and an associated promise. - ---- - -### Pattern: requestIdleCallback Without Cleanup -- **Category**: Async / Lifecycle -- **What to look for**: `requestIdleCallback(fn)` without storing handle for `cancelIdleCallback` -- **Why it's a bug**: Callback fires after component unmounts — crash or stale state update. Confirmed in PRs #98, #115. -- **The correct pattern**: -```ts -// BAD: no cleanup -useEffect(() => { - requestIdleCallback(() => computeExpensiveThing()); -}, []); - -// GOOD: cleanup on unmount -useEffect(() => { - const handle = requestIdleCallback(() => computeExpensiveThing()); - return () => cancelIdleCallback(handle); -}, []); -``` -- **Confidence heuristic**: HIGH in useEffect. MEDIUM in module-level code. - ---- - -## Section 7: Error Handling & Resource Leaks (Agent 7) - -### Pattern: Floating Promise (No .catch()) -- **Category**: Error Handling / Async -- **What to look for**: `asyncFn().then(...)` without `.catch()`, or async function call without try/catch -- **Why it's a bug**: Unhandled rejection — process crash in Node.js/Electron. Confirmed in PRs #124, #133. -- **The correct pattern**: -```ts -// BAD: floating promise -fetchEmails().then(emails => updateStore(emails)); - -// GOOD: handle errors -fetchEmails() - .then(emails => updateStore(emails)) - .catch(err => console.error('Failed to fetch emails:', err)); - -// GOOD: async/await with try/catch -try { - const emails = await fetchEmails(); - updateStore(emails); -} catch (err) { - console.error('Failed to fetch emails:', err); -} -``` -- **Confidence heuristic**: HIGH at IPC boundary (main process crash terminates app). MEDIUM for renderer-side fire-and-forget. - ---- - -### Pattern: Event Listener Without Cleanup Return -- **Category**: Resource Leak / React -- **What to look for**: `addEventListener` or `.on()` in React useEffect without returning a cleanup function -- **Why it's a bug**: Listener accumulates on re-renders — memory leak, duplicate event handling. Confirmed in PRs #23, #66, #98, #147. -- **The correct pattern**: -```tsx -// BAD: listener leaks -useEffect(() => { - window.addEventListener('resize', handleResize); -}, []); - -// GOOD: cleanup return -useEffect(() => { - window.addEventListener('resize', handleResize); - return () => window.removeEventListener('resize', handleResize); -}, []); -``` -- **Confidence heuristic**: HIGH (90) for any addEventListener/on() in useEffect. - ---- - -### Pattern: Unbounded Map/Set Growth -- **Category**: Resource Leak / Memory -- **What to look for**: `Map` or `Set` that adds entries without corresponding delete/clear logic -- **Why it's a bug**: Memory grows proportional to operations — eventual OOM in long-running app. Confirmed in PRs #28, #57, #98, #101. -- **The correct pattern**: -```ts -// BAD: grows forever -const processedEmails = new Map(); -function markProcessed(id: string) { - processedEmails.set(id, true); // never cleared -} - -// GOOD: bounded with cleanup -const processedEmails = new Map(); -function markProcessed(id: string) { - processedEmails.set(id, true); - if (processedEmails.size > 10000) { - const oldest = [...processedEmails.keys()].slice(0, 5000); - oldest.forEach(k => processedEmails.delete(k)); - } -} -``` -- **Confidence heuristic**: HIGH if entries are per-operation (per-email, per-request). MEDIUM for bounded-by-design collections. - ---- - -### Pattern: Module-Level State Not Reset on Context Change -- **Category**: Resource Leak / Multi-Account -- **What to look for**: `let cache = new Map()` at module level, never cleared on account switch -- **Why it's a bug**: State persists across account switches — stale/cross-account data. Confirmed in PR #115. -- **The correct pattern**: -```ts -// BAD: persists across accounts -let senderCache = new Map(); - -// GOOD: clear on account switch -let senderCache = new Map(); -export function resetSenderCache() { - senderCache.clear(); -} -// Call resetSenderCache() in account switch handler -``` -- **Confidence heuristic**: HIGH in multi-account app. Check if module-level state is account-scoped. - ---- - -### Pattern: removeAllListeners() Too Broad -- **Category**: Resource Leak / Events -- **What to look for**: `emitter.removeAllListeners()` without specifying event name -- **Why it's a bug**: Removes ALL listeners including other modules' — breaks unrelated features. Confirmed in PR #22. -- **The correct pattern**: -```ts -// BAD: removes everything -ipcMain.removeAllListeners(); - -// GOOD: remove specific handler -ipcMain.removeListener('sync:start', syncHandler); - -// GOOD: remove by event name at most -emitter.removeAllListeners('specific-event'); -``` -- **Confidence heuristic**: HIGH (90) on shared emitters like `ipcMain`. - ---- - -### Pattern: Silent Catch Block -- **Category**: Error Handling / UX -- **What to look for**: `catch (e) { console.error(e) }` without user-facing feedback -- **Why it's a bug**: User sees nothing happened — retries — confused. Confirmed in PRs #23, #124, #128, #147, #153, #154. -- **The correct pattern**: -```ts -// BAD: silent failure -try { - await sendDraft(draft); -} catch (e) { - console.error(e); // user sees nothing -} - -// GOOD: user feedback -try { - await sendDraft(draft); -} catch (e) { - console.error('Send failed:', e); - showToast('Failed to send email. Please try again.', 'error'); -} -``` -- **Confidence heuristic**: MEDIUM-HIGH depending on operation visibility. HIGH for user-initiated actions. - ---- - -### Pattern: Circular Object in JSON.stringify -- **Category**: Error Handling / Serialization -- **What to look for**: `JSON.stringify(obj)` where obj may contain circular references (DOM nodes, Electron objects) -- **Why it's a bug**: Throws TypeError — suppresses all subsequent output if in logging path. Confirmed in PR #102. -- **The correct pattern**: -```ts -// BAD: throws on circular refs -console.log(JSON.stringify(electronResponse)); - -// GOOD: safe serialization -import { inspect } from 'util'; -console.log(inspect(electronResponse, { depth: 2 })); -``` -- **Confidence heuristic**: MEDIUM — depends on object source. HIGH for Electron/DOM objects. - ---- - -### Pattern: Writable Stream Without Error Handler -- **Category**: Error Handling / IO -- **What to look for**: `fs.createWriteStream(path)` without `.on('error', handler)` -- **Why it's a bug**: Disk errors (full disk, permissions) — uncaught exception — process crash. Confirmed in PR #102. -- **The correct pattern**: -```ts -// BAD: no error handler -const stream = fs.createWriteStream(outputPath); -stream.write(data); - -// GOOD: error handler -const stream = fs.createWriteStream(outputPath); -stream.on('error', (err) => { - console.error(`Write failed for ${outputPath}:`, err); -}); -stream.write(data); -``` -- **Confidence heuristic**: HIGH — always add error handler to writable streams. - ---- - -## Section 8: Security & Input Validation (Agent 8) - -### Pattern: dangerouslySetInnerHTML Without Sanitization -- **Category**: Security / XSS -- **What to look for**: `dangerouslySetInnerHTML={{ __html: content }}` without `DOMPurify.sanitize()` wrapping content -- **Why it's a bug**: XSS — malicious email content executes JavaScript in Electron renderer. Full access to Node.js if contextIsolation is disabled. Confirmed in PRs #56, #82, #95, #118. -- **The correct pattern**: -```tsx -// BAD: raw HTML injection -
- -// GOOD: sanitized -import DOMPurify from 'dompurify'; -
-``` -- **Confidence heuristic**: HIGH (95) — critical security issue. Always flag unsanitized innerHTML. - ---- - -### Pattern: Path Traversal via User Input -- **Category**: Security / File System -- **What to look for**: `path.join(baseDir, filename)` where filename comes from untrusted source (email attachment, user input) -- **Why it's a bug**: `../../.ssh/authorized_keys` writes outside intended directory. Confirmed in PR #40. -- **The correct pattern**: -```ts -// BAD: path traversal -const filePath = path.join(attachmentDir, attachment.filename); - -// GOOD: strip directory components -const filePath = path.join(attachmentDir, path.basename(attachment.filename)); -``` -- **Confidence heuristic**: HIGH (95) if filename comes from email/external source. - ---- - -### Pattern: postMessage Without Origin Check -- **Category**: Security / IPC -- **What to look for**: `window.addEventListener('message', handler)` without `event.origin` validation -- **Why it's a bug**: Any iframe/window can send messages to the app — potential XSS vector. Confirmed in PR #39. -- **The correct pattern**: -```ts -// BAD: accepts from anywhere -window.addEventListener('message', (event) => { - processData(event.data); -}); - -// GOOD: validate origin -window.addEventListener('message', (event) => { - if (event.origin !== expectedOrigin) return; - processData(event.data); -}); -``` -- **Confidence heuristic**: HIGH if the app processes message data from iframes. - ---- - -### Pattern: SQL LIKE With User Input -- **Category**: Security / SQL -- **What to look for**: `LIKE '%${userInput}%'` or string concatenation in LIKE clause -- **Why it's a bug**: Matches unintended rows. If input is directly concatenated, potential SQL injection. Confirmed in PRs #75, #79. -- **The correct pattern**: -```ts -// BAD: string concat in LIKE -db.all(`SELECT * FROM emails WHERE subject LIKE '%${searchTerm}%'`); - -// GOOD: parameterized -db.all('SELECT * FROM emails WHERE subject LIKE ?', [`%${searchTerm}%`]); -``` -- **Confidence heuristic**: HIGH (90) for string concatenation. MEDIUM for parameterized LIKE with wildcard issues. - ---- - -### Pattern: base64url to base64 Without Padding -- **Category**: Security / Encoding -- **What to look for**: `replace(/-/g, '+').replace(/_/g, '/')` without adding `=` padding -- **Why it's a bug**: Corrupts binary data (attachments, images). Decoder misinterprets final bytes. Confirmed in PRs #40, #41. -- **The correct pattern**: -```ts -// BAD: missing padding -function base64urlToBase64(str: string): string { - return str.replace(/-/g, '+').replace(/_/g, '/'); -} - -// GOOD: add padding -function base64urlToBase64(str: string): string { - let result = str.replace(/-/g, '+').replace(/_/g, '/'); - while (result.length % 4) result += '='; - return result; -} -``` -- **Confidence heuristic**: HIGH (90) — confirmed corruption pattern. - ---- - -### Pattern: HTML Interpolation Without Escaping -- **Category**: Security / XSS -- **What to look for**: Template literal `
${userName}
` without HTML entity escaping -- **Why it's a bug**: User input with `