docs: add complete codebase analysis report (50 issues documented)
This commit is contained in:
@@ -0,0 +1,684 @@
|
||||
# Code Analysis Report
|
||||
|
||||
> **Audit Date**: 2026-06-29
|
||||
> **Auditor**: Codebase Explorer (brutally honest mode)
|
||||
> **Verdict**: This codebase is a functional prototype with critical security holes, broken collaboration sync, and systemic architectural debt. It is NOT production-ready.
|
||||
|
||||
---
|
||||
|
||||
## Critical Issues (Must Fix Immediately)
|
||||
|
||||
### 1. [backend/src/routes/*.ts] NO Authentication on All CRUD Routes
|
||||
**Severity**: CRITICAL
|
||||
**Impact**: Complete data exposure and manipulation by anyone
|
||||
|
||||
The following routes have **ZERO authentication middleware**:
|
||||
- `projects.ts` — `GET /api/projects` returns ALL projects to anyone. `DELETE /api/projects/:id` deletes any project without ownership check.
|
||||
- `drawings.ts` — Full CRUD with no auth.
|
||||
- `elements.ts` — Full CRUD with no auth. Anyone can create/read/update/delete elements in any drawing.
|
||||
- `layers.ts` — Full CRUD with no auth.
|
||||
- `blocks.ts` — Full CRUD with no auth.
|
||||
- `settings.ts` — Full CRUD with no auth. Anyone can read/write application settings.
|
||||
- `users.ts` — `GET /api/users` lists ALL users (with password_hash stripped). `DELETE /api/users/:id` deletes any user. `PATCH /api/users/:id` updates any user. The `authService` is imported but **never used**. Comment says "TODO: add role check middleware" — it was never done.
|
||||
|
||||
Only `auth.ts`, `shares.ts`, `notifications.ts`, and `ai.ts` have proper auth checks.
|
||||
|
||||
**The frontend sends Bearer tokens, but the backend ignores them on all CRUD routes.**
|
||||
|
||||
### 2. [backend/src/websocket/yjsServer.ts:84] WebSocket Collaboration Has No Authentication
|
||||
**Severity**: CRITICAL
|
||||
**Impact**: Anyone can connect to any collaboration document and modify it in real-time
|
||||
|
||||
The `/ws/collab/:docName` endpoint accepts any WebSocket connection without token validation. An attacker can connect to `project-<any-id>` and inject/modify/delete all shared data.
|
||||
|
||||
### 3. [backend/src/server.ts:26] CORS Allows All Origins
|
||||
**Severity**: CRITICAL
|
||||
**Impact**: Any website can make authenticated requests to the API
|
||||
|
||||
```typescript
|
||||
await fastify.register(cors, {
|
||||
origin: true, // allows ALL origins
|
||||
methods: ['GET', 'POST', 'PUT', 'PATCH', 'DELETE', 'OPTIONS'],
|
||||
});
|
||||
```
|
||||
|
||||
Combined with the missing auth on CRUD routes, any website can read/modify all data.
|
||||
|
||||
### 4. [backend/src/websocket/yjsServer.ts:43] Yjs Persistence NEVER Loads — `persistenceReady` Is Never Set
|
||||
**Severity**: CRITICAL
|
||||
**Impact**: All collaboration data is lost on server restart
|
||||
|
||||
```typescript
|
||||
let persistenceReady: Promise<void> | null = null; // initialized as null
|
||||
|
||||
// line 43:
|
||||
if (persistence && persistenceReady) { // persistenceReady is ALWAYS null!
|
||||
await persistenceReady;
|
||||
const persistedYdoc = await persistence.getYDoc(docName);
|
||||
// ...
|
||||
}
|
||||
```
|
||||
|
||||
`persistenceReady` is declared but **never assigned**. The persistence loading block is dead code. Documents are persisted to LevelDB (via the `doc.on('update')` handler) but **never loaded back** on server restart or document re-creation.
|
||||
|
||||
Additionally, `initYjsPersistence()` creates the persistence instance but doesn't set `persistenceReady`:
|
||||
```typescript
|
||||
export async function initYjsPersistence(): Promise<void> {
|
||||
persistence = new LeveldbPersistence(PERSISTENCE_DIR);
|
||||
// persistenceReady is NOT set here!
|
||||
}
|
||||
```
|
||||
|
||||
### 5. [backend/src/websocket/yjsServer.ts:125] In-Memory Doc Deleted When Last Client Disconnects
|
||||
**Severity**: CRITICAL
|
||||
**Impact**: Document state lost from memory; relies on broken persistence (see issue #4)
|
||||
|
||||
```typescript
|
||||
socket.on('close', () => {
|
||||
// ...
|
||||
if (conns.size === 0) {
|
||||
connections.delete(docName);
|
||||
docs.delete(docName); // doc removed from memory!
|
||||
}
|
||||
});
|
||||
```
|
||||
|
||||
When the last client disconnects, the Y.Doc is deleted from memory. Since persistence loading is broken (issue #4), the next client to connect gets an empty document. All collaboration data is effectively ephemeral.
|
||||
|
||||
### 6. [frontend/src/App.tsx:280-319] CRDT Sync: Groups and bgConfig NOT Synced via Yjs
|
||||
**Severity**: CRITICAL
|
||||
**Impact**: Groups and background configuration are never shared between collaborators
|
||||
|
||||
`YjsDocument` only has maps for `elements`, `layers`, `blocks`, and `meta`. There are **no maps for `groups` or `bgConfig`**. The `useYjsBinding` hook doesn't expose group or bgConfig sync methods. When user A creates a group or sets a background, user B will never see it.
|
||||
|
||||
The remote→local sync effect (lines 280-319) only handles `collab.elements`, `collab.layers`, and `collab.blocks`. Groups and bgConfig are completely absent from the collaboration layer.
|
||||
|
||||
### 7. [frontend/src/App.tsx:267-269] loadFromState Called Before Data Is Loaded — Race Condition
|
||||
**Severity**: CRITICAL
|
||||
**Impact**: Empty local state can overwrite remote CRDT data
|
||||
|
||||
```typescript
|
||||
// Push initial data to Yjs CRDT after load
|
||||
if (collab.status === 'connected') {
|
||||
collab.loadFromState({ elements: data.elements, layers: data.layers, blocks: data.blocks });
|
||||
}
|
||||
```
|
||||
|
||||
`loadFromState` in `YjsDocument.ts` (line 82) **clears all maps and replaces with local state**. If the backend returned empty arrays (new project), this wipes any existing CRDT data from other connected clients. The check `collab.status === 'connected'` doesn't guarantee that remote data has been received and merged yet.
|
||||
|
||||
### 8. [frontend/src/App.tsx:128-142] Plugin Context Has Stale Closure — Captures Empty State
|
||||
**Severity**: CRITICAL
|
||||
**Impact**: Plugins always see empty elements and layers arrays
|
||||
|
||||
```typescript
|
||||
useEffect(() => {
|
||||
const ctx: PluginContext = {
|
||||
// ...
|
||||
getElements: () => elements, // captures `elements` from first render = []
|
||||
getLayers: () => layers, // captures `layers` from first render = initialLayers
|
||||
// ...
|
||||
};
|
||||
pluginRegistry.setContext(ctx);
|
||||
}, []); // EMPTY DEPS — runs once, captures stale state
|
||||
```
|
||||
|
||||
The effect has `[]` dependencies, so it runs once on mount. `getElements()` will always return `[]` (initial state). `getLayers()` will always return `initialLayers` (never updated). Any plugin using these methods gets stale data.
|
||||
|
||||
---
|
||||
|
||||
## High Priority Issues
|
||||
|
||||
### 9. [frontend/src/App.tsx:306-317] Band-Aid Guards for Empty Remote Arrays — Incomplete and Fragile
|
||||
**Severity**: HIGH
|
||||
**Impact**: Layers/blocks can still be overwritten in edge cases; elements merge logic is flawed
|
||||
|
||||
The guards `if (collab.layers.length > 0)` and `if (collab.blocks.length > 0)` prevent empty remote data from overwriting local defaults. But:
|
||||
|
||||
1. **Elements (lines 287-303)**: The merge logic compares ID sets but doesn't handle element **updates** from remote. If remote has the same IDs but modified properties, the merge only takes remote if local doesn't have the element. Existing elements keep local state, ignoring remote changes.
|
||||
|
||||
2. **Layers (lines 307-309)**: The comparison `collab.layers.every((l, i) => l.id === layers[i]?.id)` assumes **same ordering**. If remote layers are in a different order but have the same IDs, it triggers a `setLayers` call, causing unnecessary re-renders. If a layer was renamed remotely, the ID is the same, so `sameLayers` is true and the rename is **ignored**.
|
||||
|
||||
3. **No guard for element property changes**: When a remote user modifies an element (moves it, changes color), the local client won't receive the update if it has the element locally. The merge only adds remote-only elements, it doesn't update existing ones.
|
||||
|
||||
### 10. [frontend/src/App.tsx:588] Save Action Only Saves Elements Starting with 'el-'
|
||||
**Severity**: HIGH
|
||||
**Impact**: Elements created by block drop, import, or paste are silently skipped during save
|
||||
|
||||
```typescript
|
||||
if (action === 'save') {
|
||||
// ...
|
||||
elements.forEach((el) => {
|
||||
if (!el.id.startsWith('el-')) return; // skips el_, svg_, chair-, etc.
|
||||
updateElement(token, el.id, el).catch(() => {});
|
||||
});
|
||||
}
|
||||
```
|
||||
|
||||
Block instances have IDs like `el_${Date.now()}_abc` (underscore, not hyphen). Imported elements keep their original IDs. Pasted elements get `el-${Date.now()}-${i}`. Only hyphen-prefixed IDs are saved.
|
||||
|
||||
### 11. [frontend/src/App.tsx:509-517] Block Drop Doesn't Save to Backend or Yjs
|
||||
**Severity**: HIGH
|
||||
**Impact**: Placed blocks exist only in local state; lost on reload, not shared with collaborators
|
||||
|
||||
```typescript
|
||||
const handleBlockDrop = useCallback((blockId: string, x: number, y: number) => {
|
||||
// ...
|
||||
if (instance) {
|
||||
setElements((prev) => [...prev, instance]); // local only!
|
||||
// NO call to createElementTyped
|
||||
// NO call to collab.setElement
|
||||
}
|
||||
}, [blocks, activeLayerId]);
|
||||
```
|
||||
|
||||
No backend persistence, no CRDT sync. The element vanishes on page reload.
|
||||
|
||||
### 12. [frontend/src/App.tsx:528-550] Import Doesn't Save to Backend or Yjs
|
||||
**Severity**: HIGH
|
||||
**Impact**: Imported elements/layers/blocks exist only in local state
|
||||
|
||||
Same pattern as block drop. `handleImport` adds elements to local state but never calls `createElementTyped`, `createLayerTyped`, `createBlockTyped`, or `collab.setElement`.
|
||||
|
||||
### 13. [frontend/src/App.tsx:628] Copy/Paste Uses Non-Existent `selected` Property
|
||||
**Severity**: HIGH
|
||||
**Impact**: Copy always copies the first element, never the actual selection
|
||||
|
||||
```typescript
|
||||
const selected = elements.filter((e) => (e as any).selected);
|
||||
const clip = selected.length > 0 ? selected : elements.slice(0, 1);
|
||||
```
|
||||
|
||||
`CADElement` has no `selected` property in its type definition. The `SelectionEngine` tracks selection internally but doesn't annotate elements. `(e as any).selected` is always `undefined` (falsy), so `selected` is always empty, and `clip` always becomes `elements.slice(0, 1)`.
|
||||
|
||||
### 14. [frontend/src/App.tsx:1021] activeLayerName Hardcoded to 'layer-0'
|
||||
**Severity**: HIGH
|
||||
**Impact**: Status bar always shows the name of the initial first layer, not the active layer
|
||||
|
||||
```typescript
|
||||
const activeLayerName = layers.find((l) => l.id === 'layer-0')?.name ?? '—';
|
||||
```
|
||||
|
||||
This should use `activeLayerId`, not the hardcoded `'layer-0'`. If the user selects a different layer, the status bar still shows 'Wände'.
|
||||
|
||||
### 15. [frontend/src/App.tsx:754-789] Layer Operations Don't Persist to Backend
|
||||
**Severity**: HIGH
|
||||
**Impact**: Layer visibility, lock, color, line type, transparency, rename, duplicate changes are lost on reload
|
||||
|
||||
The following handlers modify local state but **never call the backend API**:
|
||||
- `handleToggleLayer` (line 754) — no `updateLayer` call
|
||||
- `handleRenameLayer` (line 766) — no `updateLayer` call
|
||||
- `handleDuplicateLayer` (line 769) — no `createLayerTyped` call
|
||||
- `handleToggleLock` (line 778) — no `updateLayer` call
|
||||
- `handleUpdateLayerColor` (line 781) — no `updateLayer` call
|
||||
- `handleUpdateLayerLineType` (line 784) — no `updateLayer` call
|
||||
- `handleUpdateLayerTransparency` (line 787) — no `updateLayer` call
|
||||
- `handleReorderLayer` (line 802) — no backend update
|
||||
|
||||
Only `handleAddLayer` and `handleAddSubLayer` persist to backend.
|
||||
|
||||
### 16. [frontend/src/App.tsx:436-438] Block Rename Doesn't Persist to Backend
|
||||
**Severity**: HIGH
|
||||
**Impact**: Block renames are lost on reload
|
||||
|
||||
`handleRenameBlock` only updates local state. No `updateBlock` API call.
|
||||
|
||||
### 17. [frontend/src/App.tsx:440-454] Block Duplicate Doesn't Persist to Backend
|
||||
**Severity**: HIGH
|
||||
**Impact**: Duplicated blocks are lost on reload
|
||||
|
||||
`handleDuplicateBlock` only updates local state. No `createBlockTyped` API call.
|
||||
|
||||
### 18. [frontend/src/App.tsx:906-912] GROUP Command Doesn't Actually Create Groups
|
||||
**Severity**: HIGH
|
||||
**Impact**: Group/ungroup commands are non-functional
|
||||
|
||||
```typescript
|
||||
if (upper === 'GROUP' || upper === 'GRP') {
|
||||
// For now, group all elements (selection state is in InteractionEngine, not accessible)
|
||||
setCommandHistory((prev) => [...prev, { prefix: '·', text: 'Gruppe erstellt (Auswahl im Canvas erforderlich)', type: 'info' }]);
|
||||
setGroups(gm.getGroups());
|
||||
return;
|
||||
}
|
||||
```
|
||||
|
||||
The comment admits it: selection state is in `InteractionEngine` but not accessible from `App.tsx`. The command does nothing useful — it just logs a message and reads the (empty) groups list.
|
||||
|
||||
### 19. [frontend/src/crdt/AwarenessManager.ts:38-39] Cursor Data Stored in Shared Y.Doc — Persisted to Disk
|
||||
**Severity**: HIGH
|
||||
**Impact**: Cursor positions are persisted to LevelDB and replayed on reconnect
|
||||
|
||||
```typescript
|
||||
this.state = {
|
||||
cursors: this.yjsDoc.doc.getMap<UserCursor>('awareness-cursors'),
|
||||
selections: this.yjsDoc.doc.getMap<UserSelection>('awareness-selections'),
|
||||
};
|
||||
```
|
||||
|
||||
Awareness data (cursors, selections) is stored in the same `Y.Doc` as document data. This means:
|
||||
1. Cursor movements trigger document updates and are persisted to LevelDB.
|
||||
2. On reconnect, old cursor positions are replayed (stale cursors appear).
|
||||
3. Cursor updates are broadcast as document updates, mixing transient state with persistent data.
|
||||
|
||||
Standard Yjs awareness uses a separate protocol that is NOT persisted. This implementation reinvents awareness incorrectly.
|
||||
|
||||
### 20. [frontend/src/crdt/WebSocketProvider.ts:62-65] Client Sends Local State on Connect — Conflicts with Server State
|
||||
**Severity**: HIGH
|
||||
**Impact**: Initial sync can cause data corruption when both client and server have data
|
||||
|
||||
```typescript
|
||||
this.ws.onopen = () => {
|
||||
this.setStatus('connected');
|
||||
const stateUpdate = Y.encodeStateAsUpdate(this.yjsDoc.doc);
|
||||
if (stateUpdate.length > 0) {
|
||||
this.ws?.send(stateUpdate); // sends local state to server
|
||||
}
|
||||
};
|
||||
```
|
||||
|
||||
The server also sends its state to the client (yjsServer.ts:100-103). Both sides apply each other's updates. Yjs CRDTs are designed to handle this, but the `loadFromState` call in App.tsx (which clears and replaces) can conflict with the server's state, causing data loss.
|
||||
|
||||
### 21. [backend/src/routes/projects.ts:9-11] listProjects Returns ALL Projects — No Owner Filtering
|
||||
**Severity**: HIGH
|
||||
**Impact**: Any user (or unauthenticated caller) sees all projects in the system
|
||||
|
||||
Even if auth were added, `listProjects()` returns all projects without filtering by `owner_id` or shares.
|
||||
|
||||
---
|
||||
|
||||
## Medium Priority Issues
|
||||
|
||||
### 22. [frontend/src/App.tsx:1207 lines] Monolithic Component — God Component Anti-Pattern
|
||||
**Severity**: MEDIUM
|
||||
**Impact**: Unmaintainable, every state change re-renders the entire component tree
|
||||
|
||||
`App.tsx` is 1207 lines containing:
|
||||
- All application state (30+ useState calls)
|
||||
- All event handlers (40+ useCallback calls)
|
||||
- All side effects (10+ useEffect calls)
|
||||
- The entire render tree
|
||||
- Auth gating logic
|
||||
|
||||
No state management library, no context splitting, no component decomposition beyond presentational children.
|
||||
|
||||
### 23. [frontend/src/App.tsx:380] Stale `elements` in handleElementsDeleted Dependencies
|
||||
**Severity**: MEDIUM
|
||||
**Impact**: Unnecessary callback re-creation, potential stale closure bugs
|
||||
|
||||
```typescript
|
||||
}, [elements, layers, blocks, groups, bgConfig, syncHistory, token, collab]);
|
||||
```
|
||||
|
||||
`elements` is in the dependency array but the callback uses `prev` (functional update), not `elements` directly. This causes the callback to be re-created on every element change, which cascades to all child components.
|
||||
|
||||
### 24. [frontend/src/App.tsx:607-621] Export Uses NW.js-Specific `nwsave` Attribute
|
||||
**Severity**: MEDIUM
|
||||
**Impact**: Export dialog doesn't work in standard browsers
|
||||
|
||||
```typescript
|
||||
input.setAttribute('nwsave', ''); // NW.js only!
|
||||
```
|
||||
|
||||
In a standard browser, this creates a file input that opens the file picker, not a save dialog. The filename extraction `input.value` may not work as expected.
|
||||
|
||||
### 25. [frontend/src/services/api.ts:354-388] Project Load Cache Race Condition
|
||||
**Severity**: MEDIUM
|
||||
**Impact**: Concurrent calls can create duplicate drawings
|
||||
|
||||
```typescript
|
||||
const projectLoadCache = new Map<string, Promise<ProjectData>>();
|
||||
|
||||
export async function loadProjectDataTyped(token: string, projectId: string): Promise<ProjectData> {
|
||||
const existing = projectLoadCache.get(projectId);
|
||||
if (existing) return existing;
|
||||
// ...
|
||||
projectLoadCache.set(projectId, promise);
|
||||
promise.finally(() => projectLoadCache.delete(projectId));
|
||||
return promise;
|
||||
}
|
||||
```
|
||||
|
||||
The cache deduplicates concurrent calls, but the `finally` cleanup runs as soon as the promise settles. If a second call comes in after the first completes but before React processes the result, it will create a new request (and potentially a new drawing).
|
||||
|
||||
### 26. [frontend/src/App.tsx:66] Global Mutable Set for StrictMode Dedup — Module-Level State
|
||||
**Severity**: MEDIUM
|
||||
**Impact**: State leaks between component instances; breaks if component is reused
|
||||
|
||||
```typescript
|
||||
const loadedProjects = new Set<string>(); // module-level mutable state
|
||||
```
|
||||
|
||||
This Set is never used in the current code (the dedup is handled by `projectLoadCache` in api.ts). Dead code that could cause confusion.
|
||||
|
||||
### 27. [frontend/src/App.tsx:170-173] SeatingService Instantiated on Every Elements Change
|
||||
**Severity**: MEDIUM
|
||||
**Impact**: Unnecessary object creation and garbage collection pressure
|
||||
|
||||
```typescript
|
||||
const seatCount = useMemo(() => {
|
||||
const svc = new SeatingService(); // new instance every time!
|
||||
return svc.countSeats(elements).total;
|
||||
}, [elements]);
|
||||
```
|
||||
|
||||
`SeatingService` is stateless but recreated on every `elements` change. Should be a module-level singleton or ref.
|
||||
|
||||
### 28. [frontend/src/App.tsx:467-472] SVG Import Doesn't Persist to Backend
|
||||
**Severity**: MEDIUM
|
||||
**Impact**: Imported SVG blocks are lost on reload
|
||||
|
||||
`handleSvgImport` adds the block to local state but doesn't call `createBlockTyped`.
|
||||
|
||||
### 29. [frontend/src/App.tsx:474-488] Save Group as Block Only Uses Single Selected Element
|
||||
**Severity**: MEDIUM
|
||||
**Impact**: Block-from-selection only captures one element
|
||||
|
||||
```typescript
|
||||
const selectedEls = selectedElement ? [selectedElement] : [];
|
||||
```
|
||||
|
||||
`selectedElement` is a single `CADElement | null`, not an array. Multi-element selections can't be saved as blocks.
|
||||
|
||||
### 30. [backend/src/database/SqliteAdapter.ts:267] Session Expiry Comparison Uses Wrong Format
|
||||
**Severity**: MEDIUM
|
||||
**Impact**: Expired sessions may not be cleaned up properly
|
||||
|
||||
```typescript
|
||||
deleteExpiredSessions(): void {
|
||||
this.db.prepare('DELETE FROM sessions WHERE expires_at < ?').run(Date.now());
|
||||
}
|
||||
```
|
||||
|
||||
`expires_at` is stored as a number (milliseconds from `Date.now() + SESSION_TTL_MS`), but SQLite may store it as a string depending on column type. The comparison `expires_at < Date.now()` works if both are numbers, but if SQLite stores it as text, the comparison is lexicographic.
|
||||
|
||||
### 31. [frontend/src/App.tsx:421-428] Text Edit Uses window.prompt — Blocking UX
|
||||
**Severity**: MEDIUM
|
||||
**Impact**: Poor UX, blocks the main thread, no styling control
|
||||
|
||||
```typescript
|
||||
const handleTextEdit = useCallback((el: CADElement) => {
|
||||
const text = window.prompt('Text eingeben:', '');
|
||||
// ...
|
||||
}, []);
|
||||
```
|
||||
|
||||
Also, the text edit doesn't persist to backend or sync to Yjs.
|
||||
|
||||
### 32. [frontend/src/App.tsx:657-680] Image Insert Doesn't Save to Backend or Yjs
|
||||
**Severity**: MEDIUM
|
||||
**Impact**: Inserted images exist only in local state
|
||||
|
||||
The image insert handler creates an element with a base64 data URL in properties but doesn't call `createElementTyped` or `collab.setElement`.
|
||||
|
||||
### 33. [backend/src/routes/*.ts] No Input Validation on Any Route
|
||||
**Severity**: MEDIUM
|
||||
**Impact**: Invalid data can be stored, potential for unexpected behavior
|
||||
|
||||
No route validates the structure of `request.body`. For example, `createElement` accepts any `Partial<DBElement>` — there's no check that `type` is a valid `ElementType`, that coordinates are numbers, or that `properties_json` is valid JSON.
|
||||
|
||||
### 34. [frontend/src/App.tsx:637-642] Paste Doesn't Save to Backend or Yjs
|
||||
**Severity**: MEDIUM
|
||||
**Impact**: Pasted elements exist only in local state
|
||||
|
||||
```typescript
|
||||
setElements((prev) => [...prev, ...pasted]);
|
||||
// NO createElementTyped, NO collab.setElement
|
||||
```
|
||||
|
||||
### 35. [frontend/src/crdt/useYjsBinding.ts:110] useEffect Dependencies Include Values That Don't Change
|
||||
**Severity**: MEDIUM
|
||||
**Impact**: Yjs document is recreated if userName or userColor changes (unlikely but possible)
|
||||
|
||||
```typescript
|
||||
}, [docName, wsUrl, userId, userName, userColor, enabled]);
|
||||
```
|
||||
|
||||
If `userName` or `userColor` changes (e.g., user updates their profile), the entire Yjs document is destroyed and recreated, losing all local state.
|
||||
|
||||
### 36. [frontend/src/App.tsx:145] Online Count Includes Self Even When Disconnected
|
||||
**Severity**: MEDIUM
|
||||
**Impact**: Status bar shows incorrect collaborator count
|
||||
|
||||
```typescript
|
||||
const onlineCount = collab.cursors.length + 1;
|
||||
```
|
||||
|
||||
Always adds 1 for self, even if collaboration is disconnected. Also, `cursors` only includes visible cursors from the awareness manager, which may include stale entries.
|
||||
|
||||
---
|
||||
|
||||
## Low Priority Issues
|
||||
|
||||
### 37. [frontend/src/App.tsx:50-63] Mock Data Used as Initial State
|
||||
**Severity**: LOW
|
||||
**Impact**: Misleading initial state; mock command history and KI messages shown before real data loads
|
||||
|
||||
`initialCommandHistory` and `initialKIMessages` are hardcoded mock data that show fake status messages ("110 Objekte", "Auto-Save aktiv") before any project is loaded.
|
||||
|
||||
### 38. [frontend/src/App.tsx:40-46] Initial Layers Hardcoded — Not from Backend
|
||||
**Severity**: LOW
|
||||
**Impact**: Default layers shown before project loads; may differ from backend layers
|
||||
|
||||
The 5 initial layers (Wände, Türen, Bestuhlung, Bühne, Hintergrund) are hardcoded. If the backend has different layers, there's a flash of wrong data before the load completes.
|
||||
|
||||
### 39. [frontend/src/services/api.ts:34,44,54] Return Type `any` for Auth Functions
|
||||
**Severity**: LOW
|
||||
**Impact**: No type safety on user object
|
||||
|
||||
```typescript
|
||||
export async function login(...): Promise<{ user: any; session: { token: string } }>
|
||||
export async function register(...): Promise<{ user: any; session: { token: string } }>
|
||||
export async function getMe(token: string): Promise<any>
|
||||
```
|
||||
|
||||
### 40. [frontend/src/App.tsx:667-671] Multiple `as any` Casts for Image Element
|
||||
**Severity**: LOW
|
||||
**Impact**: Type safety bypassed for image elements
|
||||
|
||||
```typescript
|
||||
const imgEl: CADElement = {
|
||||
id: `el-${Date.now()}`,
|
||||
type: 'image' as any, // 'image' is not in ElementType union!
|
||||
// ...
|
||||
} as any;
|
||||
```
|
||||
|
||||
`'image'` is not a valid `ElementType`. The type system is bypassed with `as any`.
|
||||
|
||||
### 41. [frontend/src/history/HistoryManager.ts] generateId() Method Never Used
|
||||
**Severity**: LOW
|
||||
**Impact**: Dead code
|
||||
|
||||
The `generateId()` method exists but is never called. History entries use hardcoded IDs like `'undo-0'`, `'current'`, `'redo-0'`.
|
||||
|
||||
### 42. [frontend/src/App.tsx:501-503] handleBlockSearch Is a No-Op
|
||||
**Severity**: LOW
|
||||
**Impact**: Dead code, search is handled elsewhere
|
||||
|
||||
```typescript
|
||||
const handleBlockSearch = useCallback((_query: string) => {
|
||||
// Search is handled locally in BlockLibrary component
|
||||
}, []);
|
||||
```
|
||||
|
||||
### 43. [frontend/src/App.tsx:691-695] Zoom Fit/100 Are No-Ops
|
||||
**Severity**: LOW
|
||||
**Impact**: Ribbon buttons don't actually zoom
|
||||
|
||||
```typescript
|
||||
if (action === 'zoom-fit') {
|
||||
setCommandHistory((prev) => [...prev, { prefix: '·', text: 'Zoom: an Ansicht angepasst', type: 'info' }]);
|
||||
return; // no actual zoom!
|
||||
}
|
||||
```
|
||||
|
||||
The `handleZoomFit` callback (line 797) also just logs a message. Only the canvas toolbar buttons (handleZoomIn/Out/Fit in CanvasArea) actually zoom.
|
||||
|
||||
### 44. [frontend/src/App.tsx:683-686] Format Actions Are No-Ops
|
||||
**Severity**: LOW
|
||||
**Impact**: Format buttons do nothing
|
||||
|
||||
```typescript
|
||||
if (action.startsWith('format-')) {
|
||||
setCommandHistory((prev) => [...prev, { prefix: '·', text: `Format: ${action.replace('format-', '')} (Auswahl erforderlich)`, type: 'info' }]);
|
||||
return;
|
||||
}
|
||||
```
|
||||
|
||||
### 45. [frontend/src/App.tsx:706-708] Search Action Is a No-Op
|
||||
**Severity**: LOW
|
||||
**Impact**: Search button does nothing
|
||||
|
||||
### 46. [backend/src/database/SqliteAdapter.ts:93] Indentation Inconsistency
|
||||
**Severity**: LOW
|
||||
**Impact**: Code smell
|
||||
|
||||
Line 93 has inconsistent indentation (space before `const sets`).
|
||||
|
||||
### 47. [frontend/src/App.tsx:159] BackgroundService Instantiated in useRef — Never Cleaned Up
|
||||
**Severity**: LOW
|
||||
**Impact**: Potential memory leak if BackgroundService holds resources
|
||||
|
||||
```typescript
|
||||
const bgServiceRef = React.useRef<BackgroundService>(new BackgroundService());
|
||||
```
|
||||
|
||||
The service is created once and never destroyed. If it holds image references or event listeners, they leak.
|
||||
|
||||
### 48. [frontend/src/App.tsx:349-352] handleToggleElementVisible Calls API Inside setElements
|
||||
**Severity**: LOW
|
||||
**Impact**: Side effect inside state updater — may cause double calls in StrictMode
|
||||
|
||||
```typescript
|
||||
setElements((prev) => prev.map((e) => {
|
||||
if (e.id === id) {
|
||||
// ...
|
||||
if (token) {
|
||||
updateElement(token, newEl.id, newEl).then(...).catch(...); // SIDE EFFECT IN UPDATER!
|
||||
}
|
||||
return newEl;
|
||||
}
|
||||
return e;
|
||||
}));
|
||||
```
|
||||
|
||||
Calling async API inside a state updater function is an anti-pattern. In React StrictMode, the updater may be called twice, causing duplicate API calls.
|
||||
|
||||
### 49. [frontend/src/App.tsx:951-960] Plugin Context Duplicated in handleCommand
|
||||
**Severity**: LOW
|
||||
**Impact**: Code duplication, same stale closure issue as issue #8
|
||||
|
||||
The `PluginContext` is recreated inside `handleCommand` with the same stale closure problem.
|
||||
|
||||
### 50. [frontend/src/App.tsx:799] handleZoomIn/Out/Fit Only Log Messages
|
||||
**Severity**: LOW
|
||||
**Impact**: Status bar zoom buttons are no-ops; actual zoom is in CanvasArea
|
||||
|
||||
---
|
||||
|
||||
## Architecture Overview & Assessment
|
||||
|
||||
### Stack
|
||||
- **Frontend**: React 18 + TypeScript + Vite, HTML5 Canvas rendering
|
||||
- **Backend**: Node.js + Fastify + better-sqlite3 + Yjs + y-leveldb
|
||||
- **Collaboration**: Custom Yjs WebSocket protocol (not y-websocket)
|
||||
- **Auth**: Custom JWT-like session tokens (not real JWT)
|
||||
|
||||
### Architecture Diagram
|
||||
```
|
||||
┌─────────────────────────────────────────────────────┐
|
||||
│ App.tsx (1207 lines) │
|
||||
│ ┌──────────────────────────────────────────────┐ │
|
||||
│ │ 30+ useState, 40+ useCallback, 10+ useEffect │ │
|
||||
│ │ All state, all handlers, all logic │ │
|
||||
│ └──────────────────────────────────────────────┘ │
|
||||
│ ┌──────┐ ┌────────┐ ┌──────────┐ ┌──────────────┐ │
|
||||
│ │Topbar│ │RibbonBar│ │LeftSidebar│ │ CanvasArea │ │
|
||||
│ │ │ │ │ │ │ │ (Canvas2D) │ │
|
||||
│ └──────┘ └────────┘ └──────────┘ └──────────────┘ │
|
||||
│ ┌──────────┐ ┌──────────┐ ┌────────────────────┐ │
|
||||
│ │RightSidebar│ │StatusBar│ │ CommandLine │ │
|
||||
│ │ │ │ │ │ │ │
|
||||
│ └──────────┘ └──────────┘ └────────────────────┘ │
|
||||
└─────────────────────────────────────────────────────┘
|
||||
│ │
|
||||
▼ ▼
|
||||
┌─────────────────┐ ┌──────────────────────┐
|
||||
│ api.ts (REST) │ │ Yjs CRDT (WebSocket) │
|
||||
│ fetch + Bearer │ │ Custom WS protocol │
|
||||
└─────────────────┘ └──────────────────────┘
|
||||
│ │
|
||||
▼ ▼
|
||||
┌─────────────────────────────────────────┐
|
||||
│ Fastify Backend │
|
||||
│ ┌─────────┐ ┌──────────┐ ┌──────────┐ │
|
||||
│ │ Routes │ │ AuthService│ │ Yjs WS │ │
|
||||
│ │(NO AUTH)│ │ │ │(NO AUTH) │ │
|
||||
│ └─────────┘ └──────────┘ └──────────┘ │
|
||||
│ ┌──────────────────────────────────┐ │
|
||||
│ │ SqliteAdapter (better-sqlite3) │ │
|
||||
│ │ + LevelDB (Yjs persistence) │ │
|
||||
│ └──────────────────────────────────┘ │
|
||||
└─────────────────────────────────────────┘
|
||||
```
|
||||
|
||||
### Assessment
|
||||
|
||||
**This is a prototype-quality codebase with significant issues across all dimensions.**
|
||||
|
||||
1. **Security**: Effectively non-existent. All CRUD routes and the WebSocket endpoint are completely open. This is the single most critical problem.
|
||||
|
||||
2. **Collaboration**: Broken in multiple ways. Persistence doesn't load, groups/bgConfig aren't synced, awareness is stored in the persistent document, and the sync merge logic is flawed.
|
||||
|
||||
3. **Data Persistence**: Inconsistent. Elements are saved on create/modify/delete, but layers, blocks, imports, paste, block drops, and image inserts are NOT saved. Many operations are local-only.
|
||||
|
||||
4. **Architecture**: God component anti-pattern. App.tsx is unmaintainable at 1207 lines with all state and logic. No state management, no context splitting, no separation of concerns.
|
||||
|
||||
5. **Type Safety**: Bypassed in multiple places with `as any` casts. The `ElementType` union doesn't include `'image'` but it's used.
|
||||
|
||||
6. **Error Handling**: Inconsistent. Some API calls have `.catch(() => {})` (silent failures), others log to console. No user-facing error feedback except command history messages.
|
||||
|
||||
7. **Code Quality**: Significant dead code (no-op handlers, unused variables), inconsistent patterns, and band-aid fixes for fundamental design issues.
|
||||
|
||||
---
|
||||
|
||||
## Summary & Recommendations
|
||||
|
||||
### The User's Assessment Is Correct
|
||||
The user believes "fast nichts brauchbares dabei" (almost nothing useful). This is **largely accurate**. The codebase has the structure of a real application but fails on fundamentals:
|
||||
|
||||
- **Security is completely absent** on all data routes
|
||||
- **Collaboration sync is broken** in 6+ different ways
|
||||
- **Data persistence is inconsistent** — many operations are local-only
|
||||
- **The God Component** makes the code unmaintainable
|
||||
- **Multiple no-op handlers** mean UI buttons do nothing
|
||||
|
||||
### What Does Work
|
||||
- Basic canvas rendering and interaction (drawing tools, selection, snap)
|
||||
- Auth flow (login/register/logout) — but only on auth routes
|
||||
- Project sharing routes have proper auth and ownership checks
|
||||
- Notification routes have proper auth
|
||||
- The HistoryManager (undo/redo) is well-implemented
|
||||
- The type definitions are reasonable
|
||||
- The block service logic (SVG import, instance creation) is functional
|
||||
|
||||
### Recommended Next Steps (Priority Order)
|
||||
|
||||
1. **Add auth middleware to ALL routes** — Create a Fastify preHandler that validates the Bearer token and attaches the user. Apply it to all route modules except auth.ts register/login.
|
||||
|
||||
2. **Fix Yjs persistence** — Set `persistenceReady` in `initYjsPersistence()`. Don't delete docs from memory on last disconnect (or implement a TTL cache).
|
||||
|
||||
3. **Add auth to WebSocket** — Validate token in the WebSocket upgrade handler.
|
||||
|
||||
4. **Sync groups and bgConfig via Yjs** — Add `groups` and `bgConfig` maps to `YjsDocument`.
|
||||
|
||||
5. **Fix data persistence** — All state-changing handlers must call the appropriate API. Layer toggles, block renames, imports, paste, block drops, image inserts.
|
||||
|
||||
6. **Fix the CRDT merge logic** — Use Y.Map observers for granular element updates instead of array replacement. Don't use `loadFromState` (which clears everything) after initial load.
|
||||
|
||||
7. **Fix the plugin context stale closure** — Use refs for `elements` and `layers` in the plugin context, or move plugin context to a separate provider.
|
||||
|
||||
8. **Decompose App.tsx** — Extract state into a reducer or state manager, split handlers into custom hooks, separate auth gating from editor.
|
||||
|
||||
9. **Fix CORS** — Restrict to known origins.
|
||||
|
||||
10. **Add input validation** — Use a schema validator (Zod, Joi, or Fastify schema) on all route bodies.
|
||||
Reference in New Issue
Block a user