22 KiB
phase, reviewed, depth, files_reviewed, files_reviewed_list, findings, status
| phase | reviewed | depth | files_reviewed | files_reviewed_list | findings | status | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| 10-ux-interaction | 2026-06-16T00:00:00Z | standard | 45 |
|
|
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:
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:
// 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:
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:
- If the component is unmounted before it ever mounts (e.g. destroyed in a v-if branch during the same tick),
onUnmountedfires but the listeners were already registered against the specificonScrollclosure; 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. - In SSR environments
windowis undefined and this crashes at import time. - 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:
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:
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:
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.)
Warnings
WR-01: onDropDocOnFolder in StorageBrowser.vue does not guard against dropping a folder onto itself
File: frontend/src/components/storage/StorageBrowser.vue:377-384
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.
Fix: Add an early return in onDropDocOnFolder:
async function onDropDocOnFolder(folderId) {
if (!draggingFile.value) return
if (draggingFile.value.id === folderId) { // ← guard
dragOverFolderId.value = null
return
}
// ...
}
WR-02: confirmRemoveOnly in DocumentView.vue silently swallows errors
File: frontend/src/views/DocumentView.vue:281-289
Issue:
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:
} 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:
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:
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:
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:
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:
function onOutsideClick(e) {
if (
!e.target.closest('[data-test="folder-picker"]') &&
!e.target.closest('.relative') // ← too broad
) {
folderPickerFileId.value = null
}
}
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:
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-07: SearchableModelSelect.vue — handleBlur 150 ms timeout may close dropdown before mousedown fires on a list item
File: frontend/src/components/ui/SearchableModelSelect.vue:214-216
Issue:
function handleBlur() {
setTimeout(close, 150)
}
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:
// On <ul>: @mousedown="listMousedown = true" @mouseup="listMousedown = false"
function handleBlur() {
setTimeout(() => { if (!listMousedown) close() }, 0)
}
WR-08: CloudFolderView.vue @file-open handler is a no-op
File: frontend/src/views/CloudFolderView.vue:14
Issue:
@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.
Fix: At minimum, emit a user-visible toast:
@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:
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:
import { useRoute } from 'vue-router'
const route = useRoute()
// ...
onMounted(() => {
const connectedProvider = route.query.cloud_connected
const errorMsg = route.query.cloud_error
// ...
})
Info
IN-01: TotpEnrollment.vue — copySecret silently fails with no user feedback
File: frontend/src/components/auth/TotpEnrollment.vue:173-180
Issue:
async function copySecret() {
try {
await navigator.clipboard.writeText(secret.value)
secretCopied.value = true
setTimeout(() => { secretCopied.value = false }, 2000)
} catch {
// Fail silently
}
}
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:
} 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.'
}
IN-02: AdminAiView.vue — double blank line at line 212 and trailing blank line at 213 (cosmetic)
File: frontend/src/views/admin/AdminAiView.vue:212-213
Issue: Minor style inconsistency — extra blank line between import block and first constant definition. Minor, but inconsistent with the rest of the codebase.
Fix: Remove one blank line.
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:
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:
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 Reviewer: Claude (gsd-code-reviewer) Depth: standard