kite/.planning/research/PITFALLS.md

# Domain Pitfalls — v0.2 UI Overhaul and Optimization

**Project:** DocuVault
**Milestone:** v0.2 — UI Overhaul and Optimization on existing FastAPI + Vue 3 app
**Researched:** 2026-06-07
**Confidence:** HIGH (all pitfalls grounded in actual source files: router/index.js, App.vue, main.py, client.js, StorageBrowser.vue, DocumentCard.vue, DropZone.vue, AppSidebar.vue, and all api/*.py router files)

---

## Critical Pitfalls

Mistakes that cause regressions, security gaps, or rewrites.

---

### Pitfall 1: FastAPI Router Splitting — Prefix Already Embedded in APIRouter, Not in include_router

**Phase:** Backend refactoring (split large router files)

**What goes wrong:** Every existing router in this codebase declares its prefix on the `APIRouter` instance itself — `router = APIRouter(prefix="/api/documents", tags=["documents"])` — NOT on the `include_router(router, prefix=...)` call in `main.py`. When splitting a large file (e.g. `documents.py` at 852 lines, `admin.py` at 934 lines) into sub-modules, the new sub-routers pick up their endpoints via an `include_router` call inside the original module, or they are wired directly in `main.py`. Either way, if you add a prefix on `include_router` in addition to the prefix on the inner `APIRouter`, all URLs double-prefix to `/api/documents/api/documents/...`. Likewise, if you strip the prefix from the inner router and forget to add it to `include_router`, all URLs return 404.

**Why it happens:** The original pattern (`router = APIRouter(prefix=...)` in the module + `app.include_router(router)` without a prefix in `main.py`) is unusual but consistent in this project. Developers splitting a file expect the standard pattern where prefixes go on `include_router`, not on the router definition.

**Consequences:** 404 on all endpoints in the split module; existing tests and the frontend `client.js` (which hard-codes `/api/documents`, `/api/admin`, etc.) silently break.

**Prevention:** When splitting, create sub-routers with NO prefix: `sub = APIRouter(tags=["documents-upload"])`. Register them inside the parent module with `router.include_router(sub)` so the parent's prefix propagates. The parent module keeps its existing prefix and its existing `app.include_router(parent)` call in `main.py` is unchanged.

**Detection warning signs:** Any test that calls `/api/documents/*` returns 404 immediately after the split; Swagger UI shows doubled path segments.

---

### Pitfall 2: FastAPI Router Splitting — Circular Import via Shared Dependencies

**Phase:** Backend refactoring

**What goes wrong:** `folders.py` already exports a `document_move_router = APIRouter(prefix="/api/documents")` — a cross-cutting concern documented in the file (line 439-442). When splitting `documents.py`, if the new sub-module imports anything from `api/folders.py` (e.g. a shared validator or Pydantic schema), Python raises `ImportError: cannot import name X` at startup because both modules are in partial initialization state.

**Why it happens:** FastAPI modules that share models and validators tend to grow cross-references. The `document_move_router` in `folders.py` under the `/api/documents` prefix is the existing example — any new import in the opposite direction closes the import cycle.

**Consequences:** Application fails to start. Docker container exits immediately.

**Prevention:** Shared Pydantic models and validators must live in `db/models.py` or a dedicated `schemas/` module — not in router files. Before splitting, grep for any `from api.X import` inside the router files being split and move the shared symbols to a neutral module first.

**Detection warning signs:** `ImportError` or `cannot import name` in the uvicorn startup log.

---

### Pitfall 3: AdminLayout Vue Router Integration — beforeEach Guard Misses /admin/* Children

**Phase:** Admin panel rearchitecture

**What goes wrong:** The current guard in `router/index.js` (lines 81-93) checks `to.meta.requiresAdmin` on the matched route object. The current `/admin` route is a flat route with that meta set directly. When `/admin` becomes a parent route with nested children (`/admin/users`, `/admin/audit`, etc.), the children do NOT automatically inherit `meta: { requiresAdmin: true }` from the parent in Vue Router 4 unless the guard iterates `to.matched`. If the children are added without repeating the meta, the guard's `to.meta.requiresAdmin` check evaluates to `undefined` (falsy) for every child route — non-admin users can navigate to `/admin/users` directly.

**Why it happens:** Vue Router 4 does NOT merge parent `meta` into child `meta` by default. `to.meta` reflects only the deepest matched route's own meta, not the parent's.

**Consequences:** Security regression — the admin guard that currently works correctly stops protecting all admin child routes. Non-admin users can access `/admin/users`, `/admin/audit`, etc.

**Prevention:** Update the guard to check `to.matched.some(r => r.meta.requiresAdmin)` instead of `to.meta.requiresAdmin`. This is the correct Vue Router 4 idiom for inherited meta and requires one change to the guard. Apply the same fix to the `!to.meta.public` check if any auth routes gain children.

```javascript
// router/index.js — change:
if (to.meta.requiresAdmin && authStore.user?.role !== 'admin') {
// to:
if (to.matched.some(r => r.meta.requiresAdmin) && authStore.user?.role !== 'admin') {
```

---

### Pitfall 4: AdminLayout Integration — App.vue Layout Switch Breaks for /admin/* Routes

**Phase:** Admin panel rearchitecture

**What goes wrong:** `App.vue` currently switches between `AuthLayout` (when `route.meta.layout === 'auth'`) and the main app shell (sidebar + `<router-view>`) for everything else. The admin panel needs a third layout (`AdminLayout`) with its own nav. The naive approach — `v-else-if="route.meta.layout === 'admin'"` in `App.vue` rendering `AdminLayout` which then renders `<router-view />` — creates double rendering: `App.vue`'s `<router-view>` already resolved the full route. The inner `<router-view>` inside `AdminLayout` gets nothing because the route is already consumed.

**Consequences:** Admin layout renders with an empty content area, or the admin page renders without the admin chrome.

**Prevention:** The correct structure for a standalone admin layout in Vue Router 4:

```javascript
// router/index.js
{
  path: '/admin',
  component: AdminLayout,   // resolves to the layout shell, which contains <router-view />
  meta: { requiresAdmin: true },
  children: [
    { path: '', redirect: '/admin/users' },
    { path: 'users', component: AdminUsersView },
    { path: 'quotas', component: AdminQuotasView },
    { path: 'ai', component: AdminAiView },
    { path: 'audit', component: AdminAuditView },
  ]
}
```

`AdminLayout.vue` contains `<router-view />` where page content renders. `App.vue` remains unchanged — it renders the component that the route resolves to, which is now `AdminLayout`. No third branch in `App.vue` is needed.

---

### Pitfall 5: client.js Splitting — noRefreshPaths Breaks When Paths Move to Sub-modules

**Phase:** Frontend refactoring (client.js split)

**What goes wrong:** `client.js` has a hardcoded `noRefreshPaths` list inside `request()` (line 26) that guards against infinite refresh loops. If `request()` is moved to a shared `api/core.js` module and the path list stays there, it works. But the three blob-download functions (`adminExportAuditLogCsv`, `adminDownloadDailyExport`, `fetchDocumentContent`) each contain a duplicated copy of the 401-refresh-retry logic. When splitting `client.js` into domain files, those duplicates will be copied into new sub-files. This is the correct moment to eliminate them — but if missed, the bug surface multiplies.

**Additionally:** If components are changed to import from sub-files directly (e.g. `import { listDocuments } from '../api/documents.js'`) instead of the barrel, every other component that imports from `client.js` still works — but a future refactor that renames a function in the sub-file will miss the barrel re-export and break components that still use the barrel. Both import paths exist simultaneously and diverge.

**Prevention:** Split in this order:
1. Extract `request()` + `noRefreshPaths` into `api/core.js`. Export `request` from there.
2. Extract the 401-retry pattern from the three blob-download functions into a single `fetchWithRetry(url, headers)` helper also in `api/core.js`.
3. Create domain files (`api/documents.js`, `api/admin.js`, `api/auth.js`, etc.) that import from `api/core.js`.
4. Keep `api/client.js` as a re-export barrel: `export * from './documents.js'; export * from './admin.js'` etc. This preserves all existing `import { listDocuments } from '../api/client.js'` calls without touching any component. Never change component imports to point at sub-files directly.

---

### Pitfall 6: Drag-and-Drop on DocumentCard — dragstart vs @click Conflict

**Phase:** UX improvements (drag-and-drop)

**What goes wrong:** `DocumentCard.vue` navigates on `@click="$router.push('/document/' + doc.id)"` (line 4). Browsers fire `click` after `dragend` when the drag distance is below the browser's drag-initiation threshold — typically ~4px. A user who clicks and releases without moving far enough triggers both `dragstart` and then `click`: the card navigates to the document even though the user intended to start a drag.

**Also:** The action buttons inside `DocumentCard` use `@click.stop`. If a drag handle overlaps an action button area, `@click.stop` prevents the click from propagating but does not prevent `dragstart` from bubbling — behavior becomes inconsistent depending on which pixel the user starts from.

**Note:** `StorageBrowser.vue` already implements drag-and-drop on list rows correctly (lines 141-145: `draggable`, `@dragstart`, `@dragend`) and does not have this problem because the row's `@click` emits `file-open` rather than navigating imperatively. The risk is specific to adding drag to `DocumentCard.vue` (card grid used in `TopicsView`).

**Prevention:**
1. Track drag state with a component-level `dragging` ref. Set `dragging = true` in `@dragstart`, reset to `false` in `@dragend`. Guard `@click`: `if (this.dragging) return`.
2. Preferred: use a dedicated drag handle element (grip icon, `cursor-grab`) and set `draggable="true"` only on that element. Add `@click.stop` on the handle. The rest of the card retains click-to-navigate.

---

### Pitfall 7: Virtual Scrolling — StorageBrowser DOM Assumptions Break

**Phase:** Frontend performance (virtual scrolling)

**What goes wrong:**

`StorageBrowser.vue` renders its item list with `divide-y divide-gray-100` (line 76). The Tailwind `divide-y` utility inserts borders using adjacent-sibling CSS (`& > * + * { border-top-width: 1px }`). This only works when all children are present in the DOM simultaneously. Virtual scroll libraries keep only visible rows in the DOM — `divide-y` produces borders only between the visible subset, leaving gaps at virtual boundaries.

The folder picker dropdown inside file rows (lines 185-213) is `absolute right-0 top-full` — positioned relative to its parent row. Virtual scroll repositions rows via `transform: translateY()`. Absolutely positioned children escape their transformed parent and appear at wrong viewport positions.

CSS transitions on rows (`transition-all`, `transition-colors`) work with `v-for` because Vue animates DOM insertion/removal. Virtual scroll does not insert/remove — it repositions. Vue's `<transition-group>` cannot wrap a virtually scrolled list and will throw warnings.

**Prevention:**
1. Replace `divide-y` with `border-b border-gray-100` on each individual row element. Rows become self-contained and do not depend on DOM adjacency.
2. The folder picker dropdown must use `<Teleport to="body">` with `position: fixed` coordinates computed from `getBoundingClientRect()`. This is a prerequisite for virtual scroll adoption, not optional.
3. Remove any `<transition-group>` wrapping a virtually scrolled list. Use per-row fade-in via a CSS class added on first mount instead.

---

## Moderate Pitfalls

---

### Pitfall 8: Responsive Layout — w-64 Sidebar vs Flex-1 Content on Mobile

**Phase:** Responsive / mobile layout

**What goes wrong:** `App.vue` uses `flex h-screen overflow-hidden` with `AppSidebar` fixed at `w-64 shrink-0`. On screens narrower than ~320px the content area collapses below usable width. `StorageBrowser`'s 5-column grid `grid-cols-[2rem_1fr_6rem_8rem_6rem]` has `hidden sm:block` and `hidden md:block` to hide size and date columns — but the fixed `2rem` icon and `6rem` action columns still consume 128px on either side of the filename, leaving minimal space on a 375px screen.

**Prevention:**
1. On mobile (`< lg`), hide the sidebar. Use a hamburger button in a mobile-only header bar. Toggle with `translate-x-0 / -translate-x-full` transition on the sidebar element.
2. The sidebar open/closed state needs to be in a Pinia store ref (or provided from `App.vue`) so the hamburger button in the header, the sidebar overlay, and the main content area all react to it. Do not put it in a component's `data()`.
3. On mobile, simplify the grid to `grid-cols-[2rem_1fr_4rem]` — filename and a single action column. Test at 375px explicitly.

---

### Pitfall 9: Admin Panel + Responsive — Double Padding After Extracting Tab Content

**Phase:** Admin panel rearchitecture + responsive layout (interaction pitfall)

**What goes wrong:** `AdminView.vue` wraps all content in `p-8 max-w-5xl mx-auto`. Each tab component (`AdminUsersTab.vue`, `AdminQuotasTab.vue`, etc.) has its own internal padding. When tab components are extracted into standalone page views (`AdminUsersView.vue`, etc.), the `p-8 max-w-5xl mx-auto` wrapper moves to `AdminLayout.vue`. If the extracted page views retain their own inner padding, content is double-padded. If `AdminLayout` does not include the constraint, the new admin pages render full-bleed against the viewport.

**Prevention:** When extracting tab components into page views, strip all top-level padding and `max-w` from the extracted component — those belong to `AdminLayout`'s content area. The page components should fill 100% of the space provided by the layout slot.

---

### Pitfall 10: Options API Refactoring — Prop Mutation via v-model

**Phase:** Frontend refactoring (Vue component decomposition)

**What goes wrong:** If a component receives a prop and binds it directly to `<input v-model="propName">`, Vue emits a prop mutation warning in development. In production (no warnings), the mutation silently fails to propagate: the component's displayed value updates (local vdom), but the parent never receives the change because no `update:propName` event was emitted.

**Specific risk in this codebase:** `AdminUsersTab`, `AdminQuotasTab`, and `AuditLogTab` currently own their filter/search state internally. When extracted into standalone views, these may receive initial state from route query params and accidentally bind those props directly to inputs.

**Prevention:** Any input the component controls must bind to a local `data()` copy initialized from the prop: `data() { return { localFilter: this.initialFilter } }`. Emit `update:initialFilter` on change. Never write `v-model="$props.X"`.

---

### Pitfall 11: Options API Refactoring — data() vs computed() in Decomposed Components

**Phase:** Frontend refactoring

**What goes wrong:** When extracting a large component, logic that was a `computed()` in the parent often gets copy-pasted into a child's `data()`. A `data()` value initializes once at mount — it does not react to prop changes. If the parent later passes a different prop value (e.g. the document list changes after a search), the child's stale `data()` copy never updates.

**Prevention:** For every value in a newly created component, ask: "If the parent passes a different value for this prop, should the child update?" Yes → `computed()`. No → `data()`. Initialized from prop but locally modified afterward → `data()` with a `watch` on the prop to reset when it changes externally.

---

### Pitfall 12: Keyboard Shortcuts — Conflict with Text Inputs and Browser Defaults

**Phase:** UX improvements (keyboard shortcuts)

**What goes wrong:** A global `keydown` listener on `window` fires regardless of focused element. Shortcuts like `/` (focus search), `n` (new folder), `Delete` (delete selected) are also typed characters. They must not fire when any `<input>`, `<textarea>`, `<select>`, or `[contenteditable]` element has focus. `Escape` conflicts with native `<dialog>` cancel events if `DocumentPreviewModal` uses a native dialog — both fire, causing a double-close bug.

**Prevention:**

```javascript
function onKeydown(e) {
  const tag = document.activeElement?.tagName
  const isEditing = ['INPUT', 'TEXTAREA', 'SELECT'].includes(tag)
    || document.activeElement?.isContentEditable
  if (isEditing) return

  if (e.key === '/' && !e.ctrlKey && !e.metaKey) {
    e.preventDefault()
    searchInputRef.value?.focus()
  }
}
// Options API: register in mounted(), remove in beforeUnmount()
// Omitting the removeEventListener is the most common bug in this pattern.
```

Never call `preventDefault()` on `F5`, `Backspace`, `Ctrl+F`, or `Cmd+F` — these are browser-reserved. Use a Pinia store to track which shortcut context is active (file manager, document view, modal) so modal shortcuts suppress file manager shortcuts rather than both firing.

---

## Minor Pitfalls

---

### Pitfall 13: Loading Skeletons — Count Mismatch Causes Layout Shift

**Phase:** UX improvements (loading skeletons)

**What goes wrong:** `StorageBrowser.vue` already has a `loading` prop (line 244) that shows "Loading…" text. Replacing this with skeleton rows requires a skeleton count. Hardcoding 5 rows when `per_page=20` causes a jump; using `per_page` as the skeleton count when only 3 items exist also jars. If skeleton rows are different heights from actual rows, the layout shifts when real content loads — this is a Core Web Vitals issue.

**Prevention:** Render skeletons at the previous page's actual item count (stored in the parent store). Default to 6-8 on first load. Skeleton rows must match the height of `StorageBrowser`'s actual grid rows exactly — use the same `px-4 py-2.5 grid-cols-[2rem_1fr_6rem_8rem_6rem]` structure with gray placeholder blocks.

---

### Pitfall 14: Tailwind Purging — Dynamic Class Names in Refactored Components

**Phase:** Visual redesign (Tailwind component system)

**What goes wrong:** Vite + Tailwind purges unused CSS at build time by scanning template strings for class names. Dynamic class names constructed at runtime (e.g. `'text-' + color + '-500'`) are invisible to the purger. In development all classes are present; in production the dynamic classes are stripped and topics/provider badges render without color.

**Specific risk:** `formatters.js` functions `providerColor`, `providerBg`, `providerLabel` return class strings. If those strings are not present as complete literals anywhere in the scanned source, they are purged.

**Prevention:** Add a `safelist` in `tailwind.config.js`:
```javascript
safelist: [
  { pattern: /bg-(blue|green|purple|orange|gray|indigo|red)-(50|100|500|600)/ },
  { pattern: /text-(blue|green|purple|orange|gray|indigo|red)-(500|600|700)/ },
]
```
Alternatively, use a lookup object of complete class strings rather than string concatenation — Tailwind can scan complete strings in object literals.

---

## Integration Pitfalls (Where Two Changes Interact Badly)

---

### Integration Pitfall A: Admin Rearchitecture + client.js Splitting — Wrong Order Causes Test Blindness

**Phases:** Admin panel rearchitecture + frontend refactoring

**What goes wrong:** If `client.js` is split into domain sub-files before the admin routes are moved to `/admin/*`, the new `api/admin.js` sub-file gets created. Components are then updated to point at the new route structure. But if the split happened first and components were simultaneously moved to import from `api/admin.js` directly (bypassing the barrel), the existing tests (which mock `client.js` imports) no longer cover the new import path. Tests pass, bugs hide.

**Prevention:** Split `client.js` using the re-export barrel pattern first. Verify all existing tests still pass against the barrel. Then rearchitect admin routes. The barrel acts as a compatibility shim throughout both phases. Only remove the barrel after all tests are updated to import from sub-files.

---

### Integration Pitfall B: Virtual Scrolling + Drag-and-Drop Cannot Be Added Simultaneously

**Phases:** Frontend performance + UX improvements

**What goes wrong:** Virtual scroll and HTML5 drag-and-drop between rows both depend on the same DOM elements. Virtual scroll keeps only visible rows in the DOM. A user dragging a file toward a folder that is currently scrolled out of the virtual window will never trigger `dragover` on that folder row — it is not in the DOM. `StorageBrowser.vue`'s existing drag-into-folder implementation (lines 88-90) relies on `@dragover.prevent` on folder rows. In a virtual list, off-screen folder rows do not receive this event.

**Prevention:** Decide the list interaction model before implementing either feature. If virtual scrolling is added, redesign drag-to-folder: the sidebar folder tree (always fully rendered, outside the virtual list) becomes the drop target. This is a UX design decision that must be made before either feature is implemented, not a detail to resolve mid-phase.

---

### Integration Pitfall C: Responsive Sidebar + Admin Guard Race on Mobile Page Load

**Phases:** Responsive layout + admin panel rearchitecture

**What goes wrong:** The current `beforeEach` guard correctly awaits `authStore.refresh()` before checking `authStore.user?.role` (router lines 82-88). This is correct. The risk emerges when adding nested admin child routes: a developer adding a component-level `created()` or `onMounted()` auth check in an admin page component, trying to guard against unauthorized access "just in case." If that component check does not await the refresh (because the token is memory-only and gone on mobile page reload), `authStore.user` is null at that point, the component check redirects the user to `/login`, and the correct guard never gets to run. The result is that admins are bounced to login on every mobile page reload.

**Prevention:** The single `beforeEach` guard is the only place that should call `refresh()` or check roles. Page components must not independently re-check auth. If a component needs the current user, it reads from `authStore.user` — which is guaranteed to be populated by the time the component mounts, because the guard has already awaited the refresh.