From 8942c49f056829c552da0b07f74ec7ee47243807 Mon Sep 17 00:00:00 2001 From: Vikranth Reddimasu Date: Fri, 17 Apr 2026 21:18:02 -0400 Subject: [PATCH] =?UTF-8?q?feat(wave1):=20stop=20the=20bleeding=20?= =?UTF-8?q?=E2=80=94=20data=20integrity,=20security,=20destructive=20guard?= =?UTF-8?q?s?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit First of five rebuild waves from .gstack/qa-reports/PLAN.md. The audits (ux-audit.md, tech-audit.md) found 117 issues; this wave closes the ones that can silently corrupt user data, leak files, or ship a blank-screen production build. Each change is load-bearing. ## Data integrity - app-store: messages now carry a stable client id; updateMessageAt-by-index is replaced with updateMessage-by-id so streaming writes survive notebook switches, aborts, and setMessages swaps without landing in the wrong slot or silently dropping. Store also grows newChat(), resetMessagesOnly(), and setMessages() so the previous 4 callers of clearMessages() can each say what they actually meant. - useChat: user-initiated abort no longer falls through to the non-streaming fallback (stop button used to deliver the full reply anyway), and no longer leaves a partial assistant message in history to be replayed on the next send. Empty aborted turns are removed; partial ones are marked aborted. Switching notebooks tears down the in-flight stream. - useConversations.loadConversation fetches BEFORE clearing, so a network error no longer wipes the user's current context. Messages are swapped in one setMessages() call and sources are restored from the last message that actually had them, with document_name populated the same way the streaming path does. - ChatView retry path replaces clearMessages + N addMessage calls with a single setMessages slice — no incremental render flicker. - notebook_store.complete_ingestion now accumulates source_count / chunk_count instead of overwriting with per-job values. Previously a 6th upload would set the counter to 1. Test added. ## Security - Backend CORS drops allow_origin_regex=".*" + allow_credentials=True (a localhost-CSRF footgun that let any browser tab on the machine exfiltrate notebooks). Only the explicit dev origins + Electron's "null" origin now. - Document preview path resolution is locked to the notebook's scoped uploads directory. The old cross-notebook filename-match fallback let notebook A serve files from notebook B. Response now also sets Content-Disposition: attachment. - Electron app:openExternal now validates URL scheme — http(s) and mailto only. A renderer-side XSS via markdown can't pivot into file:// or javascript: handlers. ## Reliability / cleanup - DELETE /notebooks/:id now also drops the Chroma chunk + summary collections and rm -rfs the uploads directory. Prior behavior left both behind, leaking disk indefinitely. - Vite config sets base: './' so the packaged Electron renderer actually loads assets from file:// (previously: blank white screen in prod). - ConfirmDialog component wired to notebook delete and conversation delete. Neither destructive action previously had any guard; a misclick on a sidebar overflow menu would permanently destroy the notebook and all its documents, embeddings, and conversations with no undo. - CommandPalette useState for selectedIndex moved above the early return — Rules of Hooks compliance, future-proofs against React compiler / StrictMode. Everything lives behind a green CI: backend ruff + pytest clean, desktop tsc + vite build clean. Waves 2-5 build on this. Co-Authored-By: Claude Opus 4.7 (1M context) --- .gstack/qa-reports/PLAN.md | 170 +++++++++++++++++ .gstack/qa-reports/tech-audit.md | 148 ++++++++++++++ .gstack/qa-reports/ux-audit.md | 180 ++++++++++++++++++ apps/desktop/electron/main.cjs | 11 +- apps/desktop/src/components/chat/ChatView.tsx | 13 +- .../src/components/layout/AppShell.tsx | 4 +- .../desktop/src/components/layout/Sidebar.tsx | 94 +++++++-- .../src/components/ui/CommandPalette.tsx | 15 +- .../src/components/ui/ConfirmDialog.tsx | 93 +++++++++ .../src/components/ui/confirm-dialog.css | 112 +++++++++++ apps/desktop/src/hooks/useChat.ts | 116 +++++++---- apps/desktop/src/hooks/useConversations.ts | 38 ++-- apps/desktop/src/store/app-store.ts | 62 ++++-- apps/desktop/src/types.ts | 15 +- apps/desktop/vite.config.ts | 3 + backend/notebooklm_backend/app.py | 8 +- .../notebooklm_backend/routes/documents.py | 100 ++++------ .../notebooklm_backend/routes/notebooks.py | 27 ++- .../services/notebook_store.py | 6 +- .../services/vector_store.py | 15 ++ backend/tests/test_notebook_store.py | 25 +++ 21 files changed, 1095 insertions(+), 160 deletions(-) create mode 100644 .gstack/qa-reports/PLAN.md create mode 100644 .gstack/qa-reports/tech-audit.md create mode 100644 .gstack/qa-reports/ux-audit.md create mode 100644 apps/desktop/src/components/ui/ConfirmDialog.tsx create mode 100644 apps/desktop/src/components/ui/confirm-dialog.css diff --git a/.gstack/qa-reports/PLAN.md b/.gstack/qa-reports/PLAN.md new file mode 100644 index 0000000..1a9908f --- /dev/null +++ b/.gstack/qa-reports/PLAN.md @@ -0,0 +1,170 @@ +# UX/UX/Tech Rebuild Plan — Notebook LM + +**Based on:** `ux-audit.md` (78 findings) + `tech-audit.md` (39 findings). + +**Philosophy:** Fix what silently breaks or corrupts user data FIRST. Resurrect the signature citation feature. Then missing actions. Then reliability. Then polish. No new features until the core works. + +--- + +## Wave 1 — Stop the bleeding +*Nothing the user does should silently corrupt or lose data. Nothing shipped should be cosmetic-only.* + +| # | Fix | Files | Evidence | +|---|-----|-------|----------| +| 1.1 | **Add `base: './'` to vite config** — production Electron build is a blank screen without this | `apps/desktop/vite.config.ts` | tech 11-A | +| 1.2 | **Fix `complete_ingestion` source_count overwrite** — change absolute `=` to incremental `+= source_count` | `backend/notebooklm_backend/services/notebook_store.py:159` | tech 7-C | +| 1.3 | **Delete notebook → also delete Chroma collection + upload files** | `backend/notebooklm_backend/routes/notebooks.py:36` | tech 7-D | +| 1.4 | **Confirm before destructive actions** — wire a shared `ConfirmDialog` for: delete notebook, delete conversation, (future) delete document. Danger-styled, focus trap, Escape to cancel | `components/ui/ConfirmDialog.tsx` (new), `Sidebar.tsx:261`, `Sidebar.tsx:289` | ux C4, C9 | +| 1.5 | **Kill the stale-index bug in `updateMessageAt`** — give each message a UUID at add-time, look up by ID not by index | `store/app-store.ts:91`, `hooks/useChat.ts:25` | tech 1-A | +| 1.6 | **Fix abort → next send corruption** — on abort, either truncate the partial assistant message with a clear "(stopped)" badge, or remove it from `messages` before next `send()` | `hooks/useChat.ts:20` | ux C8, H11 | +| 1.7 | **Don't fall through to non-streaming fallback on user abort** — check `controller.signal.aborted` first | `hooks/useChat.ts:106` | tech 1-C | +| 1.8 | **Decompose `clearMessages`** — `newChat()` (UI clear + forget conversation) vs `clearMessagesOnly()` (for notebook switch keep conversation intact). Stop torching `activeSources` on every reset | `store/app-store.ts`, `hooks/useConversations.ts:31` | ux H12, H24, H25 | +| 1.9 | **Cancel in-flight stream on notebook switch** — don't let a stream complete into a different notebook's conversation | `store/app-store.ts:86`, `hooks/useChat.ts` | tech 1-B | +| 1.10 | **Fix CommandPalette hooks-after-return violation** — move `useState(selectedIndex)` above the early return | `components/ui/CommandPalette.tsx:52` | ux M22 | +| 1.11 | **Lock down CORS** — drop `allow_origin_regex=".*"`, keep only explicit localhost origins, `allow_credentials=False` | `backend/notebooklm_backend/app.py:86` | tech 13-A | +| 1.12 | **Validate `openExternal` URL scheme** — only `https?://` and `mailto:` | `apps/desktop/electron/main.cjs:165` | tech 13-B | +| 1.13 | **Fix cross-notebook file disclosure in preview** — all path strategies must resolve inside the notebook's scoped uploads directory. Remove strategy 4 (cross-notebook scan) | `backend/notebooklm_backend/routes/documents.py:154` | tech 7-B | + +**Deliverable:** No user action silently corrupts state. Production build renders. Notebook/conversation deletes have an off-ramp. Destructive races are gone. + +--- + +## Wave 2 — Citation trust layer +*The product's signature feature — grounded answers with visible citations — is 100% dead. Bring it back.* + +| # | Fix | Files | Evidence | +|---|-----|-------|----------| +| 2.1 | **Render real citations** — parse assistant output for `[N]` markers, wrap each sentence containing one in a `.cited` span with an amber left-rule, render `[N]` as a `.cite-marker` mono chip. Use ReactMarkdown's `components.{p,li}` or a small remark plugin | `components/chat/MessageBubble.tsx` | ux H14 | +| 2.2 | **Wire citation click → source open** — clicking `[N]` or hovering the sentence scrolls/highlights the matching `.source-card` in the panel, and opens the document preview when clicked. Two-way link, not just chip-to-panel | `MessageBubble.tsx`, `SourcePanel.tsx`, `App.tsx` (preview opener) | ux H17 | +| 2.3 | **Fix the source panel toggle** — `SourcePanel.tsx` must check `sourcePanelOpen` too, not just source count. Persist the toggle per-notebook | `components/layout/SourcePanel.tsx:19`, `store/app-store.ts` | ux C3, M6 | +| 2.4 | **Animate source panel in/out** — don't jerk the chat column 280px. Width + opacity transition, or slide-in overlay on narrow windows | `components/layout/SourcePanel.tsx`, `layout.css` | ux H18 | +| 2.5 | **Empty source panel affordance** — when empty but toggled open, show "Sources will appear here after your first question" in muted serif | `SourcePanel.tsx` | ux H5 | +| 2.6 | **Rewrite `DocumentPreview.css` on the design system** — warm stone palette, tokenized colors, `--radius-modal`, `--font-mono` for page counter, amber highlight using `--color-cite` | `apps/desktop/src/DocumentPreview.css` | ux H21, M21 | +| 2.7 | **Stop the double PDF load on citation click** — reuse the loaded pdf.js instance, search text via the already-rendered pdf object | `DocumentPreview.tsx:48` | tech + ux H19 | +| 2.8 | **Tighten highlight match ratio** — 0.3 is too loose. Use a scoring window + longest-common-subsequence match, cap highlight span to the matched sentence(s) only | `DocumentPreview.tsx:117` | ux H20 | +| 2.9 | **Preview modal keyboard focus + ESC** — programmatically focus the overlay on open, keyboard nav works without a click first, ESC closes | `DocumentPreview.tsx:128` | ux L8 | +| 2.10 | **Replace `📄/📑` emoji toggle with icons** | `DocumentPreview.tsx:166` | ux M20 | +| 2.11 | **Persist sources on historical messages** — `loadConversation` must synthesize `document_name` for persisted sources (same path as `useChat.meta` handler) | `hooks/useConversations.ts:35`, `types.ts:114` | tech 12-A | +| 2.12 | **Set `activeSources` from the latest assistant message with sources, not overwrite-per-message** — or don't restore at all for historical loads (ambiguous context) | `hooks/useConversations.ts:33` | tech 4-B / ux H16 | + +**Deliverable:** The amber-left-rule citation pattern from DESIGN.md actually renders. Clicking `[1]` scrolls to the right source card and opens the document at the right place. Preview modal matches the warm-stone palette. + +--- + +## Wave 3 — Missing actions & error visibility +*The product is missing table-stakes affordances. Errors vanish.* + +| # | Fix | Files | Evidence | +|---|-----|-------|----------| +| 3.1 | **Rename notebook** — backend `PATCH /notebooks/{id}`, frontend `renameNotebook()` API + context-menu action with inline edit (same pattern as conversation rename) | `backend/routes/notebooks.py`, `notebook_store.py`, `apps/desktop/src/api.ts`, `Sidebar.tsx` | ux C5 | +| 3.2 | **Delete document** — backend `DELETE /documents/{id}?notebook_id=...`, frontend action on document row (hover overflow menu, like conversations). Confirmation dialog reused from Wave 1 | `backend/routes/documents.py`, `api.ts`, `DocumentCard.tsx` | ux C6 | +| 3.3 | **Duplicate upload detection** — backend returns a clear "already indexed" signal (or upsert), frontend shows a "File already added" toast | `backend/routes/documents.py`, `ingestion.py`, `api.ts`, `AppShell.tsx:136` | ux C7 | +| 3.4 | **Real upload progress** — add an `uploadState: Record` to the store, pipe `XMLHttpRequest.upload.progress` or fetch + duplex streams, show inline progress bars in the sidebar during indexing | `store/app-store.ts`, `api.ts`, `components/documents/*` | ux H8, H23 | +| 3.5 | **Create empty notebook** — split "+ New notebook" from "Upload document". Empty notebooks are legal; upload happens after | `Sidebar.tsx:78`, `useNotebooks.ts` | ux H7 | +| 3.6 | **Wire wizard model selection to backend** — either (a) persist the chosen model and send on every chat request so backend honors it, or (b) cut step 2 entirely and let backend auto-resolve silently. Not the current half-state | `components/ui/SetupWizard.tsx:229`, `backend/routes/chat.py`, `backend/config.py` | ux C1 | +| 3.7 | **Persistent error log** — add a notification center (bell icon in sidebar footer) with last N errors. Toast auto-dismiss still exists but errors also log here with timestamp + action (retry, copy, dismiss) | `components/ui/NotificationCenter.tsx` (new), `components/ui/Toast.tsx`, `store/app-store.ts` | ux H22, L12 | +| 3.8 | **User-friendly error messages** — central error-translator: `Request failed with status 422` → "That file type isn't supported", `ERR_CONNECTION_REFUSED` → "Backend isn't running" | `apps/desktop/src/api.ts`, or `utils/errorMessages.ts` (new) | ux M28 | +| 3.9 | **Zotero button in sidebar** — not just hidden in command palette. Stamp it as an import source near the upload button with its own discoverable affordance | `components/layout/Sidebar.tsx` | ux discoverability | +| 3.10 | **Conversation load should look like a load, not a stream** — bulk `setMessages(ChatMessage[])` store action, single render, skeleton during fetch | `store/app-store.ts`, `hooks/useConversations.ts:33` | ux H15, tech 14-B | +| 3.11 | **Keep current context on load failure** — only clear messages AFTER the fetch resolves with data | `hooks/useConversations.ts:31` | ux C10 | +| 3.12 | **Ollama health in ConnectionBanner** — banner monitors both backend and Ollama. If Ollama is down, show "AI model offline — start Ollama" with one-click "Copy command" | `components/ui/ConnectionBanner.tsx` | ux M27 | +| 3.13 | **Initial ConnectionBanner check** — run `check()` on mount, not just on interval tick | `ConnectionBanner.tsx:13` | ux L3 | + +**Deliverable:** Users can rename, delete, see upload progress, and understand what goes wrong. Errors don't disappear in 4 seconds. + +--- + +## Wave 4 — Reliability +*The backend and streaming path have structural weaknesses that cause silent failure, OOM, or UI lockups.* + +| # | Fix | Files | Evidence | +|---|-----|-------|----------| +| 4.1 | **Backend crash recovery** — Electron detects unexpected backend exit, sends IPC to renderer, attempts restart, renderer shows "Backend restarting…" banner with retry countdown | `apps/desktop/electron/main.cjs:97`, new IPC channel, `App.tsx` / `ConnectionBanner.tsx` | tech 5-A | +| 4.2 | **API timeouts on every call** — accept `AbortSignal` in `request()`, default 30s for lookups, 300s for uploads, 5s for healthz | `apps/desktop/src/api.ts:53` | tech 2-A | +| 4.3 | **Exponential backoff for 503 / ECONNREFUSED** — 3 attempts, 500ms / 1s / 2s, only for idempotent GETs | `apps/desktop/src/api.ts:53` | tech 2-B | +| 4.4 | **SQLite WAL mode** — `PRAGMA journal_mode = WAL` in all three stores; `busy_timeout` in NotebookStore and MetricsStore | `backend/services/conversation_store.py`, `notebook_store.py`, `metrics_store.py` | tech 6-C, 8-B | +| 4.5 | **Embedding batch cap** — batch size 64, iterate chunks | `backend/services/vector_store.py:40` | tech 9-B | +| 4.6 | **Embeddings in threadpool** — wrap `model.encode()` via `run_in_executor` | `backend/services/embeddings.py:31` | tech 9-A | +| 4.7 | **LlamaCpp generate in threadpool** — same treatment | `backend/services/llm.py:123` | tech 10-B | +| 4.8 | **Ollama streaming timeout** — `httpx.Timeout(connect=5, read=300, write=10, pool=5)` | `backend/services/llm.py:65` | tech 10-A | +| 4.9 | **Upload size limit + stream-to-disk** — 100MB cap (configurable), chunked writes, not `await file.read()` | `backend/routes/documents.py:17` | tech 7-A | +| 4.10 | **Upsert, not add, for chunks** — lets re-uploads succeed cleanly | `backend/services/vector_store.py:40` | tech 9-C | +| 4.11 | **OCR guard for scanned PDFs** — reject with clear error if extracted text < min threshold | `backend/services/document_loader.py:80` | tech 9-D | +| 4.12 | **Disable Chroma telemetry** — `Settings(anonymized_telemetry=False)`. Aligns with offline-first | `backend/services/vector_store.py:229` | tech 6-B | +| 4.13 | **Per-conversation stream lock** — asyncio lock keyed by conversation_id to prevent concurrent writers | `backend/routes/chat.py:45` | tech 3-D | +| 4.14 | **`conversation_id` also in `done` event** — frontend uses `done` as authoritative if present | `backend/routes/chat.py`, `useChat.ts:54` | tech 3-E | +| 4.15 | **Fix `list_documents` O(n)** — query from summaries collection, not all chunks | `backend/routes/documents.py:123` | tech 14-A | +| 4.16 | **Defer document summary LLM to background** — don't block upload on summary generation | `backend/services/ingestion.py:50` | tech 10-D | +| 4.17 | **Separate context window from max output tokens** — `llm_context_window` (4096+) vs `llm_max_output_tokens` (512) in config | `backend/config.py:28` | tech 10-E | +| 4.18 | **Resolve Ollama model lazily, not at factory time** — use FastAPI `lifespan` or resolve on first chat | `backend/app.py:27` | tech 6-A | + +**Deliverable:** Backend crashes self-heal. Large PDFs don't OOM. Event loop doesn't freeze mid-stream. Uploads don't hang forever. + +--- + +## Wave 5 — Polish +*Once the product works, make it actually feel like DESIGN.md says it should.* + +| # | Fix | Files | Evidence | +|---|-----|-------|----------| +| 5.1 | **Pane resize** — drag handles between sidebar/chat and chat/source panel, persisted to localStorage | `components/layout/AppShell.tsx`, `layout.css` | ux H4 | +| 5.2 | **Reduced-motion: stop, don't flicker** — `animation: none !important` for the 3 keyframe animations (shimmer, skeleton pulse, fadeIn) instead of 0.01ms duration | `design-system/tokens.css:198` | ux L reduced-motion | +| 5.3 | **Kill hardcoded colors in `DocumentCard`** — swap `#ef4444`/`#3b82f6` for token-based file-type pills using a single neutral mono chip with file extension | `components/documents/DocumentCard.tsx:4` | ux M11 | +| 5.4 | **Tokenize `SourcePanel` relevance colors** — `--color-accent` (high), `--color-cite` (mid), `--color-text-muted` (low) | `SourcePanel.tsx:4` | ux M17 | +| 5.5 | **Clamp `relevance_score` to 0–100** — defensive, or fix backend to always return 0–100 | `SourcePanel.tsx:49`, `types.ts:26`, backend | ux M18 | +| 5.6 | **Wizard card elevation** — add `background: var(--color-bg-surface)`, `border`, `border-radius: var(--radius-modal)`, `box-shadow: var(--shadow-lg)` | `components/ui/setup-wizard.css:12` | ux M4 | +| 5.7 | **Wizard primary button color parity** — `color: #0c0a09` like the rest of the app | `setup-wizard.css:244` | ux L1 | +| 5.8 | **Wizard step dot visibility** — use `--color-border-hover` for inactive so they're actually visible | `setup-wizard.css:31` | ux L2 | +| 5.9 | **Context menu radius → modal** — `var(--radius-modal)` on `.context-menu` container | `layout.css:684` | ux M9 | +| 5.10 | **Chat empty state checks doc count** — "Add a document to get started" if notebook is empty, "What would you like to know?" otherwise | `components/chat/ChatView.tsx`, `hooks/useDocuments.ts` | ux (chat empty state) | +| 5.11 | **BibTeX export not destroyed by context resets** — enable whenever the visible conversation has messages with sources, not only `activeSources.length > 0` | `ChatView.tsx:118`, `OverflowMenu` logic | ux H25 | +| 5.12 | **Show document-count hint on welcome + sidebar sections** — helps user notice when notebook is empty | `Sidebar.tsx`, `layout.css` | ux discoverability | +| 5.13 | **Auto-scroll pause when user scrolls up** — use `isScrolledUp` to gate `scrollIntoView` during streaming | `ChatView.tsx:48` | ux M13 | +| 5.14 | **Accessibility polish** — keyboard handlers on `DocumentCard` / `SourcePanel` role=button, `aria-label` on `...` menus, `role="dialog"` + focus trap on CommandPalette and wizard | throughout | ux a11y | +| 5.15 | **Font-weight axis on Bunny Fonts URL** — switch to `plus-jakarta-sans:ital,wght@0,400..700` to support 450/550 weights already in CSS | `apps/desktop/index.html` | ux L font | +| 5.16 | **Multi-word fuzzy search in Command Palette** — split query on whitespace, score each token separately, combine | `components/ui/CommandPalette.tsx:13` | ux M24 | +| 5.17 | **Keyboard shortcut hints in palette items** — inline `⌘N`, `⌘K`, etc. | `CommandPalette.tsx` | ux L9 | +| 5.18 | **Add `?` shortcut global override** — don't block when chat textarea has focus, or re-map to `⌘/` | `AppShell.tsx:40` | ux M25 | +| 5.19 | **Remove `DropZone.tsx`** — dead code | delete file | ux M10 | +| 5.20 | **Remove duplicate `final_reply` assignment** | `backend/services/chat.py:205` | tech 3-B | + +**Deliverable:** App matches DESIGN.md. No hardcoded colors. A11y passes a basic audit. The loose ends are tied off. + +--- + +## Estimates + +| Wave | Scope | Rough work | Files touched | +|------|-------|-----------|---------------| +| 1 | Data integrity + broken basics | 1-2 PRs, ~300 lines | ~12 files | +| 2 | Citation layer | 1 PR, ~400 lines (citation renderer is meaty) | ~6 files | +| 3 | Missing actions + errors | 2 PRs, ~500 lines | ~15 files | +| 4 | Reliability / backend | 2 PRs, ~400 lines | ~14 files | +| 5 | Polish | 1-2 PRs, ~250 lines | ~20 files | + +Total: ~6–8 PRs, ~1850 lines. Each wave is a green CI before starting the next. + +--- + +## Proposed sequence + +1. Present this plan to the user, get direction on scope and priority (can drop/swap any wave item) +2. Run a short `/plan-eng-review` on the plan once it's locked (skip if user wants to go fast) +3. Execute Wave 1 end-to-end → PR → merge on green CI +4. Wave 2 → PR → merge +5. Wave 3 → 2 PRs +6. Wave 4 → 2 PRs +7. Wave 5 → 1-2 PRs + +Each PR: single wave or natural sub-wave boundary. CI green before merge. I never commit without explicit go. + +--- + +## Explicit non-goals of this rebuild + +- No new user-facing features outside what the audits surfaced +- No swap to a different framework / no rewrite of routing +- No refactor of the Zustand store architecture (fix bugs, don't reshape) +- No backend ORM migration (stay on raw sqlite3 + chroma) +- No redesign of the visual language (DESIGN.md is already approved and implemented in tokens) +- Test suite: I'll add targeted tests to Wave 1 + 4 critical paths (abort flow, stream cancel, ingestion error paths), but not a full coverage push diff --git a/.gstack/qa-reports/tech-audit.md b/.gstack/qa-reports/tech-audit.md new file mode 100644 index 0000000..7eebfb4 --- /dev/null +++ b/.gstack/qa-reports/tech-audit.md @@ -0,0 +1,148 @@ +# Tech Audit — Notebook LM (Offline RAG Desktop App) + +**Date:** 2026-04-17 +**Method:** Deep code trace across frontend + backend + Electron main + +## Severity Summary + +| Severity | Count | +|----------|-------| +| Critical | 1 | +| High | 17 | +| Medium | 13 | +| Low | 8 | + +**Top 5 threats to "actually works":** +1. **Finding 11-A** (Vite `base` missing) — packaged production app is a blank screen +2. **Finding 7-C** (`complete_ingestion` clobbers source_count) — wrong document counts after every upload +3. **Finding 9-B** (unbounded embedding batch) — OOM on large document upload, silent backend crash +4. **Finding 7-D** (delete notebook leaks Chroma + files) — storage grows without bound +5. **Finding 5-A** (backend crash leaves UI broken) — any backend crash makes the app unusable with no guidance + +--- + +## 1. State Management + +- **1-A High:** `updateMessageAt` uses positional index that can go stale. Mid-stream notebook switch + component unmount leaves `isStreaming: true` permanently. Fix: message UUIDs, not array indices (`app-store.ts:91-97`, `useChat.ts:25-27`) +- **1-B High:** Notebook switch during stream creates ghost conversation under wrong notebook. Frontend optimistic entry vs backend's in-flight conversation desync (`app-store.ts:86`, `useChat.ts:54`) +- **1-C High:** `AbortError` falls through to non-streaming fallback — user clicks stop, gets full reply anyway (`useChat.ts:106-123`) +- **1-D Medium:** `useNotebooks.create` not idempotent on rapid click — two notebooks created in non-deterministic order (`useNotebooks.ts:29-37`) + +## 2. API Layer + +- **2-A High:** No timeout on any `fetch` (only streaming has AbortSignal). `fetchConfig` can hang indefinitely on cold start if Ollama is slow (`api.ts:53-68`) +- **2-B Medium:** Single-attempt, no retry. Cold-start race with backend causes hard errors (`api.ts:53-68`) +- **2-C Low:** `getApiBase` not thread-safe — multiple IPC round-trips on startup (`api.ts:31-51`) +- **2-D Medium:** `response.json()` cast with no runtime validation — shape drift surfaces as mystery crashes (`api.ts:68`) + +## 3. Streaming Chat + +- **3-A High:** Mid-stream network break falls through to fallback instead of erroring cleanly (`api.ts:180-192`, `useChat.ts:103-123`) +- **3-B Low:** Duplicate `final_reply` assignment (`backend/services/chat.py:205-206`) +- **3-C Medium:** SSE parser doesn't handle multi-line `data:` — fragile to future events (`api.ts:161-179`) +- **3-D High:** Backend has no per-conversation lock. Rapid-fire sends + concurrent `add_message` → nondeterministic ordering in DB (`backend/routes/chat.py:45-108`) +- **3-E Medium:** `conversation_id` sent only via `meta` event — if meta drops, user's thread splits into two conversations (`useChat.ts:54`, `backend/routes/chat.py:52`) + +## 4. Hooks + +- **4-A Medium:** `renameConversation` revert reads post-optimistic state, fragile if races (`useConversations.ts:61-79`) +- **4-B Medium:** `loadConversation` sources — last message wins, overwrites in a loop (`useConversations.ts:33-38`) +- **4-C Low:** `useDocuments` fires wasted refresh on notebook-switch null state (`useDocuments.ts:10-24`) +- **4-D Low:** `useNotebooks.refresh` has stable-ish callback deps — minor inefficiency + +## 5. Electron Main + +- **5-A Critical:** Backend crash after window loaded — no IPC, no retry, no user feedback. UI fails silently on every fetch (`electron/main.cjs:97-103`) +- **5-B Medium:** Port search limited to 8000–8003; if occupied, falls back to `http://127.0.0.1:8000` which may be a foreign process (`electron/main.cjs:29-34`) +- **5-C High:** No CSP on BrowserWindow. XSS via markdown → arbitrary `shell.openExternal` → OS-level escalation path (`electron/main.cjs:127-150`) +- **5-D Low:** `before-quit` handler could loop in edge case if `stopBackend` fails to clear `backendProcess` (`electron/main.cjs:212-218`) + +## 6. Backend Startup + +- **6-A High:** `_available_ollama_models` is sync `httpx.get` at factory time. Blocks uvicorn startup. Ollama slow → Electron `waitForBackend` 30s timeout (`app.py:27-28`, `model_profiles.py:32-38`) +- **6-B Low:** ChromaDB telemetry — no `Settings(anonymized_telemetry=False)`. Log noise + violates offline promise (`vector_store.py:229`) +- **6-C High:** Three SQLite stores share `metadata.db` but only `ConversationStore` sets `busy_timeout`. Concurrent ingestion + chat can fail with `SQLITE_BUSY` silently + +## 7. Backend Routes + +- **7-A High:** `documents/upload` reads entire file into memory — no size limit. 2GB PDF OOMs backend (`routes/documents.py:17-104`) +- **7-B High:** Document preview has path traversal mitigation only in strategy 2. Strategies 1/3/4 match by filename → **cross-notebook file disclosure** (`routes/documents.py:154-235`) +- **7-C High:** `complete_ingestion` **overwrites** `source_count` with per-job count. Notebook with 5 docs + 6th upload → count becomes 1 (`notebook_store.py:159-197`) +- **7-D High:** `DELETE /notebooks/{id}` removes SQLite row but leaves ChromaDB collection + upload files on disk. Storage leak (`routes/notebooks.py:36-44`) +- **7-E Low:** `DELETE /conversations/{id}` TOCTOU — returns "deleted" even if no-op +- **7-F Medium:** `speak` endpoint accepts `dict[str, str]` — no Pydantic validation, no max-length → easy DoS (`routes/speech.py:36-50`) +- **7-G Medium:** Zotero import runs sync in one request handler. Large libraries time out with partial state (`routes/zotero.py:82-185`) + +## 8. Conversation Store + +- **8-A Medium:** `auto_title_if_needed` does get + update in two transactions → race window (`conversation_store.py:204-214`) +- **8-B Medium:** No WAL mode. Every write blocks all reads. During stream completion, sidebar refresh stalls (`conversation_store.py:44`, `notebook_store.py:25`) + +## 9. Vector Store / Embeddings + +- **9-A High:** `model.encode()` is sync CPU-bound but called from async context → blocks event loop for hundreds of ms (`embeddings.py:31-35`) +- **9-B High:** `add_chunks` embeds ALL chunks in one batch. 500-page PDF → 1000 texts to model at once → GB allocation → OOM (`vector_store.py:26-49`) +- **9-C Medium:** `collection.add()` not `.upsert()` — re-uploads fail with `DuplicateIDError` + partial state (`vector_store.py:40-48`) +- **9-D Medium:** No OCR fallback for scanned PDFs. User uploads scanned PDF, gets "success" + 0 chunks, then "no documents found" on every query (`document_loader.py:80-98`) +- **9-E Low:** `query_document_summaries` silently returns `[]` on any error — masks storage corruption (`vector_store.py:174-184`) + +## 10. LLM Providers + +- **10-A High:** Ollama streaming client uses `timeout=None` — unbounded. Hung stream holds connection forever (`llm.py:65`) +- **10-B High:** `LlamaCppBackend.generate/stream_generate` declared async but call sync blocking `llama.create_completion()` → entire event loop freezes during inference (`llm.py:123-136`) +- **10-C Medium:** `LlamaIndexRAGService.prepare_prompt()` always delegates to `_fallback_rag` → 433 lines of code that don't actually run in the streaming path (`rag_llamaindex.py:27-35`) +- **10-D Medium:** Document summary generated synchronously during ingestion → 10-doc Zotero import = 10 serial LLM calls tacked onto upload latency (`ingestion.py:50-57`) +- **10-E Medium:** `llm_context_window` and `llm_max_tokens` both set to 2048. Long prompts silently truncated with no warning (`config.py:28-29`) + +## 11. Build / Config + +- **11-A Medium (really Critical for packaged app):** `vite.config.ts` has no `base: './'`. Packaged Electron renderer loads from `file://` with absolute `/assets/...` paths → blank white screen +- **11-B Medium:** electron-builder mac arm64 only. No x64, no win, no linux +- **11-D High:** No CSP meta tag in `index.html` + +## 12. Type Safety + +- **12-A Medium:** `PersistedMessage.sources` type claims `SourceChunk[]` but backend sends raw `list[dict]`. Messages loaded from history → `document_name` undefined → source panel blank names on reloaded conversations (`types.ts:114-121`, `routes/conversations.py:29-33`) +- **12-B Low:** `meta.sources` type says `StreamSource[]` but backend can send `null` — guard `?? []` is fragile to refactor (`types.ts:29-38`) + +## 13. Security + +- **13-A High:** `allow_origin_regex = ".*"` + `allow_credentials = True` = localhost CSRF. Any malicious web page can exfiltrate notebooks/messages/files (`app.py:86`) +- **13-B High:** `shell.openExternal(url)` no scheme validation. Renderer XSS → `file:///etc/passwd` or `javascript:...` (`electron/main.cjs:165-169`) +- **13-C Medium:** `FileResponse` with `application/octet-stream` no `Content-Disposition: attachment` → webview may render inline (`routes/documents.py:227-232`) + +## 14. Performance + +- **14-A High:** `list_documents` does `collection.get()` with no limit → fetches all chunks to build file list. O(n) on collection size (`routes/documents.py:123`) +- **14-B Medium:** `loadConversation` calls `addMessage` in a loop → N Zustand re-renders (`useConversations.ts:33-39`) +- **14-C Medium:** `rag_llamaindex` scans all metadata per query before vector search — adds 100-500ms per query on large notebooks (`rag_llamaindex.py:161`) + +--- + +## Cross-Cutting + +### Missing Observability +- No structured logs, no correlation IDs +- No metrics exposed outside the `chat_metrics` table +- No crash reporting (Sentry/Bugsnag) +- Backend startup errors not surfaced to UI +- Backend exit logs only — no `app.setAboutPanelOptions` crash info + +### Zero Test Coverage +- All frontend state, hooks, api +- Streaming endpoint +- SSE parser +- Documents ingest + preview +- Vector store add/query +- Electron lifecycle + +### Migration / Upgrade Risk +- No migration runner for SQLite schema +- No ChromaDB index version stamp +- No check that stored embeddings match current embedding model — silent wrong-space queries + +### Dead Code +- `rag_llamaindex.py` `prepare_prompt` only delegates +- `chat.py:205-206` duplicate assignment +- `HashEmbeddingBackend` shipped in production builds +- `DummyBackend` is fallback for unknown providers (should raise) diff --git a/.gstack/qa-reports/ux-audit.md b/.gstack/qa-reports/ux-audit.md new file mode 100644 index 0000000..bc03165 --- /dev/null +++ b/.gstack/qa-reports/ux-audit.md @@ -0,0 +1,180 @@ +# UX Audit — Notebook LM Desktop + +**Date:** 2026-04-17 +**Method:** Full code trace of every user-facing flow +**Verdict:** UI tokens are coherent. UX has systemic problems: destructive actions with no confirmation, the signature citation feature is 100% dead, a source-panel toggle that silently does nothing, dead components, and a first-run wizard whose model selection is never read. + +--- + +## Severity Summary + +| Severity | Count | +|----------|-------| +| Critical | 10 | +| High | 25 | +| Medium | 30 | +| Low | 13 | + +**Top 5 critical issues:** +- C3: Source panel toggle (`Cmd+/`) does nothing — `SourcePanel.tsx:19` ignores `sourcePanelOpen` +- C4 + C9: Notebook and conversation delete fire without confirmation +- C5: No rename notebook anywhere (no UI, no API) +- C6: No way to delete individual documents +- C8: Aborting mid-stream leaves partial message in history, corrupts next send +- H14: Citation amber left-rule pattern is dead CSS — `MessageBubble.tsx` uses plain ReactMarkdown with no renderer + +## Flow 1: First-Run / Setup +- C1: Model selection step stores `localStorage['notebook-lm-selected-model']` at `SetupWizard.tsx:229` — never read anywhere +- C2: Wizard progress not durable; mid-flow close + Ollama-ready re-open silently marks complete, skipping upload step +- H1: Dropzone only processes `e.dataTransfer.files[0]`; others silently dropped +- H2: Upload failure leaves wizard stuck with toast that vanishes in 4s +- H3: Auto-advance at 800ms with no countdown +- M4: `.wizard-card` has no background/border/radius (`setup-wizard.css:12`) +- L1: Primary button color `#fff` vs rest-of-app `#0c0a09` +- L2: Step dots invisible in idle (color-border over bg-primary) + +## Flow 2: App Shell +- C3: `toggleSourcePanel` store action works but panel ignores `sourcePanelOpen` — only hides when `activeSources.length === 0` +- H4: Sidebar 240px + source 280px are fixed; on 13" screens chat gets ~760px +- H5: Source panel is invisible before first message — no affordance +- M5: Zotero dialog has no Escape handler +- L3: `ConnectionBanner` setInterval with no initial check — 30s delay on bad state + +## Flow 3: Notebooks +- C4: Right-click → Delete notebook fires immediately, no dialog, no undo +- C5: Context menu has only Delete — no rename action exists in UI or API +- H6: Notebook switch shows "Conversation saved" toast incorrectly (save happened during stream, not on switch) +- H7: "+ New" button forces immediate upload — no create-empty-notebook path +- M7: Notebook switch silently wipes messages/sources/conversation +- M9: `.context-menu` uses `--radius-card` (14px) — should be `--radius-modal` (20px) + +## Flow 4: Documents +- C6: No delete document action exists anywhere (no UI, no API) +- C7: No duplicate upload guard — same file uploads silently +- H8: Upload toast vanishes at 4s; large PDFs take 30–60s to index (phantom progress) +- H9: Multi-file sequential upload — failed file's name not reported +- H10: DragOverlay accepts any file type, no accept filter +- M10: `DropZone.tsx` is dead code — imported nowhere +- M11: `DocumentCard.tsx:4–11` uses Tailwind hex (`#ef4444`, `#3b82f6`) not tokens +- L5: Document list truncated at 220px — no "Show all" + +## Flow 5: Chat +- C8: `useChat.ts:20` includes partial aborted message in next send's history → backend sees corrupted exchange +- H14 (shared): **Citation pattern is dead** — ReactMarkdown in `MessageBubble.tsx:25` has no custom renderer; `.cited` / `.cite-marker` styles never activate in any response +- H11: Aborted messages look identical to complete messages +- H12: Retry wipes source panel via `clearMessages` → `setActiveSources([])` +- H13: `pendingSuggest` auto-fills textarea post-wizard with no explanation +- M12: Follow-up chips render after error messages +- M13: Auto-scroll fires on every token; users scrolling up are snapped back down +- M14: User bubble border-radius `18px 18px 4px 18px` hardcoded not tokenized + +## Flow 6: Conversations +- C9: Conversation delete fires without confirm; `...` button is opacity-0-until-hover so cursor is already positioned on it +- C10: `loadConversation` clears current messages **before** the load succeeds — network error wipes current context +- H15: Historical conversation loads message-by-message in a loop, indistinguishable from streaming +- H16: Sources overwritten to last message only (`useConversations.ts:33–38`) +- M15: Escape during rename leaves `renameValue` dirty — next rename pre-fills with abandoned text +- M16: "Show all" expands within fixed 200px scroll container (misleading) +- L7: No absolute timestamp on hover — `timeAgo()` only + +## Flow 7: Source Panel / Citations +- H14 (repeat): Citation treatment dead +- H17: No visual link between a message and its sources — no superscript, no hover highlight, no grouping +- H18: Source panel appearing/disappearing causes 280px layout jump, no transition +- M17: `relevanceColor()` uses hardcoded hex (`SourcePanel.tsx:4–8`) +- M18: `relevance_score` not range-validated — if backend returns 0–1 float, bars are invisible + +## Flow 8: Document Preview +- H19: Click-to-preview triggers a second full PDF load to search highlight text — synchronous for-loop over all pages +- H20: Highlight match ratio 0.3 = 30% word overlap → false positives across paragraphs +- H21: **Entire `DocumentPreview.css` off the design system** — cold neutrals (`#1f1f1f`, `#e5e5e5`) not warm stone tokens +- M19: iframe `onLoad` unreliable for non-PDF → "Loading..." never clears +- M20: `📄`/`📑` emoji in toggle button — inconsistent OS rendering +- M21: `border-radius: 12px` — should be `--radius-modal` (20px) +- L8: Keyboard nav requires manual focus — overlay `tabIndex={-1}` never programmatically focused + +## Flow 9: Command Palette +- M22: **Rules of Hooks violation** — `useState(selectedIndex)` at line 114 is below early-return at line 52. Works today; StrictMode or React compiler will break it +- M23: "Toggle source panel" action — broken (C3) +- M24: `matchScore` treats query as single string — `"note sum"` returns zero results +- L9: No shortcut hints in palette items +- L10: Selected and hovered look identical + +## Flow 10: Keyboard Shortcuts +- M25: `?` blocked when input focused — chat textarea almost always holds focus → `?` is essentially unreachable +- M26: Only 6 shortcuts. No notebook switching, upload, Zotero, preview, conversation nav +- L11: `Cmd+N` collides with macOS "New Window" convention + +## Flow 11: Toasts / Error Feedback +- H22: 4s auto-dismiss + no persistence = no way to retrieve missed errors +- H23: Upload "Processing..." toast phantom (gone in 4s while indexing takes 45s) +- M27: `ConnectionBanner` doesn't monitor Ollama — if Ollama crashes mid-session, banner stays green, chat fails silently +- M28: Raw backend error strings surface to users ("Request failed with status 422") +- L12: Error toasts use `role="status"` instead of `role="alert"` — not announced immediately by screen readers + +## Flow 12: Overflow Menu +- H24: "Clear chat" wipes messages + `activeConversationId` but leaves conversation in sidebar → orphan/ghost conversation +- H25: BibTeX export disabled after any context reset — even with historical messages that have sources +- M29: "Toggle sources" in menu (C3 broken) +- M30: `...` always-visible on chat header but opacity-0-until-hover on conversation rows — inconsistent + +--- + +## Cross-Cutting + +### Missing States +- Critical: `app-error` screen shows raw URL to end users, no retry button +- No empty state for source panel with zero-result queries +- No loading state for conversation history (reads as live generation) +- Chat empty state doesn't prompt to add docs when notebook is empty +- No visual distinction between truncated and complete messages + +### Accessibility +- H: `DocumentCard`, `SourcePanel` have `role="button"` but no `onKeyDown` +- H: Conversation `...` button has no `aria-label` +- M: `CommandPalette` has no `role="dialog"`, no `aria-label`, no focus trap +- M: `DocumentPreview` overlay `tabIndex={-1}` never receives programmatic focus +- L: Reduced-motion rule sets animation-duration to 0.01ms — causes flicker, should be `animation: none !important` + +### Onboarding / Discoverability +- H: No guidance post-wizard on citations, source panel, keyboard shortcuts +- H: Zotero import buried behind command palette search only +- M: Cross-notebook mode only appears with 2+ notebooks +- M: `?` shortcut hint exists on welcome but blocked by textarea focus in-app + +### Data Integrity +- Critical: `setActiveNotebookId` clears `activeConversationId` — any null-reassignment orphans active convo +- H: Aborted stream leaves partial message; included in next send's history +- H: `clearMessages()` is called for 4 different intents (new chat, clear chat, switch, load) with identical side effects + +### Error Recovery +- H: Backend down mid-stream: fallback → also fails → error in message bubble; `ConnectionBanner` up to 30s late +- H: Ollama crash mid-session: no banner, raw error in bubble +- M: Document preview fetch failure has `catch {}` — totally silent (`AppShell.tsx:221–226`) + +### Token Violations +- H: `DocumentPreview.css` entirely off-system +- M: `DocumentCard.tsx` Tailwind hex +- M: `SourcePanel.tsx` hardcoded colors +- M: `setup-wizard.css` primary button color inconsistent +- M: `DocumentPreview.css` border-radius 12px (should be 20px) +- L: Font-weight axis: Bunny Fonts URL loads only 400/500/600/700 but CSS uses 450/550 + +--- + +## Essential Fix Targets + +- `components/layout/SourcePanel.tsx` — C3 (respect `sourcePanelOpen`) +- `components/layout/Sidebar.tsx` — C4, C5, C9 (confirm, rename, confirm) +- `components/chat/MessageBubble.tsx` — H14 (custom ReactMarkdown renderer for citations) +- `hooks/useChat.ts` — C8, H11 +- `DocumentPreview.css` — H21 (full rewrite using tokens) +- `DocumentPreview.tsx` — H19, L8, M20, M21 +- `components/ui/SetupWizard.tsx` — C1, C2, M4 +- `components/documents/DocumentCard.tsx` — C6, M11 +- `components/ui/CommandPalette.tsx` — M22 (hooks ordering) +- `hooks/useConversations.ts` — C10, H15, H16 +- `store/app-store.ts` — source panel toggle semantics, clearMessages decomposition +- `components/ui/ConnectionBanner.tsx` — M27, L3 +- `design-system/tokens.css` — reduced-motion fix +- `index.html` — Bunny Fonts URL update diff --git a/apps/desktop/electron/main.cjs b/apps/desktop/electron/main.cjs index bed10c6..40dd5f9 100644 --- a/apps/desktop/electron/main.cjs +++ b/apps/desktop/electron/main.cjs @@ -162,8 +162,17 @@ ipcMain.handle('dialog:choosePath', async (_, options = {}) => { return result.filePaths[0]; }); +// Only allow http(s) and mailto — never file://, javascript:, or other +// dangerous schemes. A renderer-side XSS via rendered markdown should not +// be able to escalate into arbitrary OS actions. +const SAFE_OPEN_SCHEMES = /^(https?:|mailto:)/i; + ipcMain.handle('app:openExternal', async (_, url) => { - if (!url) return false; + if (typeof url !== 'string' || !url) return false; + if (!SAFE_OPEN_SCHEMES.test(url)) { + console.warn('[security] refused openExternal for scheme:', url.slice(0, 32)); + return false; + } await shell.openExternal(url); return true; }); diff --git a/apps/desktop/src/components/chat/ChatView.tsx b/apps/desktop/src/components/chat/ChatView.tsx index 771507e..c498b1f 100644 --- a/apps/desktop/src/components/chat/ChatView.tsx +++ b/apps/desktop/src/components/chat/ChatView.tsx @@ -169,20 +169,19 @@ export function ChatView({ pendingSuggest, onSuggestConsumed }: { pendingSuggest )} {messages.map((msg, i) => ( { - // Find the user message above this error const userMsg = messages[i - 1]; if (userMsg?.role === 'user') { - // Remove error + user message, re-send + // Drop the failed assistant turn + the triggering user message, + // then resend as a fresh exchange. One atomic setMessages call + // avoids N re-renders that reintroduce streaming flicker. const store = useAppStore.getState(); - const newMessages = messages.slice(0, i - 1); - // We can't directly set messages in store, so clear and re-add - store.clearMessages(); - newMessages.forEach((m) => store.addMessage(m)); + store.setMessages(messages.slice(0, i - 1)); + store.setActiveSources([]); send(userMsg.content); } } diff --git a/apps/desktop/src/components/layout/AppShell.tsx b/apps/desktop/src/components/layout/AppShell.tsx index bc3aa38..8e5516d 100644 --- a/apps/desktop/src/components/layout/AppShell.tsx +++ b/apps/desktop/src/components/layout/AppShell.tsx @@ -63,9 +63,7 @@ export function AppShell() { // Cmd+N — new chat if (e.metaKey && e.key === 'n') { e.preventDefault(); - const s = useAppStore.getState(); - s.clearMessages(); - s.setActiveConversationId(null); + useAppStore.getState().newChat(); return; } // ? — keyboard shortcuts (only when not typing in an input) diff --git a/apps/desktop/src/components/layout/Sidebar.tsx b/apps/desktop/src/components/layout/Sidebar.tsx index ea5a30a..ad27811 100644 --- a/apps/desktop/src/components/layout/Sidebar.tsx +++ b/apps/desktop/src/components/layout/Sidebar.tsx @@ -5,10 +5,15 @@ import { useDocuments } from '../../hooks/useDocuments'; import { useConversations } from '../../hooks/useConversations'; import { DocumentCard } from '../documents/DocumentCard'; import { showToast } from '../ui/Toast'; +import { ConfirmDialog } from '../ui/ConfirmDialog'; import { deleteNotebook } from '../../api'; import { timeAgo } from '../../utils/timeAgo'; import './layout.css'; +type PendingConfirm = + | { kind: 'delete-notebook'; notebookId: string; title: string } + | { kind: 'delete-conversation'; conversationId: string; title: string }; + // Lazy Scholar palette: warm, muted tones that match the stone-sage design system const NOTEBOOK_COLORS = [ '#7c9a82', // sage (primary accent) @@ -45,6 +50,7 @@ export function Sidebar() { const [showAllConversations, setShowAllConversations] = useState(false); const [renamingId, setRenamingId] = useState(null); const [renameValue, setRenameValue] = useState(''); + const [pendingConfirm, setPendingConfirm] = useState(null); const renameInputRef = useRef(null); useEffect(() => { @@ -61,18 +67,45 @@ export function Sidebar() { } }, [renamingId]); - const handleDeleteNotebook = async (notebookId: string) => { - try { - await deleteNotebook(notebookId); - showToast('Notebook deleted', 'success'); - if (activeNotebookId === notebookId) { - select(null); + const askDeleteNotebook = (notebookId: string) => { + const nb = notebooks.find((n) => n.notebook_id === notebookId); + setPendingConfirm({ + kind: 'delete-notebook', + notebookId, + title: nb?.title || 'this notebook', + }); + setContextMenu(null); + }; + + const askDeleteConversation = (conversationId: string) => { + const conv = conversations.find((c) => c.id === conversationId); + setPendingConfirm({ + kind: 'delete-conversation', + conversationId, + title: conv?.title || 'this conversation', + }); + setConvMenu(null); + }; + + const confirmPending = async () => { + if (!pendingConfirm) return; + const current = pendingConfirm; + setPendingConfirm(null); + + if (current.kind === 'delete-notebook') { + try { + await deleteNotebook(current.notebookId); + if (activeNotebookId === current.notebookId) { + select(null); + } + await refreshNotebooks(); + showToast('Notebook deleted', 'success'); + } catch { + showToast('Failed to delete notebook', 'error'); } - await refreshNotebooks(); - } catch (err) { - showToast('Failed to delete notebook', 'error'); + } else if (current.kind === 'delete-conversation') { + await deleteConv(current.conversationId); } - setContextMenu(null); }; const handleNewClick = () => { @@ -99,6 +132,7 @@ export function Sidebar() { renameConv(conversationId, renameValue.trim()); } setRenamingId(null); + setRenameValue(''); }; const visibleConversations = showAllConversations @@ -166,7 +200,10 @@ export function Sidebar() { onBlur={() => handleRenameSubmit(conv.id)} onKeyDown={(e) => { if (e.key === 'Enter') handleRenameSubmit(conv.id); - if (e.key === 'Escape') setRenamingId(null); + if (e.key === 'Escape') { + setRenamingId(null); + setRenameValue(''); + } }} onClick={(e) => e.stopPropagation()} /> @@ -260,7 +297,7 @@ export function Sidebar() { @@ -287,15 +324,40 @@ export function Sidebar() { )} + + setPendingConfirm(null)} + /> + + setPendingConfirm(null)} + /> ); } diff --git a/apps/desktop/src/components/ui/CommandPalette.tsx b/apps/desktop/src/components/ui/CommandPalette.tsx index aee1912..53698a5 100644 --- a/apps/desktop/src/components/ui/CommandPalette.tsx +++ b/apps/desktop/src/components/ui/CommandPalette.tsx @@ -20,14 +20,14 @@ function matchScore(query: string, label: string): number { export function CommandPalette({ open, onClose, onZoteroImport }: { open: boolean; onClose: () => void; onZoteroImport?: () => void }) { const [query, setQuery] = useState(''); + const [selectedIndex, setSelectedIndex] = useState(0); const inputRef = useRef(null); const notebooks = useAppStore((s) => s.notebooks); const documents = useAppStore((s) => s.documents); const messages = useAppStore((s) => s.messages); const setActiveNotebookId = useAppStore((s) => s.setActiveNotebookId); const toggleSourcePanel = useAppStore((s) => s.toggleSourcePanel); - const clearMessages = useAppStore((s) => s.clearMessages); - const setActiveConversationId = useAppStore((s) => s.setActiveConversationId); + const newChat = useAppStore((s) => s.newChat); const setPreviewDocument = useAppStore((s) => s.setPreviewDocument); useEffect(() => { @@ -49,6 +49,10 @@ export function CommandPalette({ open, onClose, onZoteroImport }: { open: boolea return () => window.removeEventListener('keydown', handler); }, [open, onClose]); + useEffect(() => { + setSelectedIndex(0); + }, [query]); + if (!open) return null; const allItems: PaletteItem[] = [ @@ -68,7 +72,7 @@ export function CommandPalette({ open, onClose, onZoteroImport }: { open: boolea id: 'action-new-chat', label: 'New chat', section: 'Actions', - onSelect: () => { clearMessages(); setActiveConversationId(null); onClose(); }, + onSelect: () => { newChat(); onClose(); }, }, { id: 'action-toggle-sources', @@ -111,13 +115,8 @@ export function CommandPalette({ open, onClose, onZoteroImport }: { open: boolea } } - const [selectedIndex, setSelectedIndex] = useState(0); const flatItems = sections.flatMap((s) => s.items); - useEffect(() => { - setSelectedIndex(0); - }, [query]); - const handleKeyDown = (e: React.KeyboardEvent) => { if (e.key === 'ArrowDown') { e.preventDefault(); diff --git a/apps/desktop/src/components/ui/ConfirmDialog.tsx b/apps/desktop/src/components/ui/ConfirmDialog.tsx new file mode 100644 index 0000000..f362272 --- /dev/null +++ b/apps/desktop/src/components/ui/ConfirmDialog.tsx @@ -0,0 +1,93 @@ +import { useEffect, useRef } from 'react'; +import './confirm-dialog.css'; + +export interface ConfirmDialogProps { + open: boolean; + title: string; + message: string; + /** Destructive actions get an error-toned confirm button. */ + danger?: boolean; + confirmLabel?: string; + cancelLabel?: string; + onConfirm: () => void; + onCancel: () => void; +} + +export function ConfirmDialog({ + open, + title, + message, + danger = false, + confirmLabel = 'Confirm', + cancelLabel = 'Cancel', + onConfirm, + onCancel, +}: ConfirmDialogProps) { + const cancelRef = useRef(null); + + useEffect(() => { + if (!open) return; + // Focus the Cancel button by default — the safe option is the default. + const raf = requestAnimationFrame(() => cancelRef.current?.focus()); + + const handler = (e: KeyboardEvent) => { + if (e.key === 'Escape') { + e.preventDefault(); + onCancel(); + } else if (e.key === 'Enter') { + // Only fire confirm if the focused element IS the confirm button — + // don't nuke on every Enter keypress. + const active = document.activeElement; + if (active instanceof HTMLButtonElement && active.dataset.role === 'confirm') { + e.preventDefault(); + onConfirm(); + } + } + }; + window.addEventListener('keydown', handler); + return () => { + window.removeEventListener('keydown', handler); + cancelAnimationFrame(raf); + }; + }, [open, onCancel, onConfirm]); + + if (!open) return null; + + return ( +
+
e.stopPropagation()} + > +

+ {title} +

+

+ {message} +

+
+ + +
+
+
+ ); +} diff --git a/apps/desktop/src/components/ui/confirm-dialog.css b/apps/desktop/src/components/ui/confirm-dialog.css new file mode 100644 index 0000000..a57d1bf --- /dev/null +++ b/apps/desktop/src/components/ui/confirm-dialog.css @@ -0,0 +1,112 @@ +.confirm-backdrop { + position: fixed; + inset: 0; + background: rgba(0, 0, 0, 0.5); + backdrop-filter: blur(2px); + display: flex; + align-items: center; + justify-content: center; + z-index: 1100; + animation: confirmBackdropIn 150ms var(--ease-out); +} + +@keyframes confirmBackdropIn { + from { opacity: 0; } + to { opacity: 1; } +} + +.confirm-dialog { + width: 420px; + max-width: 90vw; + padding: var(--space-6) var(--space-6) var(--space-5); + background: var(--color-bg-secondary); + border: 1px solid var(--color-border); + border-radius: var(--radius-modal); + box-shadow: var(--shadow-lg); + animation: confirmDialogIn 180ms var(--ease-out); +} + +@keyframes confirmDialogIn { + from { opacity: 0; transform: translateY(8px) scale(0.98); } + to { opacity: 1; transform: translateY(0) scale(1); } +} + +.confirm-title { + margin: 0 0 var(--space-2); + font-family: var(--font-sans); + font-size: var(--text-lg); + font-weight: 600; + letter-spacing: -0.01em; + color: var(--color-text-primary); +} + +.confirm-message { + margin: 0 0 var(--space-6); + font-size: var(--text-sm); + line-height: 1.55; + color: var(--color-text-secondary); +} + +.confirm-actions { + display: flex; + gap: var(--space-2); + justify-content: flex-end; +} + +.confirm-btn { + font-family: var(--font-sans); + font-size: var(--text-sm); + font-weight: 500; + height: 34px; + padding: 0 var(--space-5); + border-radius: var(--radius-pill); + cursor: pointer; + border: 1px solid transparent; + transition: background var(--duration-fast) var(--ease-out), + border-color var(--duration-fast) var(--ease-out), + color var(--duration-fast) var(--ease-out), + transform 60ms ease; +} + +.confirm-btn:active { + transform: scale(0.97); +} + +.confirm-btn:focus-visible { + outline: none; + box-shadow: 0 0 0 3px var(--color-accent-subtle); +} + +.confirm-btn-cancel { + background: transparent; + border-color: var(--color-border); + color: var(--color-text-secondary); +} + +.confirm-btn-cancel:hover { + background: var(--color-bg-hover); + color: var(--color-text-primary); + border-color: var(--color-border-hover); +} + +.confirm-btn-primary { + background: var(--color-accent); + color: #0c0a09; +} + +.confirm-btn-primary:hover { + background: var(--color-accent-hover); +} + +.confirm-btn-danger { + background: var(--color-error); + color: #0c0a09; +} + +.confirm-btn-danger:hover { + background: #e9546a; +} + +.confirm-btn-danger:focus-visible { + box-shadow: 0 0 0 3px rgba(251, 113, 133, 0.25); +} diff --git a/apps/desktop/src/hooks/useChat.ts b/apps/desktop/src/hooks/useChat.ts index 34e5b30..6746063 100644 --- a/apps/desktop/src/hooks/useChat.ts +++ b/apps/desktop/src/hooks/useChat.ts @@ -1,4 +1,4 @@ -import { useCallback, useRef } from 'react'; +import { useCallback, useEffect, useRef } from 'react'; import { useAppStore } from '../store/app-store'; import { streamChatMessage, sendChatMessage, listConversations } from '../api'; import { showToast } from '../components/ui/Toast'; @@ -7,30 +7,48 @@ import type { ChatStreamEvent, SourceChunk } from '../types'; export function useChat() { const messages = useAppStore((s) => s.messages); const isStreaming = useAppStore((s) => s.isStreaming); + const activeNotebookId = useAppStore((s) => s.activeNotebookId); - const assistantIndexRef = useRef(null); + const assistantIdRef = useRef(null); const bufferRef = useRef(''); const abortRef = useRef(null); + // True only for genuine user-initiated aborts, so we know not to fall back to + // the non-streaming endpoint (otherwise "stop" still delivers the full reply). + const userAbortedRef = useRef(false); + + // Cancel any in-flight stream when the user switches notebooks. The store's + // setActiveNotebookId also clears messages and activeSources, so we just + // need to tear down the network side here. + useEffect(() => { + return () => { + if (abortRef.current) { + userAbortedRef.current = true; + abortRef.current.abort(); + abortRef.current = null; + } + }; + }, [activeNotebookId]); const send = useCallback( async (prompt: string) => { if (!prompt.trim() || useAppStore.getState().isStreaming) return; const store = useAppStore.getState(); - const history = store.messages.map((m) => ({ role: m.role, content: m.content })); + // Build history from only the messages visible to the user. A previously + // aborted assistant turn, if still on screen, was already pruned below — + // but filter defensively in case. + const history = store.messages + .filter((m) => !(m.role === 'assistant' && m.aborted && m.content === '')) + .map((m) => ({ role: m.role, content: m.content })); store.addMessage({ role: 'user', content: prompt }); - - const afterUser = useAppStore.getState().messages; - const assistantIndex = afterUser.length; - store.addMessage({ role: 'assistant', content: '' }); - assistantIndexRef.current = assistantIndex; + const assistantId = store.addMessage({ role: 'assistant', content: '', streaming: true }); + assistantIdRef.current = assistantId; bufferRef.current = ''; store.setIsStreaming(true); store.setActiveSources([]); - // Cross-notebook mode: send all notebook IDs instead of just one const crossMode = store.crossNotebookMode; const body = { prompt, @@ -42,6 +60,7 @@ export function useChat() { const handleEvent = (event: ChatStreamEvent) => { const s = useAppStore.getState(); + const id = assistantIdRef.current; switch (event.type) { case 'meta': { const sources: SourceChunk[] = (event.sources ?? []).map((src) => ({ @@ -50,15 +69,19 @@ export function useChat() { relevance_score: src.relevance_score, })); s.setActiveSources(sources); - // Capture conversation_id from backend (created on first message) if (event.conversation_id && !s.activeConversationId) { s.setActiveConversationId(event.conversation_id); - // Optimistic title: use first user message const firstMsg = s.messages.find((m) => m.role === 'user'); if (firstMsg) { const title = firstMsg.content.slice(0, 50).trim(); s.setConversations([ - { id: event.conversation_id, notebook_id: s.activeNotebookId ?? '', title, created_at: new Date().toISOString(), updated_at: new Date().toISOString() }, + { + id: event.conversation_id, + notebook_id: s.activeNotebookId ?? '', + title, + created_at: new Date().toISOString(), + updated_at: new Date().toISOString(), + }, ...s.conversations, ]); } @@ -67,17 +90,12 @@ export function useChat() { } case 'token': bufferRef.current += event.delta; - if (assistantIndexRef.current !== null) { - s.updateMessageAt(assistantIndexRef.current, bufferRef.current); - } + if (id) s.updateMessage(id, { content: bufferRef.current }); break; case 'done': - if (assistantIndexRef.current !== null) { - s.updateMessageAt(assistantIndexRef.current, event.reply); - } + if (id) s.updateMessage(id, { content: event.reply, streaming: false }); s.setIsStreaming(false); - assistantIndexRef.current = null; - // Refresh conversation list to get backend-accurate titles + assistantIdRef.current = null; if (s.activeNotebookId) { listConversations(s.activeNotebookId) .then((convs) => useAppStore.getState().setConversations(convs)) @@ -85,11 +103,9 @@ export function useChat() { } break; case 'error': - if (assistantIndexRef.current !== null) { - s.updateMessageAt(assistantIndexRef.current, `Error: ${event.message}`); - } + if (id) s.updateMessage(id, { content: `Error: ${event.message}`, streaming: false }); s.setIsStreaming(false); - assistantIndexRef.current = null; + assistantIdRef.current = null; break; case 'warning': showToast(event.message, 'error'); @@ -99,26 +115,52 @@ export function useChat() { const controller = new AbortController(); abortRef.current = controller; + userAbortedRef.current = false; try { await streamChatMessage(body, handleEvent, controller.signal); abortRef.current = null; - } catch { + } catch (err) { + const wasUserAbort = userAbortedRef.current || controller.signal.aborted; abortRef.current = null; + + if (wasUserAbort) { + // User clicked stop. Mark the partial message as aborted and bail — + // do NOT fall back to the non-streaming endpoint (that would ignore + // the abort and deliver the full reply anyway). + const id = assistantIdRef.current; + if (id) { + const state = useAppStore.getState(); + const msg = state.messages.find((m) => m.id === id); + if (msg && msg.content.length === 0) { + // Nothing to keep — remove the empty assistant bubble entirely. + state.removeMessage(id); + } else if (id) { + state.updateMessage(id, { streaming: false, aborted: true }); + } + } + useAppStore.getState().setIsStreaming(false); + assistantIdRef.current = null; + return; + } + + // Real stream failure — try non-streaming fallback, but respect abort. try { const response = await sendChatMessage(body); - if (assistantIndexRef.current !== null) { - useAppStore.getState().updateMessageAt(assistantIndexRef.current, response.reply); - } + const id = assistantIdRef.current; + if (id) useAppStore.getState().updateMessage(id, { content: response.reply, streaming: false }); } catch (fallbackErr) { - if (assistantIndexRef.current !== null) { + const id = assistantIdRef.current; + if (id) { const msg = fallbackErr instanceof Error ? fallbackErr.message : 'Failed to get response'; - useAppStore.getState().updateMessageAt(assistantIndexRef.current, `Error: ${msg}`); + useAppStore.getState().updateMessage(id, { content: `Error: ${msg}`, streaming: false }); } + // Original stream error tells us more than the fallback one, usually. + console.warn('[chat] stream failed, fallback also failed', err, fallbackErr); } finally { useAppStore.getState().setIsStreaming(false); - assistantIndexRef.current = null; + assistantIdRef.current = null; } } }, @@ -126,13 +168,19 @@ export function useChat() { ); const clearChat = useCallback(() => { - const s = useAppStore.getState(); - s.clearMessages(); - s.setActiveConversationId(null); + // Also tear down any in-flight stream so it doesn't keep streaming into + // a conversation the user just wiped. + if (abortRef.current) { + userAbortedRef.current = true; + abortRef.current.abort(); + abortRef.current = null; + } + useAppStore.getState().newChat(); }, []); const abort = useCallback(() => { if (abortRef.current) { + userAbortedRef.current = true; abortRef.current.abort(); abortRef.current = null; } diff --git a/apps/desktop/src/hooks/useConversations.ts b/apps/desktop/src/hooks/useConversations.ts index 11ba442..4e4007b 100644 --- a/apps/desktop/src/hooks/useConversations.ts +++ b/apps/desktop/src/hooks/useConversations.ts @@ -27,20 +27,35 @@ export function useConversations() { const loadConversation = useCallback(async (conversationId: string) => { const store = useAppStore.getState(); - store.setActiveConversationId(conversationId); - store.clearMessages(); + // Fetch first so we don't destroy the current context on a network error. + let msgs; try { - const msgs = await getConversationMessages(conversationId); - for (const msg of msgs) { - store.addMessage({ role: msg.role as 'user' | 'assistant', content: msg.content }); - if (msg.sources) { - store.setActiveSources(msg.sources); - } - } + msgs = await getConversationMessages(conversationId); } catch { showToast('Failed to load conversation', 'error'); - store.setActiveConversationId(null); + return; } + + // Normalize historical sources so document_name is populated (same shape + // the streaming path produces). Without this the source panel renders + // blank entries on reload. + const normalizedMessages = msgs.map((msg) => ({ + id: msg.id, + role: msg.role as 'user' | 'assistant', + content: msg.content, + })); + const lastWithSources = [...msgs].reverse().find((m) => m.sources && m.sources.length > 0); + const restoredSources = lastWithSources?.sources + ? lastWithSources.sources.map((s) => ({ + ...s, + document_name: s.document_name ?? (s.source_path.split(/[/\\]/).pop() ?? s.source_path), + })) + : []; + + // Now swap state in one shot — no incremental renders, no torn intermediates. + store.setActiveConversationId(conversationId); + store.setMessages(normalizedMessages); + store.setActiveSources(restoredSources); }, []); const deleteConv = useCallback(async (conversationId: string) => { @@ -48,8 +63,7 @@ export function useConversations() { await apiDeleteConversation(conversationId); const store = useAppStore.getState(); if (store.activeConversationId === conversationId) { - store.setActiveConversationId(null); - store.clearMessages(); + store.newChat(); } await refresh(); showToast('Conversation deleted', 'success'); diff --git a/apps/desktop/src/store/app-store.ts b/apps/desktop/src/store/app-store.ts index a6782a9..17b17d9 100644 --- a/apps/desktop/src/store/app-store.ts +++ b/apps/desktop/src/store/app-store.ts @@ -2,6 +2,14 @@ import { create } from 'zustand'; import type { BackendConfig, ChatMessage, Conversation, DocumentInfo, Notebook, SourceChunk } from '../types'; +function makeId(): string { + // crypto.randomUUID is available in Electron's renderer and modern browsers. + if (typeof crypto !== 'undefined' && typeof crypto.randomUUID === 'function') { + return crypto.randomUUID(); + } + return `msg_${Date.now()}_${Math.random().toString(36).slice(2, 10)}`; +} + interface AppState { // Connection status: 'checking' | 'ready' | 'error'; @@ -39,9 +47,19 @@ interface AppState { setActiveNotebookId: (id: string | null) => void; // Actions — chat - addMessage: (msg: ChatMessage) => void; - updateMessageAt: (index: number, content: string) => void; - clearMessages: () => void; + /** Add a message. If `msg.id` is omitted, a stable id is generated. Returns the id. */ + addMessage: (msg: Omit & { id?: string }) => string; + /** Replace the content of a message by id. Marks streaming=false when done. */ + updateMessage: (id: string, patch: Partial>) => void; + /** Remove a message by id (used when the user aborts an empty assistant turn). */ + removeMessage: (id: string) => void; + /** Bulk replace messages — used when loading a historical conversation. */ + setMessages: (messages: ChatMessage[]) => void; + /** Clear on-screen chat AND forget the active conversation (user-initiated "new chat"). */ + newChat: () => void; + /** Clear on-screen chat only — used internally when loading a different conversation. + * Does NOT touch activeConversationId so callers can restore state on fetch failure. */ + resetMessagesOnly: () => void; setIsStreaming: (val: boolean) => void; // Actions — conversations @@ -57,6 +75,7 @@ interface AppState { // Actions — sources & UI setActiveSources: (sources: SourceChunk[]) => void; toggleSourcePanel: () => void; + setSourcePanelOpen: (open: boolean) => void; setPreviewDocument: (doc: DocumentInfo | null) => void; } @@ -80,22 +99,34 @@ export const useAppStore = create((set) => ({ setStatus: (status) => set({ status }), setConfig: (config) => set({ config }), - // Notebooks — switching clears chat and documents + // Notebooks — switching resets notebook-scoped state. Conversation and + // in-flight stream should be canceled by the caller BEFORE this is invoked. setNotebooks: (notebooks) => set({ notebooks }), setActiveNotebookId: (id) => - set({ activeNotebookId: id, messages: [], activeSources: [], documents: [], activeConversationId: null }), + set({ + activeNotebookId: id, + messages: [], + activeSources: [], + documents: [], + activeConversationId: null, + }), // Chat - addMessage: (msg) => set((state) => ({ messages: [...state.messages, msg] })), - updateMessageAt: (index, content) => - set((state) => { - const messages = [...state.messages]; - if (messages[index]) { - messages[index] = { ...messages[index], content }; - } - return { messages }; - }), - clearMessages: () => set({ messages: [], activeSources: [] }), + addMessage: (msg) => { + const id = msg.id ?? makeId(); + set((state) => ({ messages: [...state.messages, { ...msg, id }] })); + return id; + }, + updateMessage: (id, patch) => + set((state) => ({ + messages: state.messages.map((m) => (m.id === id ? { ...m, ...patch } : m)), + })), + removeMessage: (id) => + set((state) => ({ messages: state.messages.filter((m) => m.id !== id) })), + setMessages: (messages) => set({ messages }), + newChat: () => + set({ messages: [], activeSources: [], activeConversationId: null, isStreaming: false }), + resetMessagesOnly: () => set({ messages: [], activeSources: [] }), setIsStreaming: (val) => set({ isStreaming: val }), // Conversations @@ -111,5 +142,6 @@ export const useAppStore = create((set) => ({ // Sources & UI setActiveSources: (sources) => set({ activeSources: sources }), toggleSourcePanel: () => set((state) => ({ sourcePanelOpen: !state.sourcePanelOpen })), + setSourcePanelOpen: (open) => set({ sourcePanelOpen: open }), setPreviewDocument: (doc) => set({ previewDocument: doc }), })); diff --git a/apps/desktop/src/types.ts b/apps/desktop/src/types.ts index d425e3f..ed58e17 100644 --- a/apps/desktop/src/types.ts +++ b/apps/desktop/src/types.ts @@ -1,11 +1,24 @@ export interface ChatMessage { + /** Stable client-side id. Assigned at add-time so streaming updates and + * aborts survive re-renders, notebook switches, and history edits. */ + id: string; + role: 'user' | 'assistant'; + content: string; + /** True while the assistant reply is still streaming. */ + streaming?: boolean; + /** True if the user aborted this assistant reply. */ + aborted?: boolean; +} + +/** Wire shape the backend expects — no client-side id field. */ +export interface ChatHistoryItem { role: 'user' | 'assistant'; content: string; } export interface ChatRequest { prompt: string; - history?: ChatMessage[]; + history?: ChatHistoryItem[]; notebook_id?: string | null; notebook_ids?: string[] | null; conversation_id?: string | null; diff --git a/apps/desktop/vite.config.ts b/apps/desktop/vite.config.ts index 8b0f57b..d4b1b39 100644 --- a/apps/desktop/vite.config.ts +++ b/apps/desktop/vite.config.ts @@ -4,4 +4,7 @@ import react from '@vitejs/plugin-react' // https://vite.dev/config/ export default defineConfig({ plugins: [react()], + // Relative base — packaged Electron loads the renderer from file:///…/dist/index.html, + // which breaks absolute /assets/… URLs. + base: './', }) diff --git a/backend/notebooklm_backend/app.py b/backend/notebooklm_backend/app.py index 8fff1b8..99d9e1c 100644 --- a/backend/notebooklm_backend/app.py +++ b/backend/notebooklm_backend/app.py @@ -75,7 +75,10 @@ def create_app() -> FastAPI: app.state.speech_service = SpeechService(settings) app.state.agent_service = agent_service - # Allow renderer (http://localhost:5173) to call the API in dev; loosened in v1 for simplicity. + # Explicit localhost dev origins + Electron's file:// origin ("null"). + # We used to allow_origin_regex=".*" with credentials — a localhost CSRF + # footgun (any browser tab on the machine could hit the API). Credentials + # aren't used, so leave them off. app.add_middleware( CORSMiddleware, allow_origins=[ @@ -83,8 +86,7 @@ def create_app() -> FastAPI: "http://127.0.0.1:5173", "null", # Electron file:// origin ], - allow_origin_regex=".*", - allow_credentials=True, + allow_credentials=False, allow_methods=["*"], allow_headers=["*"], ) diff --git a/backend/notebooklm_backend/routes/documents.py b/backend/notebooklm_backend/routes/documents.py index 473f7b4..da0e30f 100644 --- a/backend/notebooklm_backend/routes/documents.py +++ b/backend/notebooklm_backend/routes/documents.py @@ -163,71 +163,55 @@ async def preview_document( from urllib.parse import unquote settings: AppConfig = request.app.state.settings - + # Decode URL-encoded path decoded_path = unquote(source_path) - file_path = Path(decoded_path) - filename = file_path.name - - # Security: ensure the file is within the uploads directory - uploads_dir = settings.data_dir / "uploads" / notebook_id - uploads_base = settings.data_dir / "uploads" - + incoming_path = Path(decoded_path) + filename = incoming_path.name + + # The notebook-scoped uploads directory is the only path we'll serve from. + # Previous versions had a cross-notebook fallback that let any notebook + # serve any filename — a disclosure bug we're deliberately closing here. + uploads_dir = (settings.data_dir / "uploads" / notebook_id).resolve() + import logging - + try: - # Strategy: Try multiple locations in order of preference - # 1. New format: uploads/{notebook_id}/filename (most likely) - potential_path = uploads_dir / filename - if potential_path.exists(): - file_path = potential_path - logging.info(f"Found file at: {file_path}") - # 2. Absolute path from metadata (if it exists and is within uploads) - elif file_path.is_absolute() and file_path.exists(): - resolved_path = file_path.resolve() - resolved_uploads_base = uploads_base.resolve() - - if not str(resolved_path).startswith(str(resolved_uploads_base)): + # Resolve the candidate path within the notebook's scoped directory. + # Strategy 1: standard new format — uploads/{notebook_id}/filename. + candidate = (uploads_dir / filename).resolve() + + # Strategy 2: an absolute path from older metadata — accept only if + # it resolves *inside* this notebook's uploads_dir. + if not candidate.exists() and incoming_path.is_absolute(): + abs_candidate = incoming_path.resolve() + try: + abs_candidate.relative_to(uploads_dir) + candidate = abs_candidate + except ValueError: + # Absolute path escapes this notebook's directory — refuse. raise HTTPException(status_code=403, detail="Access denied") - - file_path = resolved_path - logging.info(f"Found file at absolute path: {file_path}") - # 3. Old format: uploads/filename (for backward compatibility) - elif (uploads_base / filename).exists(): - file_path = uploads_base / filename - logging.info(f"Found file at old location: {file_path}") - # 4. Search in all notebook subdirectories - else: - found = False - if uploads_base.exists(): - for subdir in uploads_base.iterdir(): - if subdir.is_dir(): - candidate = subdir / filename - if candidate.exists(): - file_path = candidate - found = True - logging.info(f"Found file in subdirectory: {file_path}") - break - - if not found: - # Log all possible locations for debugging - logging.error(f"File not found: {filename}") - logging.error(f"Searched in: {uploads_dir}") - logging.error(f"Searched in: {uploads_base}") - if uploads_base.exists(): - logging.error(f"Subdirectories: {list(uploads_base.iterdir())}") - raise HTTPException( - status_code=404, - detail=f"File not found: {filename}. Notebook ID: {notebook_id}", - ) - - if not file_path.exists(): - raise HTTPException(status_code=404, detail=f"File not found: {file_path}") - + + # Final guard: candidate must exist AND sit inside uploads_dir + # (protects against `..` traversal in the filename). + if not candidate.exists(): + logging.warning("preview 404: notebook=%s filename=%s", notebook_id, filename) + raise HTTPException( + status_code=404, + detail=f"File not found in notebook: {filename}", + ) + try: + candidate.relative_to(uploads_dir) + except ValueError: + raise HTTPException(status_code=403, detail="Access denied") + return FileResponse( - path=str(file_path), - filename=file_path.name, + path=str(candidate), + filename=candidate.name, media_type="application/octet-stream", + headers={ + "Content-Disposition": f'attachment; filename="{candidate.name}"', + }, ) except HTTPException: raise diff --git a/backend/notebooklm_backend/routes/notebooks.py b/backend/notebooklm_backend/routes/notebooks.py index 079889e..fd29fcf 100644 --- a/backend/notebooklm_backend/routes/notebooks.py +++ b/backend/notebooklm_backend/routes/notebooks.py @@ -1,5 +1,7 @@ from __future__ import annotations +import logging +import shutil import uuid from datetime import datetime, timezone @@ -8,6 +10,8 @@ from ..models.notebook import CreateNotebookRequest, IngestionJobStatus, NotebookMetadata from ..services.notebook_store import NotebookStore +logger = logging.getLogger(__name__) + router = APIRouter(prefix="/notebooks", tags=["notebooks"]) @@ -36,11 +40,32 @@ async def create_notebook(request: Request, body: CreateNotebookRequest) -> Note @router.delete("/{notebook_id}") async def delete_notebook(request: Request, notebook_id: str) -> dict[str, str]: store: NotebookStore = request.app.state.notebook_store - # Cascade: delete conversations for this notebook first + settings = request.app.state.settings + vector_store = request.app.state.vector_store + + # 1) Cascade: delete conversations for this notebook (messages go with them). from ..services.conversation_store import ConversationStore conv_store: ConversationStore = request.app.state.conversation_store conv_store.delete_conversations_for_notebook(notebook_id) + + # 2) Drop the Chroma collection(s) for this notebook so vectors don't + # leak into future queries and disk isn't held forever. + try: + vector_store.delete_notebook_collections(notebook_id) + except Exception: + logger.exception("Failed to delete Chroma collections for notebook %s", notebook_id) + + # 3) Remove the uploads directory for this notebook. + uploads_dir = settings.data_dir / "uploads" / notebook_id + if uploads_dir.exists(): + try: + shutil.rmtree(uploads_dir) + except Exception: + logger.exception("Failed to remove uploads dir %s", uploads_dir) + + # 4) Finally remove the SQLite row. store.delete_notebook(notebook_id) + return {"status": "deleted", "notebook_id": notebook_id} diff --git a/backend/notebooklm_backend/services/notebook_store.py b/backend/notebooklm_backend/services/notebook_store.py index b4165d1..719d00a 100644 --- a/backend/notebooklm_backend/services/notebook_store.py +++ b/backend/notebooklm_backend/services/notebook_store.py @@ -177,10 +177,14 @@ def complete_ingestion( """, ("completed", message, now.isoformat(), source_count, chunk_count, job_id), ) + # Accumulate counts across multiple ingestion jobs for the same notebook + # (a 6th upload should leave source_count at 6, not reset to 1). conn.execute( """ UPDATE notebooks - SET source_count = ?, chunk_count = ?, updated_at = ? + SET source_count = source_count + ?, + chunk_count = chunk_count + ?, + updated_at = ? WHERE notebook_id = ? """, (source_count, chunk_count, now.isoformat(), notebook_id), diff --git a/backend/notebooklm_backend/services/vector_store.py b/backend/notebooklm_backend/services/vector_store.py index e57e535..7588c3f 100644 --- a/backend/notebooklm_backend/services/vector_store.py +++ b/backend/notebooklm_backend/services/vector_store.py @@ -23,6 +23,21 @@ def _collection_name(self, notebook_id: str) -> str: def get_collection(self, notebook_id: str) -> Collection: return self.client.get_or_create_collection(name=self._collection_name(notebook_id)) + def delete_notebook_collections(self, notebook_id: str) -> None: + """Drop the chunk and summary collections for a deleted notebook. + + Both deletes are best-effort: missing collections are not an error. + """ + for name in ( + self._collection_name(notebook_id), + self._doc_summaries_collection_name(notebook_id), + ): + try: + self.client.delete_collection(name=name) + except Exception: + # Collection may not exist yet for never-ingested notebooks. + pass + def add_chunks(self, notebook_id: str, chunks: Iterable[TextChunk]) -> int: chunk_list = list(chunks) if not chunk_list: diff --git a/backend/tests/test_notebook_store.py b/backend/tests/test_notebook_store.py index f4e5fc7..ab52b47 100644 --- a/backend/tests/test_notebook_store.py +++ b/backend/tests/test_notebook_store.py @@ -28,3 +28,28 @@ def test_notebook_store_persistence(tmp_path): jobs = store.list_jobs() assert jobs[0].status == "completed" + + +def test_complete_ingestion_accumulates_counts_across_jobs(tmp_path): + """Regression: a 2nd upload to the same notebook must add to source_count, + not overwrite it with the per-job value.""" + settings = AppConfig( + workspace_root=tmp_path, + data_dir=tmp_path / "data", + models_dir=tmp_path / "models", + index_dir=tmp_path / "indexes", + cache_dir=tmp_path / "cache", + ) + store = NotebookStore(settings) + first = store.start_ingestion(NotebookIngestionRequest(path="a.md", title="NB")) + store.complete_ingestion(first.job_id, "done", source_count=3, chunk_count=12) + + second = store.start_ingestion( + NotebookIngestionRequest(path="b.md", title="NB", notebook_id=first.notebook_id) + ) + store.complete_ingestion(second.job_id, "done", source_count=2, chunk_count=7) + + nb = store.get_notebook(first.notebook_id) + assert nb is not None + assert nb.source_count == 5 # 3 + 2 + assert nb.chunk_count == 19 # 12 + 7