diff --git a/.planning/phases/10-ux-interaction/10-REVIEW.md b/.planning/phases/10-ux-interaction/10-REVIEW.md new file mode 100644 index 0000000..b351941 --- /dev/null +++ b/.planning/phases/10-ux-interaction/10-REVIEW.md @@ -0,0 +1,500 @@ +--- +phase: 10-ux-interaction +reviewed: 2026-06-16T00:00:00Z +depth: standard +files_reviewed: 45 +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 + - 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/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 + - 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/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 +findings: + critical: 4 + warning: 9 + info: 4 + total: 17 +status: issues_found +--- + +# Phase 10: Code Review Report + +**Reviewed:** 2026-06-16T00:00:00Z +**Depth:** standard +**Files Reviewed:** 45 +**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. + +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. + +--- + +## Critical Issues + +### CR-01: Duplicate account settings implementation — diverged behaviour + +**File:** `frontend/src/views/AccountView.vue:1` and `frontend/src/components/settings/SettingsAccountTab.vue:1` + +**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. + +**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. + +--- + +### CR-02: `closeFolderPicker` in `DocumentCard.vue` closes on any `.relative` element — not just its own trigger + +**File:** `frontend/src/components/documents/DocumentCard.vue:179-184` + +**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 + } +} +``` +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: +//
+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 `