docs(10): add code review report
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
curo1305
co-authored by
Claude Sonnet 4.6
Claude Sonnet 4.6
parent
a6e130b49e
commit
37f49bc6ea
@@ -1,15 +1,11 @@
|
||||
---
|
||||
phase: 10-ux-interaction
|
||||
reviewed: 2026-06-16T00:00:00Z
|
||||
reviewed: 2026-06-16T12:00:00Z
|
||||
depth: standard
|
||||
files_reviewed: 45
|
||||
files_reviewed: 34
|
||||
files_reviewed_list:
|
||||
- frontend/src/App.vue
|
||||
- frontend/src/__tests__/keyboard.test.js
|
||||
- frontend/src/components/auth/TotpEnrollment.vue
|
||||
- frontend/src/components/cloud/CloudCredentialModal.vue
|
||||
- frontend/src/components/cloud/CloudFolderTreeItem.vue
|
||||
- frontend/src/components/cloud/CloudProviderTreeItem.vue
|
||||
- frontend/src/components/documents/DocumentCard.vue
|
||||
- frontend/src/components/documents/SearchBar.vue
|
||||
- frontend/src/components/folders/FolderRow.vue
|
||||
@@ -17,15 +13,12 @@ files_reviewed_list:
|
||||
- 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/settings/SettingsAccountTab.vue
|
||||
- frontend/src/components/settings/SettingsCloudTab.vue
|
||||
- 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/SearchableModelSelect.vue
|
||||
- frontend/src/components/ui/ToastContainer.vue
|
||||
- frontend/src/components/ui/__tests__/AppIcon.test.js
|
||||
- frontend/src/components/ui/__tests__/BreadcrumbBar.test.js
|
||||
@@ -35,314 +28,139 @@ files_reviewed_list:
|
||||
- frontend/src/components/upload/DropZone.vue
|
||||
- frontend/src/stores/__tests__/toast.test.js
|
||||
- frontend/src/stores/toast.js
|
||||
- frontend/src/views/AccountView.vue
|
||||
- frontend/src/views/CloudFolderView.vue
|
||||
- frontend/src/views/CloudStorageView.vue
|
||||
- frontend/src/views/DocumentView.vue
|
||||
- frontend/src/views/FileManagerView.vue
|
||||
- frontend/src/views/SettingsView.vue
|
||||
- frontend/src/views/SharedView.vue
|
||||
- frontend/src/views/__tests__/FileManagerView.test.js
|
||||
- frontend/src/views/admin/AdminAiView.vue
|
||||
- frontend/src/views/admin/AdminAuditView.vue
|
||||
- frontend/src/views/admin/AdminOverviewView.vue
|
||||
- frontend/src/views/admin/AdminQuotasView.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: 4
|
||||
warning: 9
|
||||
critical: 2
|
||||
warning: 6
|
||||
info: 4
|
||||
total: 17
|
||||
total: 12
|
||||
status: issues_found
|
||||
---
|
||||
|
||||
# Phase 10: Code Review Report
|
||||
|
||||
**Reviewed:** 2026-06-16T00:00:00Z
|
||||
**Reviewed:** 2026-06-16T12:00:00Z
|
||||
**Depth:** standard
|
||||
**Files Reviewed:** 45
|
||||
**Files Reviewed:** 34
|
||||
**Status:** issues_found
|
||||
|
||||
## Summary
|
||||
|
||||
This phase delivered 10+ UX interaction plans covering keyboard shortcuts, skeleton loading, drag-and-drop, OS-level file drag overlay, toast notifications, Teleport-based dropdowns, and breadcrumb/empty-state polish. The overall architecture is sound and the CLAUDE.md shared-module rules are consistently followed. However, several correctness bugs, one security gap, and multiple quality issues were found.
|
||||
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.
|
||||
|
||||
The most critical problems are: (1) a duplicate, diverged implementation of the entire account settings screen split between `AccountView.vue` and `SettingsAccountTab.vue`; (2) `closeFolderPicker` in `DocumentCard.vue` closing on any click inside any `.relative` element across the entire page — not only its own trigger; (3) global `window.addEventListener` calls in `SearchableModelSelect.vue` are registered outside `onMounted`, so they run during SSR or module evaluation in test environments and are never cleaned up when multiple instances mount and unmount; and (4) the `generateRandomPassword` Fisher-Yates shuffle in `AdminUsersView.vue` reuses only 4 bytes for shuffling a 16-character array, which produces a weak shuffle distribution.
|
||||
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: Duplicate account settings implementation — diverged behaviour
|
||||
### CR-01: Direct Vue prop mutation in DocumentCard raises runtime warning and can fail silently
|
||||
|
||||
**File:** `frontend/src/views/AccountView.vue:1` and `frontend/src/components/settings/SettingsAccountTab.vue:1`
|
||||
**File:** `frontend/src/components/documents/DocumentCard.vue:86`
|
||||
|
||||
**Issue:** Both files implement the identical Account settings UI (account info, TOTP, change password, sign-out all). They have already diverged: `SettingsAccountTab` calls `useToastStore` and shows toasts when sessions are revoked after TOTP disable or password change (lines 154–156, 239–241); `AccountView` does neither. The CLAUDE.md rule "things that look the same are the same in code" and "no dead code — files with no active route and no active import are deleted immediately" is violated. Whichever is currently routed, the other leaks functionality and will continue to diverge.
|
||||
**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.
|
||||
|
||||
**Fix:** Determine which is the canonical implementation (should be `SettingsAccountTab` because it is the smart component under `SettingsView`). If `AccountView.vue` has an active route pointing directly to it, that route must be removed and the component deleted. If `AccountView.vue` is already orphaned, delete it immediately. Do not keep both alive.
|
||||
```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: `closeFolderPicker` in `DocumentCard.vue` closes on any `.relative` element — not just its own trigger
|
||||
### CR-02: Escape key fires both modal-close and clearSearch simultaneously
|
||||
|
||||
**File:** `frontend/src/components/documents/DocumentCard.vue:179-184`
|
||||
**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:
|
||||
|
||||
**Issue:**
|
||||
```js
|
||||
function closeFolderPicker(e) {
|
||||
if (
|
||||
!e.target.closest('[data-test="folder-picker"]') &&
|
||||
!e.target.closest('.relative') // ← matches ANY .relative on the page
|
||||
) {
|
||||
showFolderPicker.value = false
|
||||
// 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 guard `e.target.closest('.relative')` matches any ancestor `div.relative` on the page, not only the move-button container inside this card. This means clicking any other component that wraps its content in a `div class="relative"` — including `SearchableModelSelect`, `StorageBrowser`'s own picker trigger, or any other card — will prevent this picker from closing. In practice, clicking a sibling `DocumentCard`'s move button will open both pickers simultaneously and neither will close the other. The same pattern also exists in `FolderRow.vue:178-183` with identical semantics.
|
||||
|
||||
**Fix:** Use a `ref` on the trigger element and check it directly:
|
||||
```js
|
||||
// Store a ref to the wrapper div:
|
||||
// <div class="relative" ref="moveWrapperRef">
|
||||
const moveWrapperRef = ref(null)
|
||||
|
||||
function closeFolderPicker(e) {
|
||||
if (
|
||||
!e.target.closest('[data-test="folder-picker"]') &&
|
||||
!(moveWrapperRef.value && moveWrapperRef.value.contains(e.target))
|
||||
) {
|
||||
showFolderPicker.value = false
|
||||
}
|
||||
}
|
||||
```
|
||||
Apply the same fix to `FolderRow.vue`'s `handleOutsideClick`.
|
||||
|
||||
---
|
||||
|
||||
### CR-03: Global `window.addEventListener` called at module/setup scope in `SearchableModelSelect.vue`
|
||||
|
||||
**File:** `frontend/src/components/ui/SearchableModelSelect.vue:268-273`
|
||||
|
||||
**Issue:**
|
||||
```js
|
||||
window.addEventListener('scroll', onScroll, true)
|
||||
window.addEventListener('resize', onScroll)
|
||||
onUnmounted(() => {
|
||||
window.removeEventListener('scroll', onScroll, true)
|
||||
window.removeEventListener('resize', onScroll)
|
||||
})
|
||||
```
|
||||
The `addEventListener` calls are at the top level of `<script setup>`, executed immediately when the component is set up — not inside `onMounted`. In a standard browser SPA this works because `window` exists, but:
|
||||
1. If the component is unmounted before it ever mounts (e.g. destroyed in a v-if branch during the same tick), `onUnmounted` fires but the listeners were already registered against the specific `onScroll` closure; since each component instance creates a new closure, multiple mounts accumulate multiple event listeners even after unmounting — resulting in stale, never-removed listeners for every instance beyond the first.
|
||||
2. In SSR environments `window` is undefined and this crashes at import time.
|
||||
3. In the Vitest test environment (jsdom), these listeners attach immediately on import and remain attached across all tests.
|
||||
|
||||
**Fix:** Move both `addEventListener` calls inside `onMounted`:
|
||||
```js
|
||||
onMounted(() => {
|
||||
window.addEventListener('scroll', onScroll, true)
|
||||
window.addEventListener('resize', onScroll)
|
||||
})
|
||||
onUnmounted(() => {
|
||||
window.removeEventListener('scroll', onScroll, true)
|
||||
window.removeEventListener('resize', onScroll)
|
||||
})
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### CR-04: Weak Fisher-Yates shuffle in `AdminUsersView.vue` — only 4 bytes reused for 12 positions
|
||||
|
||||
**File:** `frontend/src/views/admin/AdminUsersView.vue:303-316`
|
||||
|
||||
**Issue:**
|
||||
```js
|
||||
const posArr = new Uint8Array(8)
|
||||
crypto.getRandomValues(posArr)
|
||||
// ...
|
||||
for (let i = chars.length - 1; i > 0; i--) {
|
||||
const j = posArr[4 + (i % 4)] % (i + 1) // ← reuses bytes 4-7 cyclically
|
||||
;[chars[i], chars[j]] = [chars[j], chars[i]]
|
||||
}
|
||||
```
|
||||
`chars` has 16 entries. The shuffle loop runs from `i = 15` down to `i = 1` — 15 iterations. It only has 4 bytes (`posArr[4]` through `posArr[7]`) and cycles through them via `i % 4`. Each byte is reused for 4 different swap positions. Because a reused byte produces a correlated index at each position, the resulting permutation space is drastically smaller than 16! (the expected 2^128 for a 16-char password). The comment "256 % 64 === 0, no modulo bias" applies only to character selection, not to the shuffle.
|
||||
|
||||
This is a temporary password presented to an admin; the reduced entropy may allow an attacker who compromises an account creation log (or intercepts the clipboard) to brute-force the temporay credential faster.
|
||||
|
||||
**Fix:** Generate a fresh random byte for every swap position:
|
||||
```js
|
||||
async function generateRandomPassword() {
|
||||
const upper = 'ABCDEFGHJKLMNPQRSTUVWXYZ'
|
||||
const lower = 'abcdefghijkmnpqrstuvwxyz'
|
||||
const digits = '23456789'
|
||||
const special = '!@#$%^&*'
|
||||
const charset = upper + lower + digits + special
|
||||
|
||||
const charArr = new Uint8Array(16)
|
||||
crypto.getRandomValues(charArr)
|
||||
const chars = Array.from(charArr, byte => charset[byte % charset.length])
|
||||
|
||||
// Ensure required character classes
|
||||
const reqArr = new Uint8Array(4)
|
||||
crypto.getRandomValues(reqArr)
|
||||
chars[0] = upper[reqArr[0] % upper.length]
|
||||
chars[1] = lower[reqArr[1] % lower.length]
|
||||
chars[2] = digits[reqArr[2] % digits.length]
|
||||
chars[3] = special[reqArr[3] % special.length]
|
||||
|
||||
// Full shuffle — one fresh byte per swap
|
||||
const swapArr = new Uint8Array(chars.length)
|
||||
for (let i = chars.length - 1; i > 0; i--) {
|
||||
crypto.getRandomValues(swapArr) // refresh every position
|
||||
const j = swapArr[i] % (i + 1)
|
||||
;[chars[i], chars[j]] = [chars[j], chars[i]]
|
||||
}
|
||||
return chars.join('')
|
||||
}
|
||||
```
|
||||
(Or generate a full 16-byte array for swap indices once and accept the minor modulo bias for the shuffle — the password length and character variety already provide sufficient entropy.)
|
||||
The `document.querySelector('[role="dialog"]')` check leverages the existing `role="dialog"` attributes already present on both modal panels.
|
||||
|
||||
---
|
||||
|
||||
## Warnings
|
||||
|
||||
### WR-01: `onDropDocOnFolder` in `StorageBrowser.vue` does not guard against dropping a folder onto itself
|
||||
### WR-01: Outside-click detector uses `.closest('.relative')` — picker fails to close when clicking another card
|
||||
|
||||
**File:** `frontend/src/components/storage/StorageBrowser.vue:377-384`
|
||||
**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 drop handler emits `file-move` without checking whether `draggingFile.value.id === folderId`. While the backend should reject this, the frontend optimistically emits the event and the view's `doMove` calls `moveToFolder` anyway, potentially causing a backend error that surfaces to the user with a confusing toast.
|
||||
**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.
|
||||
|
||||
**Fix:** Add an early return in `onDropDocOnFolder`:
|
||||
```js
|
||||
async function onDropDocOnFolder(folderId) {
|
||||
if (!draggingFile.value) return
|
||||
if (draggingFile.value.id === folderId) { // ← guard
|
||||
dragOverFolderId.value = null
|
||||
return
|
||||
}
|
||||
// ...
|
||||
}
|
||||
`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" ...>
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### WR-02: `confirmRemoveOnly` in `DocumentView.vue` silently swallows errors
|
||||
|
||||
**File:** `frontend/src/views/DocumentView.vue:281-289`
|
||||
|
||||
**Issue:**
|
||||
```js
|
||||
async function confirmRemoveOnly() {
|
||||
try {
|
||||
await api.deleteDocumentRemoveOnly(doc.value.id)
|
||||
showCloudDeleteWarning.value = false
|
||||
router.push('/')
|
||||
} catch (e) {
|
||||
// error shown inline if needed — modal stays open
|
||||
}
|
||||
}
|
||||
```
|
||||
The catch block is empty. If `deleteDocumentRemoveOnly` fails (e.g. network error, 403), the user sees nothing — the modal just stays open with no indication of what happened. The comment "error shown inline if needed" is aspirational; nothing is actually shown.
|
||||
|
||||
**Fix:**
|
||||
```js
|
||||
} catch (e) {
|
||||
// Show error inside the modal
|
||||
cloudDeleteError.value = e.message || 'Failed to remove document. Please try again.'
|
||||
}
|
||||
```
|
||||
Add a `<p v-if="cloudDeleteError" ...>` inside the modal template.
|
||||
|
||||
---
|
||||
|
||||
### WR-03: `handleFolderRename` in `FileManagerView.vue` silently swallows rename errors
|
||||
|
||||
**File:** `frontend/src/views/FileManagerView.vue:141-143`
|
||||
|
||||
**Issue:**
|
||||
```js
|
||||
async function handleFolderRename({ id, name }) {
|
||||
if (!name) return
|
||||
try { await foldersStore.renameFolder(id, name) } catch {}
|
||||
}
|
||||
```
|
||||
Rename failures are completely invisible to the user. If the server rejects the rename (duplicate name, permission error), the UI silently does nothing and the old name may still appear, causing confusion.
|
||||
|
||||
**Fix:** Show a toast on error:
|
||||
```js
|
||||
async function handleFolderRename({ id, name }) {
|
||||
if (!name) return
|
||||
try {
|
||||
await foldersStore.renameFolder(id, name)
|
||||
} catch (e) {
|
||||
useToastStore().show('Rename failed: ' + (e.message || 'unknown error'), 'error')
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### WR-04: `confirmDeleteFolder` in `FileManagerView.vue` swallows delete errors silently
|
||||
|
||||
**File:** `frontend/src/views/FileManagerView.vue:148-152`
|
||||
|
||||
**Issue:**
|
||||
```js
|
||||
async function confirmDeleteFolder() {
|
||||
if (!folderToDelete.value) return
|
||||
try { await foldersStore.deleteFolder(folderToDelete.value.id) }
|
||||
finally { folderToDelete.value = null }
|
||||
}
|
||||
```
|
||||
If `deleteFolder` throws (e.g. folder not empty, server error), the error is discarded and the user has no feedback. The modal closes as if nothing happened.
|
||||
|
||||
**Fix:**
|
||||
```js
|
||||
async function confirmDeleteFolder() {
|
||||
if (!folderToDelete.value) return
|
||||
try {
|
||||
await foldersStore.deleteFolder(folderToDelete.value.id)
|
||||
} catch (e) {
|
||||
useToastStore().show('Delete failed: ' + (e.message || 'unknown error'), 'error')
|
||||
} finally {
|
||||
folderToDelete.value = null
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### WR-05: `OsDragOverlay` mixes Options API with a `<script>` block while all other new components use `<script setup>`
|
||||
|
||||
**File:** `frontend/src/components/layout/OsDragOverlay.vue:18-65`
|
||||
|
||||
**Issue:** The component is implemented with the Options API (`export default { ... }`), diverging from every other component added in this phase which uses `<script setup>`. This is not a bug, but it creates inconsistency, and the Options API form exposes `data()` properties directly on `vm` — which the tests then rely on (`w.vm.showOverlay`, `w.vm.dragDepth`). If this component is ever migrated to `<script setup>`, the tests will break because `defineExpose` would be needed. The test reliance on internal `vm` data is itself a quality issue.
|
||||
|
||||
**Fix:** Rewrite as `<script setup>` with `defineExpose({ showOverlay, dragDepth })` for testability, or document the intentional Options API choice. The tests should be updated to test observable behaviour (DOM visibility) instead of internal state.
|
||||
|
||||
---
|
||||
|
||||
### WR-06: `StorageBrowser.vue` — `onOutsideClick` folder picker closes on any `.relative` element
|
||||
|
||||
**File:** `frontend/src/components/storage/StorageBrowser.vue:429-436`
|
||||
|
||||
**Issue:** Identical to CR-02 — the `onOutsideClick` guard uses `e.target.closest('.relative')` which matches any `div.relative` on the page:
|
||||
```js
|
||||
function onOutsideClick(e) {
|
||||
// DocumentCard.vue:
|
||||
function closeFolderPicker(e) {
|
||||
if (
|
||||
!e.target.closest('[data-test="folder-picker"]') &&
|
||||
!e.target.closest('.relative') // ← too broad
|
||||
!e.target.closest('[data-folder-picker-trigger]')
|
||||
) {
|
||||
folderPickerFileId.value = null
|
||||
showFolderPicker.value = false
|
||||
}
|
||||
}
|
||||
```
|
||||
Because `StorageBrowser` renders multiple file rows each with their own `div.relative` move-button wrapper, clicking one file's move button while another file's picker is open will not close the open picker — both will appear open simultaneously.
|
||||
|
||||
**Fix:** Store the trigger element reference per `fileId` (already done via `pickerTriggerMap`) and check containment against it:
|
||||
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)
|
||||
@@ -357,144 +175,183 @@ function onOutsideClick(e) {
|
||||
|
||||
---
|
||||
|
||||
### WR-07: `SearchableModelSelect.vue` — `handleBlur` 150 ms timeout may close dropdown before `mousedown` fires on a list item
|
||||
### WR-02: Fisher-Yates shuffle in password generator uses only 4 bytes for 15 swap operations
|
||||
|
||||
**File:** `frontend/src/components/ui/SearchableModelSelect.vue:214-216`
|
||||
**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:
|
||||
|
||||
**Issue:**
|
||||
```js
|
||||
function handleBlur() {
|
||||
setTimeout(close, 150)
|
||||
// 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]]
|
||||
}
|
||||
```
|
||||
List items use `@mousedown.prevent` to block the blur, but the `<li>` for "manual entry" at the bottom uses `@mousedown.prevent="selectManual"`. If a user clicks that item slower than 150 ms after moving focus, the `handleBlur` timeout races with the `mousedown`. On some browsers under CPU load, 150 ms is insufficient. A common robust value is 200–300 ms; alternatively, track `isMousedownOnList` via `@mousedown` on the `<ul>` to cancel the close:
|
||||
|
||||
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
|
||||
// On <ul>: @mousedown="listMousedown = true" @mouseup="listMousedown = false"
|
||||
function handleBlur() {
|
||||
setTimeout(() => { if (!listMousedown) close() }, 0)
|
||||
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-08: `CloudFolderView.vue` `@file-open` handler is a no-op
|
||||
### WR-06: `StorageBrowser` `pickerTriggerMap` retains stale DOM element references
|
||||
|
||||
**File:** `frontend/src/views/CloudFolderView.vue:14`
|
||||
**File:** `frontend/src/components/storage/StorageBrowser.vue:391`
|
||||
|
||||
**Issue:**
|
||||
```html
|
||||
@file-open="file => {}"
|
||||
```
|
||||
Clicking a file row in cloud mode silently does nothing. No navigation, no preview, no download — the user gets no feedback. This may be intentional (cloud files are not individually previewable yet), but it should either be removed (so `StorageBrowser` emits nothing and there is no registered handler) or show a "preview not available for cloud files" toast.
|
||||
**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:
|
||||
|
||||
**Fix:** At minimum, emit a user-visible toast:
|
||||
```js
|
||||
@file-open="file => useToastStore().show('Open file: ' + file.name, 'info')"
|
||||
```
|
||||
Or, if cloud file access is out of scope for this phase, add a comment and open a follow-up issue.
|
||||
|
||||
---
|
||||
|
||||
### WR-09: `SettingsView.vue` reads `window.location.search` directly instead of using the router
|
||||
|
||||
**File:** `frontend/src/views/SettingsView.vue:118-133`
|
||||
|
||||
**Issue:**
|
||||
```js
|
||||
const params = new URLSearchParams(window.location.search)
|
||||
```
|
||||
The `router.replace({ path: '/settings' })` call on line 124 correctly strips the query parameters from the URL, but reading from `window.location.search` instead of `useRoute().query` bypasses Vue Router's abstraction. In hash-mode routing this will silently fail because `window.location.search` is always empty — query params live in the hash segment. In memory-history mode (used in tests) it also fails. This creates a maintenance trap: if the project ever switches router modes, OAuth callback handling silently breaks.
|
||||
|
||||
**Fix:**
|
||||
```js
|
||||
import { useRoute } from 'vue-router'
|
||||
const route = useRoute()
|
||||
// ...
|
||||
onMounted(() => {
|
||||
const connectedProvider = route.query.cloud_connected
|
||||
const errorMsg = route.query.cloud_error
|
||||
// ...
|
||||
// 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: `TotpEnrollment.vue` — `copySecret` silently fails with no user feedback
|
||||
### IN-01: `console.error` left in `DocumentCard` production paths without user-visible feedback
|
||||
|
||||
**File:** `frontend/src/components/auth/TotpEnrollment.vue:173-180`
|
||||
**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`:
|
||||
|
||||
**Issue:**
|
||||
```js
|
||||
async function copySecret() {
|
||||
import { useToastStore } from '../../stores/toast.js'
|
||||
const toast = useToastStore()
|
||||
|
||||
async function moveToFolder(folderId) {
|
||||
showFolderPicker.value = false
|
||||
try {
|
||||
await navigator.clipboard.writeText(secret.value)
|
||||
secretCopied.value = true
|
||||
setTimeout(() => { secretCopied.value = false }, 2000)
|
||||
} catch {
|
||||
// Fail silently
|
||||
await moveDocument(props.doc.id, folderId)
|
||||
} catch (e) {
|
||||
toast.show('Move failed: ' + (e.message || 'unknown error'), 'error')
|
||||
}
|
||||
}
|
||||
```
|
||||
If the Clipboard API is unavailable (HTTP context, older browser, permission denied), the user clicks the copy button and nothing happens — no feedback at all. In a TOTP enrollment context, failure to copy the secret is significant: the user needs it as a fallback.
|
||||
|
||||
**Fix:** Show an error or fallback message:
|
||||
---
|
||||
|
||||
### 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
|
||||
} catch {
|
||||
// Clipboard API unavailable — instruct user to copy manually
|
||||
secretCopied.value = false
|
||||
error.value = 'Could not copy automatically. Please select and copy the key manually.'
|
||||
// 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-02: `AdminAiView.vue` — double blank line at line 212 and trailing blank line at 213 (cosmetic)
|
||||
### IN-04: `AppIcon.vue` and three other new UI components use Options API while all new views use `<script setup>`
|
||||
|
||||
**File:** `frontend/src/views/admin/AdminAiView.vue:212-213`
|
||||
**File:** `frontend/src/components/ui/AppIcon.vue`, `BreadcrumbBar.vue`, `EmptyState.vue`, `ToastContainer.vue`
|
||||
|
||||
**Issue:** Minor style inconsistency — extra blank line between import block and first constant definition. Minor, but inconsistent with the rest of the codebase.
|
||||
**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:** Remove one blank line.
|
||||
**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.
|
||||
|
||||
---
|
||||
|
||||
### IN-03: `StorageBrowser.skeleton.test.js` — UX-13 tests are all `.todo`
|
||||
|
||||
**File:** `frontend/src/components/storage/__tests__/StorageBrowser.skeleton.test.js:58-62`
|
||||
|
||||
**Issue:**
|
||||
```js
|
||||
describe('UX-13: StorageBrowser folder picker uses Teleport + getBoundingClientRect', () => {
|
||||
it.todo('folder picker dropdown is teleported to body (escapes overflow container)')
|
||||
it.todo('dropdown position reflects getBoundingClientRect of the trigger button')
|
||||
it.todo('window scroll while open recalculates position')
|
||||
})
|
||||
```
|
||||
Three tests for the StorageBrowser folder picker are marked todo. The equivalent tests were written for `DocumentCard` and `FolderRow` in `dropdown.test.js`, but the `StorageBrowser` version (which is the canonical picker going forward) has no coverage. Per CLAUDE.md: "every new function, endpoint, and UI component must have at least one test."
|
||||
|
||||
**Fix:** Implement the three tests for `StorageBrowser`'s teleported folder picker, mirroring the pattern already established in `dropdown.test.js`.
|
||||
|
||||
---
|
||||
|
||||
### IN-04: `AppSidebar.vue` — `sharedCount` counts all items, not just unread/new
|
||||
|
||||
**File:** `frontend/src/components/layout/AppSidebar.vue:244-247`
|
||||
|
||||
**Issue:**
|
||||
```js
|
||||
const data = await api.getSharedWithMe()
|
||||
const items = Array.isArray(data) ? data : (data.items ?? [])
|
||||
sharedCount.value = items.length
|
||||
```
|
||||
The badge shows a count of all documents shared with the user, not just newly shared ones since last visit. This means the badge will never decrease as the user reads items, so it functions more as a total indicator than an "unread" badge. This is likely intentional for v0.1 but is misleading UX — the badge appears as a notification count (purple pill, same pattern as an unread count) but never clears.
|
||||
|
||||
**Fix:** Document the intentional behaviour with a comment, or implement a "seen" timestamp so the badge reflects genuinely new shares. At minimum, rename the variable to `totalSharedCount` to make intent clear to future maintainers.
|
||||
|
||||
---
|
||||
|
||||
_Reviewed: 2026-06-16T00:00:00Z_
|
||||
_Reviewed: 2026-06-16T12:00:00Z_
|
||||
_Reviewer: Claude (gsd-code-reviewer)_
|
||||
_Depth: standard_
|
||||
|
||||
Reference in New Issue
Block a user