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.

// 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:

// 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:

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:

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.