---
phase: 10-ux-interaction
reviewed: 2026-06-16T12:00:00Z
depth: standard
files_reviewed: 34
files_reviewed_list:
  - frontend/src/App.vue
  - frontend/src/__tests__/keyboard.test.js
  - frontend/src/components/documents/DocumentCard.vue
  - frontend/src/components/documents/SearchBar.vue
  - frontend/src/components/folders/FolderRow.vue
  - frontend/src/components/layout/AppSidebar.vue
  - frontend/src/components/layout/OsDragOverlay.vue
  - frontend/src/components/layout/__tests__/AppSidebar.empty.test.js
  - frontend/src/components/layout/__tests__/OsDragOverlay.test.js
  - frontend/src/components/storage/StorageBrowser.vue
  - frontend/src/components/storage/__tests__/StorageBrowser.dragmove.test.js
  - frontend/src/components/storage/__tests__/StorageBrowser.skeleton.test.js
  - frontend/src/components/ui/AppIcon.vue
  - frontend/src/components/ui/BreadcrumbBar.vue
  - frontend/src/components/ui/EmptyState.vue
  - frontend/src/components/ui/ToastContainer.vue
  - frontend/src/components/ui/__tests__/AppIcon.test.js
  - frontend/src/components/ui/__tests__/BreadcrumbBar.test.js
  - frontend/src/components/ui/__tests__/EmptyState.test.js
  - frontend/src/components/ui/__tests__/ToastContainer.test.js
  - frontend/src/components/ui/__tests__/dropdown.test.js
  - frontend/src/components/upload/DropZone.vue
  - frontend/src/stores/__tests__/toast.test.js
  - frontend/src/stores/toast.js
  - frontend/src/views/FileManagerView.vue
  - frontend/src/views/admin/AdminAuditView.vue
  - frontend/src/views/admin/AdminUsersView.vue
  - frontend/src/views/admin/__tests__/AdminAuditView.skeleton.test.js
  - frontend/src/views/admin/__tests__/AdminUsersView.skeleton.test.js
  - frontend/src/views/SharedView.vue
  - frontend/src/views/CloudFolderView.vue
  - frontend/src/views/CloudStorageView.vue
  - frontend/src/components/sharing/ShareModal.vue
  - frontend/src/components/folders/FolderDeleteModal.vue
findings:
  critical: 2
  warning: 6
  info: 4
  total: 12
status: issues_found
---

# Phase 10: Code Review Report

**Reviewed:** 2026-06-16T12:00:00Z
**Depth:** standard
**Files Reviewed:** 34
**Status:** issues_found

## Summary

Phase 10 delivers skeleton loading states, keyboard shortcuts (`/`, `U`, `N`, `Escape`), OS drag-and-drop overlay, drag-to-move documents, Teleport-based dropdown positioning, a toast notification system, and new shared UI components (`EmptyState`, `BreadcrumbBar`, `ToastContainer`, `AppIcon`). The implementation is well-structured: shared formatters are correctly imported in all reviewed views, event listeners are properly cleaned up in `onUnmounted`, and the Teleport+`getBoundingClientRect` pattern is consistent across components.

Two blockers were found: a direct Vue prop mutation in `DocumentCard.vue` (raises a runtime warning and can silently fail to update the UI), and a keyboard shortcut bleed-through where pressing Escape while a modal is open fires both the modal's close handler and `clearSearch` simultaneously. Six warnings cover the `closest('.relative')` outside-click detector pattern (present in three files) that prevents pickers from closing correctly, a non-compliant Fisher-Yates shuffle in the admin password generator, an unused import, a custom local date formatter that violates the shared-module rule, an upload count metric that can be unreliable, and a leak of stale DOM references in `StorageBrowser`'s folder picker map.

---

## Critical Issues

### CR-01: Direct Vue prop mutation in DocumentCard raises runtime warning and can fail silently

**File:** `frontend/src/components/documents/DocumentCard.vue:86`

**Issue:** The `@unshared` event handler directly assigns `doc.is_shared = false` where `doc` is a component prop declared via `defineProps`. Vue 3 wraps props in a `readonly` proxy; writing to `doc.is_shared` triggers `[Vue warn]: Set operation on key "is_shared" failed: target is readonly` in development and silently does nothing in production builds that enforce the readonly constraint, leaving the card's "Shared" pill stale until the next full data fetch.

```html
<!-- current: mutates readonly prop -->
@unshared="doc.is_shared = false"
```

**Fix:** Emit the event upward and let the parent manage the mutation. `DocumentCard` already has an `emit` defined:

```js
// DocumentCard.vue — add 'unshared' to defineEmits
const emit = defineEmits(['reclassified', 'unshared'])
```

```html
<!-- DocumentCard.vue template — delegate to parent -->
<ShareModal
  v-if="showShareModal"
  :doc="doc"
  @close="showShareModal = false"
  @unshared="$emit('unshared', doc.id)"
/>
```

The parent `FileManagerView` already handles `@unshared` correctly by looking the document up in the Pinia store and updating store state there.

---

### CR-02: Escape key fires both modal-close and clearSearch simultaneously

**File:** `frontend/src/App.vue:38-40` / `frontend/src/components/sharing/ShareModal.vue:143` / `frontend/src/components/folders/FolderDeleteModal.vue:73`

**Issue:** Both `ShareModal` and `FolderDeleteModal` attach a `window` `keydown` listener that closes the modal on Escape. `App.vue` also attaches a `document` `keydown` listener calling `clearSearch` on Escape. The guard at `App.vue:31-32` only returns early when a form **input element** is focused. When a modal is open and no field inside it is active (e.g., immediately after `FolderDeleteModal` opens), the guard does not block, so `clearSearch` fires at the same time as the modal close — erasing any active search query unintentionally.

As a secondary issue, the `U` and `N` keyboard shortcuts also fire when `FolderDeleteModal` is visible (no input focused), meaning a user could accidentally trigger an upload picker or new-folder inline input while staring at a destructive confirmation dialog.

**Fix:** Check for an open `role="dialog"` element in `App.vue`'s handler before processing any shortcut:

```js
// App.vue
function onKeydown(e) {
  const tag = document.activeElement?.tagName
  if (['INPUT', 'TEXTAREA', 'SELECT'].includes(tag) || document.activeElement?.isContentEditable) return
  // Block all shortcuts when any modal is open
  if (document.querySelector('[role="dialog"]')) return

  if (e.key === '/' && !e.ctrlKey && !e.metaKey) {
    e.preventDefault()
    routeViewRef.value?.focusSearch?.()
  }
  if (e.key === 'Escape') { routeViewRef.value?.clearSearch?.() }
  if (e.key === 'u' || e.key === 'U') { routeViewRef.value?.triggerUpload?.() }
  if (e.key === 'n' || e.key === 'N') { routeViewRef.value?.startNewFolder?.() }
}
```

The `document.querySelector('[role="dialog"]')` check leverages the existing `role="dialog"` attributes already present on both modal panels.

---

## Warnings

### WR-01: Outside-click detector uses `.closest('.relative')` — picker fails to close when clicking another card

**File:** `frontend/src/components/documents/DocumentCard.vue:180-181` (same pattern in `frontend/src/components/folders/FolderRow.vue:179-180` and `frontend/src/components/storage/StorageBrowser.vue:431-432`)

**Issue:** The `closeFolderPicker` / `handleOutsideClick` / `onOutsideClick` guards check `e.target.closest('.relative')` to decide whether a click was inside the trigger wrapper. The `DocumentCard` root element itself carries `class="group … relative"` (line 3). This means any click anywhere on any `DocumentCard` in the list matches `.closest('.relative')`, so an open folder-picker in one card will never close when the user clicks a sibling card. In practice, repeated clicks on different move buttons leave multiple visual states stale.

`FolderRow`'s `.relative` wrapper (line 34) is narrower (only the three-dot button area), so that bug is less visible but has the same root cause. `StorageBrowser` has one `.relative` per Move button in file rows, which is also narrow but still fragile — any nested `div.relative` elsewhere breaks the assumption.

**Fix:** Use a unique `data-*` attribute on the trigger wrapper and check containment against it:

```html
<!-- DocumentCard.vue: -->
<div data-folder-picker-trigger>
  <button ref="pickerTriggerEl" @click.stop="toggleFolderPicker" ...>
```

```js
// DocumentCard.vue:
function closeFolderPicker(e) {
  if (
    !e.target.closest('[data-test="folder-picker"]') &&
    !e.target.closest('[data-folder-picker-trigger]')
  ) {
    showFolderPicker.value = false
  }
}
```

Apply the same fix to `FolderRow.vue` (`data-folder-menu-trigger`) and to `StorageBrowser.vue` using the already-available `pickerTriggerMap` reference:

```js
// StorageBrowser.vue onOutsideClick:
function onOutsideClick(e) {
  if (!folderPickerFileId.value) return
  const trig = pickerTriggerMap.get(folderPickerFileId.value)
  if (
    !e.target.closest('[data-test="folder-picker"]') &&
    !(trig && trig.contains(e.target))
  ) {
    folderPickerFileId.value = null
  }
}
```

---

### WR-02: Fisher-Yates shuffle in password generator uses only 4 bytes for 15 swap operations

**File:** `frontend/src/views/admin/AdminUsersView.vue:311-316`

**Issue:** The shuffle loop runs from `i = 15` down to `i = 1` (15 iterations). Each iteration computes `j = posArr[4 + (i % 4)] % (i + 1)`, cycling through only 4 distinct bytes (`posArr[4]`–`posArr[7]`). Because the same random bytes are reused at multiple positions, the resulting permutation space is dramatically smaller than 16! — the same 4 bytes produce correlated swap indices, concentrating the required chars (placed at positions 0–3 before the shuffle) near the front of the output for many seeds. The comment on line 294 correctly notes no modulo bias for character selection; the shuffle itself is under-seeded.

**Fix:** Generate one fresh random value per swap:

```js
// Replace the shuffle block (lines 308-315):
const swapArr = new Uint32Array(chars.length)
crypto.getRandomValues(swapArr)
for (let i = chars.length - 1; i > 0; i--) {
  const j = swapArr[i] % (i + 1)
  ;[chars[i], chars[j]] = [chars[j], chars[i]]
}
```

A single `getRandomValues` call for a `Uint32Array` of `chars.length` elements provides one independent 32-bit value per swap — sufficient entropy and still a single API call.

---

### WR-03: `onUnmounted` imported but never called in FileManagerView

**File:** `frontend/src/views/FileManagerView.vue:47`

**Issue:** `onUnmounted` is destructured in the Vue import but the component never calls it. The view sets up no event listeners that require teardown (keyboard shortcuts live in `App.vue`). Per CLAUDE.md: "files with no active route and no active import are deleted immediately — not commented out, not kept 'just in case'"; the same applies to unused identifiers within a file.

**Fix:** Remove `onUnmounted` from the import line:

```js
import { ref, reactive, computed, watch, onMounted } from 'vue'
```

---

### WR-04: AdminAuditView defines its own `formatTimestamp` instead of importing from shared formatters

**File:** `frontend/src/views/admin/AdminAuditView.vue:335-342`

**Issue:** `AdminAuditView` defines a local `formatTimestamp` function that converts an ISO string to a display date. CLAUDE.md states: "No component may define its own `formatDate` or `formatSize`. Always import from `utils/formatters.js`." While the function is named differently from `formatDate`, it performs date formatting that belongs in `formatters.js`.

**Fix:** Move the function to `utils/formatters.js` and import it:

```js
// utils/formatters.js — add:
export function formatTimestamp(iso) {
  if (!iso) return '—'
  try {
    return new Date(iso).toISOString().replace('T', ' ').slice(0, 19)
  } catch {
    return iso
  }
}
```

```js
// AdminAuditView.vue — replace local function with:
import { formatTimestamp } from '../../utils/formatters.js'
// Delete the local formatTimestamp definition
```

---

### WR-05: Upload succeeded-count is unreliable when multiple batches overlap

**File:** `frontend/src/views/FileManagerView.vue:110-120`

**Issue:** Each call to `onFilesSelected` prepends new items to `uploadQueue` with `unshift`, then measures success with `uploadQueue.value.slice(0, files.length)`. Because `uploadQueue` is never trimmed between calls, a second upload batch started while items from the first batch are still settling changes what `slice(0, files.length)` sees: if a previous batch's items remain at the front before the new items are prepended, the count will include wrong items. Additionally, the queue grows indefinitely during the session, holding references to completed reactive upload-item objects for the entire session.

**Fix:** Prune settled items at the start of each batch before adding new ones:

```js
async function onFilesSelected({ files, autoClassify }) {
  // Remove settled items from prior batches before prepending new ones
  uploadQueue.value = uploadQueue.value.filter(i => !i.done && !i.error && !i.quotaError)
  // ... rest of function unchanged
}
```

---

### WR-06: `StorageBrowser` `pickerTriggerMap` retains stale DOM element references

**File:** `frontend/src/components/storage/StorageBrowser.vue:391`

**Issue:** `pickerTriggerMap` is a plain `Map` that stores references from `fileId` to a DOM button element, populated in `openFolderPicker`. The map is never cleared when files are removed from the `:files` prop or when the component unmounts. If the user deletes a file while its picker is open (or navigates away mid-interaction), the stale entry means the `onWindowScroll` repositioner will call `getBoundingClientRect()` on a detached element, which silently returns a zeroed rect and places the picker in the top-left corner of the screen.

**Fix:** Clear the map in `onUnmounted` and prune entries when a picker is closed or a move is committed:

```js
// In onUnmounted:
onUnmounted(() => {
  document.removeEventListener('click', onOutsideClick)
  window.removeEventListener('scroll', onWindowScroll, true)
  window.removeEventListener('resize', onWindowScroll)
  pickerTriggerMap.clear()  // add this line
})

// In openFolderPicker, when toggling off:
if (folderPickerFileId.value === fileId) {
  folderPickerFileId.value = null
  pickerTriggerMap.delete(fileId)  // add this line
  return
}
```

---

## Info

### IN-01: `console.error` left in `DocumentCard` production paths without user-visible feedback

**File:** `frontend/src/components/documents/DocumentCard.vue:203, 217`

**Issue:** The `moveToFolder` and `reanalyze` catch blocks call `console.error(...)` without showing anything to the user. Unlike `AppIcon`'s `console.warn` (guarded by `import.meta.env.DEV`), these fire in production. A failed move or re-analysis silently disappears from the user's perspective.

**Fix:** Surface errors through the toast store instead of (or in addition to) `console.error`:

```js
import { useToastStore } from '../../stores/toast.js'
const toast = useToastStore()

async function moveToFolder(folderId) {
  showFolderPicker.value = false
  try {
    await moveDocument(props.doc.id, folderId)
  } catch (e) {
    toast.show('Move failed: ' + (e.message || 'unknown error'), 'error')
  }
}
```

---

### IN-02: `FolderDeleteModal` supports both emit and callback-prop patterns simultaneously

**File:** `frontend/src/components/folders/FolderDeleteModal.vue:60-86`

**Issue:** `FolderDeleteModal` defines `onConfirm` and `onCancel` callback props **and** emits `confirm` / `cancel` events. `handleConfirm` calls both `emit('confirm')` and `props.onConfirm()` (when set). In the current codebase only the emit-based usage is active (props are always `null`). The dual interface is dead API surface that will confuse future consumers who might pass both, triggering a double-action.

**Fix:** Remove the `onConfirm` and `onCancel` props entirely. Emit-based communication is the canonical Vue pattern for child-to-parent notification.

---

### IN-03: Search bar hidden at root folder level — `'/'` shortcut silently does nothing there

**File:** `frontend/src/components/storage/StorageBrowser.vue:302`

**Issue:** `showSearch` is `props.mode === 'local' && props.breadcrumb.length > 0`, meaning the search bar only appears when inside a subfolder. The global `'/'` keyboard shortcut calls `focusSearch()` which resolves to a no-op at the root level because `searchBarRef` is not mounted. The user pressing `'/'` on the home screen gets no response.

**Fix:** Either extend search to the root level (straightforward change to `showSearch`), or have the keyboard shortcut give feedback when search is unavailable:

```js
// App.vue or FileManagerView:
if (e.key === '/') {
  if (!browserRef.value?.searchBarRef?.value) {
    // search not available here — optionally show a brief toast
    return
  }
  e.preventDefault()
  routeViewRef.value?.focusSearch?.()
}
```

---

### IN-04: `AppIcon.vue` and three other new UI components use Options API while all new views use `<script setup>`

**File:** `frontend/src/components/ui/AppIcon.vue`, `BreadcrumbBar.vue`, `EmptyState.vue`, `ToastContainer.vue`

**Issue:** All four components are written with Options API (`export default { ... }`), while every view and smart component added in Phase 10 uses `<script setup>`. The project stack description says "Vue 3 (Options API)", but the preponderance of Phase 10 work uses Composition API. The inconsistency creates two code styles in the same component layer. Additionally, `OsDragOverlay.vue` uses Options API, and the `OsDragOverlay.test.js` tests assert on `w.vm.showOverlay` and `w.vm.dragDepth` (internal state), which would break if the component were migrated to `<script setup>` without `defineExpose`.

**Fix:** No immediate action required — this is style-level. If Options API is the project standard, document it in CLAUDE.md and add a note that `<script setup>` components must `defineExpose` any properties asserted by tests. If migrating all new components to `<script setup>` is desired, do it as a separate dedicated commit.

---

_Reviewed: 2026-06-16T12:00:00Z_
_Reviewer: Claude (gsd-code-reviewer)_
_Depth: standard_