curo/kite
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

docs(11): add code review report

Browse Source
This commit is contained in:
curo1305
2026-06-17 09:26:45 +02:00
parent f667a3bbc8
commit 8ac5b15f51
1 changed files with 345 additions and 0 deletions
Download Patch File Download Diff File Expand all files Collapse all files
.planning/phases/11-visual-design-responsive-layout-cleanup/11-REVIEW.md
+345
View File
@@ -0,0 +1,345 @@
---
phase: 11-visual-design-responsive-layout-cleanup
reviewed: 2026-06-17T00:00:00Z
depth: standard
files_reviewed: 13
files_reviewed_list:
- frontend/vite.config.js
- frontend/src/router/index.js
- frontend/src/App.vue
- frontend/src/layouts/AdminLayout.vue
- frontend/src/components/layout/AppSidebar.vue
- frontend/src/components/admin/AdminSidebar.vue
- frontend/src/components/storage/StorageBrowser.vue
- frontend/src/components/sharing/ShareModal.vue
- frontend/src/components/cloud/CloudCredentialModal.vue
- frontend/src/components/folders/FolderDeleteModal.vue
- frontend/src/components/documents/DocumentPreviewModal.vue
- frontend/src/views/DocumentView.vue
- frontend/src/router/__tests__/router.guard.test.js
findings:
critical: 4
warning: 7
info: 4
total: 15
status: issues_found
---
# Phase 11: Code Review Report
**Reviewed:** 2026-06-17T00:00:00Z
**Depth:** standard
**Files Reviewed:** 13
**Status:** issues_found
## Summary
This phase ships responsive drawer navigation, lazy-loading for routes, and modal polish. The core drawer implementation is structurally sound — drawer state is correctly owned by the layout components, backdrop is teleported to body to guarantee stacking, and route-change watchers auto-close the drawer. The router guard logic is correct and well-tested. `rollup-plugin-visualizer` is correctly placed in `devDependencies`.
Four blocker-class issues were found: the iframe in `DocumentPreviewModal` renders arbitrary blobs without a `sandbox` attribute (XSS vector in the app's own origin); `CloudCredentialModal` can submit an empty server URL without any client-side validation; `suggestTopics()` writes errors into the wrong ref (`classifyError` instead of a dedicated `suggestError`), causing stale errors to bleed across unrelated actions; and the high-traffic routes `/`, `/topics`, `/document/:id`, and `/settings` lack explicit `meta` flags, making the auth guard work only by convention and leaving unauthenticated access untested by the new guard test suite.
---
## Structural Findings (fallow)
None provided.
---
## Narrative Findings (AI reviewer)
## Critical Issues
### CR-01: iframe in DocumentPreviewModal has no sandbox attribute — XSS in app origin
**File:** `frontend/src/components/documents/DocumentPreviewModal.vue:52-57`
**Issue:** The preview iframe loads an object URL created from a blob fetched from the backend. No `sandbox` attribute is present on the `<iframe>`. Any document the backend serves — HTML files, PDFs with embedded JavaScript, SVG files — is rendered by the browser inside an iframe whose origin is `blob:<app-origin>/...`, which runs in the same origin as the Vue SPA. Script executing in that iframe has full access to `document.cookie`, `localStorage`, and can `postMessage` to the parent frame or make credentialed XHR requests. An attacker who can upload a file (any authenticated user) can craft an HTML document that exfiltrates auth state or performs CSRF actions against the API. The CLAUDE.md security protocol prohibits XSS via user data and requires no `innerHTML` with user content; a sandboxed iframe is the equivalent requirement for embedded document rendering.
**Fix:**
```html
<iframe
v-if="blobUrl"
class="w-full h-full border-0"
:src="blobUrl"
title="Document preview"
sandbox="allow-scripts"
></iframe>
```
For PDF rendering `allow-scripts` is required by most PDF.js-based renderers. If the iframe does not need to communicate back to the parent or access cookies, do not add `allow-same-origin`. If only PDFs are previewed, the strictest safe value is `sandbox="allow-scripts"`. Evaluate whether `allow-same-origin` is actually needed before adding it, as that combination weakens the sandbox significantly.
---
### CR-02: CloudCredentialModal submits empty server URL with no client-side validation
**File:** `frontend/src/components/cloud/CloudCredentialModal.vue:301-313`
**Issue:** `submit()` reads `finalUrl = resolvedServerUrl.value` and immediately calls `api.connectWebDav(props.provider.key, finalUrl, ...)`. `resolvedServerUrl` returns `autoServerUrl.value` for the Nextcloud path, which evaluates to `''` when either `serverBase` or `username` is empty (line 219: `if (!serverBase.value || !username.value) return ''`). For the plain WebDAV path, `serverUrl` initializes to `''` and has no HTML `required` attribute — the form's `@submit.prevent` bypass prevents browser validation from firing. There is no guard in `submit()` that checks `!finalUrl`. An empty string is sent to the backend, which may attempt a connection to a relative path or log a misleading error. The user sees "Connection failed" with no actionable message about what field is missing.
**Fix:**
```js
async function submit() {
connectError.value = ''
const finalUrl = resolvedServerUrl.value
if (!finalUrl) {
connectError.value = 'Please enter the server URL before connecting.'
return
}
if (!username.value.trim()) {
connectError.value = 'Please enter a username.'
return
}
saving.value = true
try {
await api.connectWebDav(props.provider.key, finalUrl, username.value, password.value)
emit('connected')
emit('close')
} catch (e) {
connectError.value = e.message || 'Connection failed. Please check your credentials.'
} finally {
saving.value = false
}
}
```
---
### CR-03: `suggestTopics()` writes errors into `classifyError` — wrong ref, stale error leaks across actions
**File:** `frontend/src/views/DocumentView.vue:246-257`
**Issue:** The `suggestTopics` function (line 253) assigns exceptions to `classifyError.value` — the same ref used by `reclassify()`. Two concrete bugs result:
1. `suggestTopics()` never resets `classifyError` before running (unlike `reclassify()` which clears it at line 234), so a prior reclassify failure remains visible while the suggest request is in-flight.
2. A `suggestTopics` failure displays an error under the Topics header near the Re-classify button — the wrong location for a suggest-related message, misleading the user into thinking re-classification failed.
3. Running `reclassify()` after a `suggestTopics` failure will clear the suggest error (correct), but running `suggestTopics` after a `reclassify` failure will leave the reclassify error visible (incorrect).
**Fix:** Add a separate `suggestError` ref and reset it at function entry:
```js
const suggestError = ref(null)
async function suggestTopics() {
suggesting.value = true
suggestError.value = null
try {
const result = await api.suggestTopics(doc.value.id)
suggestions.value = result.suggested
selectedSuggestions.value = []
} catch (e) {
suggestError.value = e.message || 'Failed to fetch suggestions.'
} finally {
suggesting.value = false
}
}
```
Add `<p v-if="suggestError" class="text-red-500 text-xs mt-2">{{ suggestError }}</p>` near the Suggest Topics button.
---
### CR-04: Routes `/`, `/topics`, `/document/:id`, `/settings` have no meta — auth guard relies on implicit convention, unauthenticated access untested
**File:** `frontend/src/router/index.js:13-17`
**Issue:** The five routes on lines 13–17 have no `meta` object. The navigation guard at line 96 uses `!to.meta.public` to decide whether to attempt a silent token refresh — this works because `undefined` is falsy. However:
1. Future guard branches that check `to.meta.requiresAuth` explicitly (mirroring the pattern on `/cloud`, `/folders/:folderId`, `/shared`) would silently skip these routes, potentially allowing unauthenticated access to the file manager and document pages.
2. The D-09 admin-redirect guard (line 113: `!isAdminRoute && !to.meta.public && isAdmin`) correctly fires on these routes only because of the same implicit convention. This is correct today but fragile.
3. The `router.guard.test.js` test suite covers unauthenticated redirect only for `/settings` (via the refresh-fails test at line 183) and does not include a negative test asserting that navigating to `/` or `/document/some-uuid` without a token redirects to `/login`. This means the primary auth protection on the file manager has no regression test.
**Fix:** Add explicit meta to all five routes and add unauthenticated-redirect tests:
```js
{ path: '/', component: FileManagerView, meta: { requiresAuth: true } },
{ path: '/topics', component: () => import('../views/TopicsView.vue'), meta: { requiresAuth: true } },
{ path: '/topics/:name', component: () => import('../views/TopicsView.vue'), meta: { requiresAuth: true } },
{ path: '/document/:id', component: () => import('../views/DocumentView.vue'), meta: { requiresAuth: true } },
{ path: '/settings', component: () => import('../views/SettingsView.vue'), meta: { requiresAuth: true } },
```
Add to `router.guard.test.js`:
```js
it('unauthenticated user visiting / is redirected to /login', async () => {
useAuthStore.mockReturnValue({
accessToken: null, user: null,
refresh: vi.fn().mockRejectedValue(new Error('no session')),
})
await router.push('/')
expect(router.currentRoute.value.path).toBe('/login')
})
```
---
## Warnings
### WR-01: AdminLayout has no Escape key handler for the mobile drawer
**File:** `frontend/src/layouts/AdminLayout.vue:46-59`
**Issue:** `App.vue` registers a `keydown` listener on `document` (lines 98–100) but that listener is not active when the admin layout is rendered — `App.vue` routes admin paths through a bare `<router-view />` (line 3), bypassing the `div` that owns the keydown listener. `AdminLayout.vue` registers no keyboard event handler. An admin user who opens the mobile drawer on a phone cannot close it with Escape. The `CloudCredentialModal` uses `@keydown.escape.window` which confirms the escape pattern is understood; the drawer simply lacks it.
**Fix:**
```js
// Add to AdminLayout.vue <script setup>
import { ref, watch, onMounted, onUnmounted } from 'vue'
function onKeydown(e) {
if (e.key === 'Escape' && drawerOpen.value) {
drawerOpen.value = false
}
}
onMounted(() => document.addEventListener('keydown', onKeydown))
onUnmounted(() => document.removeEventListener('keydown', onKeydown))
```
---
### WR-02: Folder picker outside-click handler uses `.closest('.relative')` — brittle class-based selector
**File:** `frontend/src/components/storage/StorageBrowser.vue:411-418`
**Issue:** `onOutsideClick` dismisses the folder picker unless the clicked element is inside `[data-test="folder-picker"]` **or** inside any element with class `.relative`. The class `relative` is a Tailwind positioning utility applied to many elements in the DOM tree. As the surrounding layout evolves, any new `class="relative"` ancestor element anywhere on the page will silently prevent the picker from closing when users click in that area. This is not currently an active bug but is a maintenance time-bomb.
**Fix:** Replace the class selector with a dedicated data attribute on the trigger button wrapper:
```html
<!-- In the template, change line 153 -->
<div class="relative" data-picker-trigger>
```
```js
function onOutsideClick(e) {
if (
!e.target.closest('[data-test="folder-picker"]') &&
!e.target.closest('[data-picker-trigger]')
) {
folderPickerFileId.value = null
}
}
```
---
### WR-03: Folder picker flip-up condition is logically incorrect — picker clips at 120–239 px remaining
**File:** `frontend/src/components/storage/StorageBrowser.vue:379`
**Issue:** The positioning logic:
```js
if (spaceBelow >= dropH || spaceBelow > 120) {
```
where `dropH = 240`. The second sub-condition `spaceBelow > 120` is a subset of `spaceBelow >= 240` only partially — when `120 < spaceBelow < 240`, the first condition is false but the second is true, so the picker still opens **below** the trigger even though only 50–99% of its height fits in the viewport. The flip-above path only activates when `spaceBelow <= 120`. In practice, clicks near the bottom third of a laptop screen (where the file list ends) will show a partially clipped picker instead of flipping it upward.
**Fix:** Use a single threshold:
```js
function updatePickerPosition(rect) {
const spaceBelow = window.innerHeight - rect.bottom
const dropH = 240
if (spaceBelow >= dropH) {
pickerStyle.value = { top: `${rect.bottom + 4}px`, left: `${rect.left}px`, width: '192px' }
} else {
pickerStyle.value = { bottom: `${window.innerHeight - rect.top + 4}px`, left: `${rect.left}px`, width: '192px' }
}
}
```
---
### WR-04: No modal locks body scroll — background content scrolls behind overlay on mobile
**File:** `frontend/src/components/sharing/ShareModal.vue:4`, `frontend/src/components/folders/FolderDeleteModal.vue:4`, `frontend/src/components/documents/DocumentPreviewModal.vue:6`, `frontend/src/components/cloud/CloudCredentialModal.vue:4`
**Issue:** None of the four modals add `overflow: hidden` to `document.body` on mount. On mobile and on any scrollable page, the `fixed inset-0` overlay does not prevent the background from scrolling beneath it. `ShareModal` and `CloudCredentialModal` have their own inner `overflow-y-auto` panels, creating a double-scroll UX where a user dragging on the panel may inadvertently scroll the background page.
**Fix:** Add scroll lock in each modal's `onMounted`/`onUnmounted`. A composable avoids duplication:
```js
// src/composables/useBodyScrollLock.js
import { onMounted, onUnmounted } from 'vue'
export function useBodyScrollLock() {
onMounted(() => { document.body.style.overflow = 'hidden' })
onUnmounted(() => { document.body.style.overflow = '' })
}
```
Call `useBodyScrollLock()` in each of the four modal `<script setup>` blocks.
---
### WR-05: FolderDeleteModal fires both `emit('confirm')` and `props.onConfirm()` — dual-invocation risk
**File:** `frontend/src/components/folders/FolderDeleteModal.vue:79-87`
**Issue:** `handleConfirm()` calls `emit('confirm')` then conditionally calls `props.onConfirm()`. `handleCancel()` does the same for cancel. A parent that binds both `@confirm="deleteFolder"` and `:on-confirm="deleteFolder"` will invoke the irreversible delete operation twice. The second invocation will likely 404 after the first succeeds, but the double navigation or double toast may leave the UI in an inconsistent state. This dual-API pattern (Vue events + callback props) has no justification in the codebase; the Vue convention is emits only.
**Fix:** Remove `onConfirm` and `onCancel` props entirely. Standardize on `@confirm` / `@cancel` emits. Update all callers to use the event binding.
---
### WR-06: `handleRevoke` reads `shares.value[-1]` before the bounds check
**File:** `frontend/src/components/sharing/ShareModal.vue:210-212`
**Issue:**
```js
const removedIdx = shares.value.findIndex(s => s.id === shareId)
const removed = shares.value[removedIdx] // reads undefined when removedIdx === -1
if (removedIdx !== -1) shares.value.splice(removedIdx, 1)
```
In JavaScript, `array[-1]` is `undefined` (not the last element). When `findIndex` returns `-1`, `removed` is `undefined`. The guard on line 220 (`if (removed && removedIdx !== -1)`) prevents corruption today, but reading at a negative index and then depending on the result being falsy is fragile — if `removed` ever could be a falsy-but-valid object (e.g., `null`), the re-insert guard would silently fail. The early-return pattern is safer.
**Fix:**
```js
async function handleRevoke(shareId) {
const removedIdx = shares.value.findIndex(s => s.id === shareId)
if (removedIdx === -1) return
const removed = shares.value[removedIdx]
shares.value.splice(removedIdx, 1)
try {
await docsStore.revokeShare(shareId)
toast.show('Share revoked', 'success')
if (shares.value.length === 0) emit('unshared', props.doc.id)
} catch (e) {
shares.value.splice(removedIdx, 0, removed)
error.value = e.message || 'Failed to remove access.'
}
}
```
---
### WR-07: vite.config.js conditionalAnalyzer dynamic import throws on missing package — no user-facing guidance
**File:** `frontend/vite.config.js:7-12`
**Issue:** `conditionalAnalyzer` returns a rejected Promise when `rollup-plugin-visualizer` is not installed. The package is in `devDependencies` so it is absent in production installs. If `ANALYZE=true` is accidentally set in a production CI environment (or the package is pruned), the `await conditionalAnalyzer(env)` on line 17 will reject and abort the Vite build with an opaque `Cannot find package` error. There is also no `"analyze"` script in `package.json`; the usage is documented only in a code comment.
**Fix:** Add a try/catch to degrade gracefully:
```js
async function conditionalAnalyzer(env) {
if (env.ANALYZE !== 'true') return null
try {
const { visualizer } = await import('rollup-plugin-visualizer')
return visualizer({ open: false, filename: 'stats.html', gzipSize: true })
} catch {
console.warn('[vite] rollup-plugin-visualizer not found — skipping. Install it or unset ANALYZE.')
return null
}
}
```
Add to `package.json` scripts: `"analyze": "ANALYZE=true vite build"`.
---
## Info
### IN-01: `fetchDocumentContent` imported twice in DocumentView.vue
**File:** `frontend/src/views/DocumentView.vue:156-157`
**Issue:**
```js
import * as api from '../api/client.js'
import { fetchDocumentContent } from '../api/client.js'
```
`fetchDocumentContent` is accessible as `api.fetchDocumentContent`. The named import on line 157 is redundant and unused as a standalone — line 206 calls `fetchDocumentContent(doc.value.id)` while the `import * as api` already covers it.
**Fix:** Remove line 157 and change line 206 to `api.fetchDocumentContent(doc.value.id)`.
---
### IN-02: `confirmDelete` in DocumentView uses `window.confirm` instead of a modal
**File:** `frontend/src/views/DocumentView.vue:272`
**Issue:** `confirm(...)` is a synchronous browser-native dialog that cannot be styled, is suppressed in some embedded contexts (PWAs, iframes), and is inconsistent with the rest of the app's modal-based confirmation UX. The folder delete path uses a proper `FolderDeleteModal`; the document delete path uses `window.confirm`. This creates an inconsistent experience and is blocked in some mobile browsers.
**Fix:** Replace with a `DocumentDeleteModal` component or generalize `FolderDeleteModal` into a configurable `ConfirmDeleteModal` that accepts `title`, `body`, and confirm-button text as props.
---
### IN-03: Two `console.error` calls remain in production path in DocumentView
**File:** `frontend/src/views/DocumentView.vue:208, 223`
**Issue:** Both are in `openPdf()`. The first (line 208) silently returns on a non-2xx response with no user-facing feedback at all. The second (line 223) logs to the console on a network error without surfacing a toast. These are debug artifacts.
**Fix:** Remove `console.error`. For the `res.status` failure case, emit a toast: `toast.show('Failed to open document', 'error')`.
---
### IN-04: AppSidebar shared-with-me count has no refresh path after share/unshare
**File:** `frontend/src/components/layout/AppSidebar.vue:242-248`
**Issue:** The badge count is fetched once on mount. After `ShareModal` emits `'unshared'` (reducing the share count) or after a share is created, the sidebar badge remains stale for the session lifetime. This is a UX inconsistency rather than a correctness defect.
**Fix:** Move the shared-count state into a Pinia store (or expose a `refresh()` method on the store). `ShareModal` can call `sharedStore.refresh()` after a successful share or revoke.
---
_Reviewed: 2026-06-17T00:00:00Z_
_Reviewer: Claude (gsd-code-reviewer)_
_Depth: standard_