diff --git a/CODEBASE-REVIEW.md b/CODEBASE-REVIEW.md index 682d28d..8dbc347 100644 --- a/CODEBASE-REVIEW.md +++ b/CODEBASE-REVIEW.md @@ -1,2995 +1,663 @@ # Codebase Review - graph-core -_Full multi-agent review of 2026-08-26 (v1.16.0, commit 719ef04): 304 files across `src/` and `electron/` read file-by-file by 22 review agents; every high and medium finding was then handed to an independent agent instructed to refute it (258 agents, 12.1M tokens, 3764 tool calls). Findings below are the surviving set. CSS was excluded from the review scope; the 180 low-severity notes were not adversarially verified and sit in the appendix._ +_Multi-agent review of 261015 (1.20.0-rc.3, commit 8553ef8). 220 of 226 source files in `src/`, `electron/`, `shared/` and `scripts/` were read by 20 review agents (two passes; the second covered files the first pass skipped). Every high and medium finding was given to an independent agent instructed to refute it. Low-severity notes were not adversarially verified and are in the appendix. CSS and test files were out of scope. The six type-declaration files in `src/types/` (944 lines, declarations only) were not read by a reviewer; they are covered by `vue-tsc`. This replaces the 260826 review._ -> **Status: reported, not remediated.** Nothing in this document has been fixed. Three unrelated fixes landed the same day (detached-panel layout, search ranking, notes-editor newline) and are not part of this review's findings. +> **Status: reported, not remediated.** ## Baseline gates -| Gate | Command | Result | -| ----------------------- | --------------------------------------------- | --------------------------------------------------- | -| Lint | `npm run lint` | Pass - 0 errors, 475 warnings (warn-only by config) | -| Format | `npm run format:check` | Pass - clean | -| Types | `npm run type-check` (`vue-tsc --noEmit`) | Pass - clean | -| Tests | `npm run test:run` | Pass - 1578/1578 in 96 files | -| Docs | `make docs-build` (`zensical build --strict`) | Pass - no issues | -| File size (<= 1000 LOC) | - | **Fail** - `src/App.vue` at 1149 | +| Gate | Command | Result | +| ------------------------- | ---------------------- | ----------------------------------------------------------------------------------------------------- | +| Lint | `npm run lint` | Pass - 0 errors, 499 warnings (warn-only by config) | +| Format | `npm run format:check` | Pass | +| Types | `npm run type-check` | Pass | +| Dead code | `npx knip` | Pass - 2 config hints (redundant entry patterns for `src/main.js`, `electron/main.js` in `knip.json`) | +| Tests | `npm run test:run` | Pass - 2306/2306 in 162 files | +| Docs | `make docs-build` | Pass | +| File size (<= 1000 lines) | - | **Fail** - `src/components/DetailPanel.vue` 1192, `src/App.vue` 1186, `src/services/api.ts` 1011 | -The 475 lint warnings break down as 187 `max-nested-callbacks`, 181 `max-lines-per-function`, 61 `complexity`, 30 `no-unused-vars`, 9 `max-lines`, 7 `max-depth`. Two are worth acting on independently of the findings below: the **30 `no-unused-vars`** warnings are real dead code that the gate lets through silently, and the **9 `max-lines`** warnings show eslint already has a file-size rule configured that is not set to fail at the project's 1000-line limit. No TODO/FIXME/HACK markers exist anywhere in the source. +Lint warnings: 200 `max-nested-callbacks`, 190 `max-lines-per-function`, 60 `complexity`, 31 `no-unused-vars`, 11 `max-lines`, 7 `max-depth`. Unchanged since 260826: the 31 `no-unused-vars` warnings are dead code the gate lets through, and `max-lines` is configured but does not fail at the 1000-line limit. ## Summary -- **416 raw findings** -> **205 confirmed** (6 high, 91 medium, 108 low), **31 refuted**, 180 lower-confidence notes not adversarially verified (appendix). -- Confirmed by category: correctness 99, dead-code 63, consistency 17, design 16, typing 8, docstring 1, naming 1. - -Severity is the verifier's corrected severity where it differs from the reviewer's. Refutation was real: 31 findings were thrown out, and a number of others were downgraded rather than dropped. +- Confirmed: **57** (1 high, 30 medium, 26 low). Refuted: 3. Unverified low-confidence notes: 152 (appendix). +- By category: consistency 6, correctness 40, dead-code 7, design 4, docstring 1. +- Severity shown is the verifier corrected severity where it differs from the reviewer. ### Recurring themes -1. **Sensitive-note encryption leaks around its own boundary.** Decrypted plaintext is persisted to `sessionStorage` by the undo stack; disabling the feature permanently destroys trashed sensitive notes; masking is absent from person/organization notes and from the table view's Notes column; the wrapped key is handed to the renderer through `DB_GET_ALL_SETTINGS`. -2. **Props and events dropped between component layers.** `MainToolbar` sits between `App.vue` and `SettingsPanel` and silently swallows `currentWorkspace`, the SSL-skip toggle, and `import-complete`. `TableView`, `CardsView` and `DetachedView` each emit a payload shape their handler does not expect. -3. **No in-flight guards on async loaders.** `loadChildren`, `initGraph`, `loadTasks`, the search's `loadMoreResults`, and `PersonDetailForm`'s org watcher all race; several let a stale result win. -4. **Errors collected into refs nothing reads.** `useNodeTable`, `useDataLoading` and others write `loading`/`error` on every path with no UI consumer, so failures are fully silent. -5. **Undo/redo integrity.** Stacks are global and survive a workspace switch; redo of a create mints a new id and orphans later commands; several mutating operations push no command at all; nothing broadcasts to detached windows. -6. **Table and tree invariants in the DB layer.** Deleting a column never reindexes cells, shifting every column's data left; `setCells` uses `INSERT OR REPLACE` and erases omitted columns; `restoreNode` can restore under a still-deleted parent; `moveNode` has no ancestor guard. -7. **Forked logic that has already diverged.** The agent loop exists in both `electron/ipc/agent.js` and `src/services/agentService.js`; sidebar collapse state is forked between `useSidebar.ts` and `AppSidebar.vue`; `isTextInput` duplicates the `inputOwnership` module that was created to own that decision. -8. **Unreachable subsystems.** The graph edit/prompt modal subsystem, most of `useViewStateController`, several `useSelection` helpers and nine timing constants are built but never reached. - -## High (6) - -Each of these is reachable from the UI and loses or corrupts user data, or breaks a shipped feature outright. - -#### 1. reload() bypasses the encryption choke point and always fails on an encrypted database - -`electron/database/backup.js`:82 - correctness - -`reload()` reads the file with `fs.readFileSync(ctx.dbPath)` and hands the raw bytes straight to `new ctx.SQL.Database(buffer)`. Every other read path goes through `ctx._deserialize` (`index.js:_init` line 61, `restoreBackup` line 66). An encrypted file begins with the ASCII magic `GCENC1` (`encryption.js:isEncrypted`), so sql.js rejects it with "file is not a database" and the handler throws. `docs/architecture/encryption.md:36` names exactly `_save`, `backup`, and `restoreBackup` as the choke-point users and omits `reload`, confirming this is an oversight rather than a deliberate exception. This is reachable from the UI: `electron/ipc/database.js:184` registers `DB_RELOAD` -> `db.reload()`, `useMaintenanceDialogs.js:47` passes `api.reload` into `useSnapshots.reloadDatabase()`, and `App.vue:900` binds `@reload-database`. Result: on any encrypted database, the maintenance "Reload database" action is permanently broken. Note also that `reload()` does not re-run `createTables`/`runMigrations` after swapping `ctx.db`. - -**Fix:** Change to `const buffer = ctx._deserialize(fs.readFileSync(ctx.dbPath))`, mirroring `restoreBackup`. Add a test that constructs an encrypted Database, calls `reload()`, and asserts the node count (the existing `database-encryption.test.js` covers `backup()` but not `reload()`). Add `reload` to the choke-point list in docs/architecture/encryption.md. - -#### 2. SENSITIVE_DISABLE permanently loses trashed sensitive notes - -`electron/ipc/sensitiveNotes.js`:84 - correctness - -The disable handler selects every encrypted note including soft-deleted ones: - - const encrypted = db._query("SELECT id FROM nodes WHERE notes LIKE 'SNENC1:%'") - for (const { id } of encrypted) { - db.updateNode(id, { notes_sensitive: false }) - } - db.deleteSetting(SETTINGS_KEY) - -but the decrypt path cannot handle a trashed row. `updateNode` -> `_encodeNotesForWrite(id, data)` calls `this.getNode(id)`, whose SQL is `WHERE id = ? AND deleted_at IS NULL`. For a trashed node `existing` is null, so `wasSensitive=false`, `willBeSensitive=false`, `toggling=false`, `'notes' in data` is false, and the function returns `data` unchanged. The subsequent `UPDATE nodes SET notes_sensitive = ?, updated_at = ... WHERE id = ?` has no deleted filter, so the row DOES get `notes_sensitive = 0` while `notes` stays `SNENC1:...`. Then `deleteSetting` + `setSession(null)` drop the only key that could ever decrypt it. - -I verified this against the real Database in a throwaway vitest probe (since removed). Output: - - rows selected: [ { id: 1 }, { id: 2 } ] - after: [ - { id: 1, title: 'live', notes: 'secret-live', notes_sensitive: 0 }, - { id: 2, title: 'trashed', notes: 'SNENC1:bnygcb+FEouNk1R7wJF9NhIT...', notes_sensitive: 0 } - ] - -Restoring that node from the trash afterwards shows raw ciphertext with no flag and no way to recover the plaintext. - -**Fix:** Decrypt notes directly rather than going through `updateNode`'s live-node-only toggle path: read `id, notes` in the SELECT, call `session.decryptForRead(notes)` (or `decryptNote`) per row, and write the plaintext back with an explicit `UPDATE nodes SET notes = ?, notes_sensitive = 0 WHERE id = ?`. Add a regression test covering a trashed sensitive note across disable. - -#### 3. Hidden quick-capture window blocks window-all-closed and defeats the macOS activate handler - -`electron/main.js`:446 - correctness - -quickCapture.js builds its capture window once and only ever calls `win.hide()` (quickCapture.js:34, :62) — it is never closed or destroyed. A hidden BrowserWindow is still an open window, so it stays in `BrowserWindow.getAllWindows()`. - -Two consequences after the user presses the capture hotkey even once: - -1. `app.on('window-all-closed', () => { if (process.platform !== 'darwin') app.quit() })` never fires on Windows/Linux, because the hidden capture window is still open after the main window closes. The app becomes an invisible zombie process with no window and no way to quit short of the task manager. -2. `app.on('activate', () => { if (BrowserWindow.getAllWindows().length === 0) createWindow() })` (main.js:434) sees length 1, so on macOS clicking the dock icon after closing the main window never recreates it. - -This is also why `setupSaveOnClose(mainWindow)` closing the last visible window does not lead to quit on Linux/Windows, contradicting the comment at main.js:83-87 which assumes `window-all-closed` calls `app.quit()`. - -**Fix:** Exclude the capture window from both checks — e.g. have createQuickCapture expose `getWindow()` / an `isCaptureWindow(win)` predicate and filter it out in `window-all-closed` and `activate`, or destroy the capture window when the last non-capture window closes. - -#### 4. Command serialization writes decrypted sensitive-note plaintext to sessionStorage - -`src/commands/ApplyNotesEditCommand.js`:25 - correctness - -`toJSON()` returns `{ ..., oldNotes: this.oldNotes, newNotes: this.newNotes }`. `useUndoRedo` (src/composables/useUndoRedo.ts:15-23) persists every pushed command with `JSON.stringify(serializeStack(stack))` into `sessionStorage['graphcore-undoStack']`, and re-hydrates it on reload. The same applies to `EditCommand.toJSON()` (oldValues/newValues always contain `notes`, because `pickNodeFields` copies all 28 NODE_UPDATE_FIELDS) and `DeleteCommand.toJSON()` (full `nodeData` including `notes`). - -Notes marked `notes_sensitive` are stored as `SNENC1:` ciphertext in the database (electron/database/index.js `_encodeNotesForWrite`) and are decrypted only into memory for the length of an unlocked session; docs/architecture/sensitive-notes.md states relock "clear[s] the sensitive-notes key from memory" and lists only in-memory plaintext as an honest limit. Editing or AI-rewriting a sensitive note therefore deposits its decrypted content in the renderer's sessionStorage backing store on disk, where it survives the idle relock and any later `sensitiveLock()`. - -**Fix:** Either exclude sensitive payloads from the persisted stack (e.g. have commands expose a `toJSON({ persist: true })` that omits note bodies and mark such commands non-restorable), or drop persistence for commands touching notes of a node with `notes_sensitive`. Whatever is chosen needs a test asserting no plaintext of a sensitive note reaches sessionStorage, and the honest-limits section of the doc updated. - -#### 5. MainToolbar drops the currentWorkspace prop, so JSON/CSV import always targets the 'work' workspace - -`src/components/MainToolbar.vue`:109 - correctness - -App.vue passes `:current-workspace="currentWorkspace"` (src/App.vue:892). MainToolbar declares no such prop and does not forward it to SettingsPanel, which declares `currentWorkspace: { type: String, default: 'work' }` (SettingsPanel.vue:44) and passes it down to DataSettings (SettingsPanel.vue:192). DataSettings uses it as the import target: `api.importJSON(data, null, props.currentWorkspace)` / `api.importCSV(text, null, props.currentWorkspace)` (DataSettings.vue:72, :74). With the prop dropped, importing while any non-'work' workspace is active silently writes every imported node into the 'work' workspace. - -**Fix:** Declare `currentWorkspace: { type: String, default: 'work' }` on MainToolbar and pass `:current-workspace="currentWorkspace"` through to . Cover with an integration test that imports while a second workspace is selected and asserts the nodes land in it. - -#### 6. deleteColumn drops a column definition without reindexing cells, shifting every column's data left - -`src/composables/useColumnOperations.js`:68 - correctness - -deleteColumn filters the column out of `column_definitions` and emits `structure-change`, and nothing anywhere touches the cells. DetailPanel.vue:602 just calls `updateTable(nodeId, { column_definitions })` and reloads; `electron/database/tables.js:70` only does `updates.push('column_definitions = ?')`. Cells are addressed positionally: NodeSpreadsheet.vue:116 builds each row with `cols.forEach((col, c) => row[col.name] = cellData.find(cl => cl.col_index === c)...)`. So after deleting the column at index 1 of [A,B,C,D], the remaining [A,C,D] render cell col_index 0,1,2 - column C now shows B's old data, D shows C's, and the cells that belonged to D are orphaned rows in the DB. The same absence of reindexing means re-adding a column resurrects stale values. - -**Fix:** Delete the removed column's cells and decrement col_index for all cells to the right in the same transaction as the column_definitions update (a dedicated `deleteTableColumn(nodeId, colIndex)` DB op), rather than emitting a bare column_definitions replacement. - -## Medium (91) - -#### 7. restoreBackup and reload replace ctx.db without closing the old handle or re-running migrations - -`electron/database/backup.js`:67 - correctness - -Both `restoreBackup` (line 67) and `reload` (line 83) do `ctx.db = new ctx.SQL.Database(buffer)` and drop the previous instance without calling `.close()`. sql.js allocates the database inside the WASM heap, so each restore/reload leaks the full previous database. Separately, neither re-runs `createTables`/`runMigrations` afterwards, so restoring a backup taken before a column migration (exactly the backups `runColumnMigrations` writes with the `-pre-workspace-migration` suffix, migrations.js:38) leaves the process running against a schema that lacks `workspace_id`, `tags`, `collapsed` and the `graph_*` columns — every subsequent query referencing them throws. - -**Fix:** Call `ctx.db.close()` before reassigning, and run the schema/migration pass on the newly loaded database (extract `_initSchema` into something callable from backup.js, or expose `ctx._initSchema()`). Add a test that restores a pre-workspace_id backup and then calls `getNodes({workspace_id: 'work'})`. - -#### 8. File header is not authenticated: a key slot can be stripped undetected - -`electron/database/encryption.js`:90 - correctness - -function encryptDatabase(plaintext, databaseKey, slots = []) { -const header = [Buffer.from(ENCRYPTION_MAGIC), Buffer.from([FORMAT_VERSION, slots.length])] -... -const cipher = crypto.createCipheriv('aes-256-gcm', databaseKey, nonce) - -No `cipher.setAAD(header)` is called, and `decryptDatabase` correspondingly does not `setAuthTag`-bind the header. The slot table (magic, version, slot count, per-slot type/length/blob) therefore sits outside GCM's integrity guarantee. An attacker with write access to `graph.db` can cleanly remove a whole slot — delete its `type|length|blob` bytes and decrement the count byte — and the file still decrypts successfully, because `payloadOffset` recomputes from the modified header. Removing the type-2 password slot silently destroys the documented recovery path ('This slot is the answer to keychain loss'), and `unlockWithPassword` then reports the honest-but-too-late `'This file has no password slot'`. - -docs/architecture/encryption.md claims more than the code delivers: 'GCM authenticates as well as encrypts: a tampered or corrupted file, and a wrong password, fail loudly at unwrap or decrypt rather than producing garbage data.' - -**Fix:** Pass the concatenated header as AAD on both sides (`cipher.setAAD(headerBytes)` in `encryptDatabase`, `decipher.setAAD(file.subarray(0, offset))` in `decryptDatabase`). This changes the on-disk contract, so bump `FORMAT_VERSION` to 2 and keep a v1 read path, or accept a one-time re-save. Update the doc's file-format section either way. - -#### 9. _save overwrites the whole database file in place, so a crash mid-write truncates it - -`electron/database/index.js`:262 - correctness - -`fs.writeFileSync(this.dbPath, this._serialize(Buffer.from(data)))` replaces the entire database file on every `_run`. `writeFileSync` truncates before writing, so an interruption between truncate and completion leaves a partial file. `_init`'s corrupt-file handling (lines 66-70) then preserves the wreckage and boots an empty database — recoverable, but the user's data is gone from the live file. Given `_save` fires on every single write, the exposure is continuous. - -**Fix:** Write to `${dbPath}.tmp` and `fs.renameSync` onto the target — rename is atomic within a filesystem. This is a small change fully contained in `_save` (and `backup`). - -#### 10. linkNodes does not check the reverse row, so a bidirectional link can be stored twice - -`electron/database/links.js`:45 - correctness - -The module docstring states a link is "stored as a single directed row (source_id -> target_id) ... treated as bidirectional on read and removal", and `unlinkNodes` and `getLinkedNodes` do honour both directions. But `linkNodes` inserts blindly and relies only on `UNIQUE(source_id, target_id)`, which does not cover the swapped pair. Verified: - -``` -link1 { success: true } // linkNodes(x, y) -link2 { success: true } // linkNodes(y, x) -allLinks [ {source_id:4,target_id:5}, {source_id:5,target_id:4} ] -``` - -Visible symptom: `getAllLinks` feeds `addLinkEdges` (useGraphElements.js:299), which keys edges by `link-${sourceId}-${targetId}`, so the pair renders as two separate edges in the graph. `unlinkNodes` then removes both at once, so the counts also disagree with what the user clicked. - -**Fix:** Make the insert direction-agnostic - check for an existing row in either direction before inserting (or normalise so the lower id is always `source_id`) - and return `{ success: true }` idempotently when the link already exists. - -#### 11. seedDefaultWorkspaces resurrects the 'work' and 'private' workspaces after the user deletes them - -`electron/database/migrations.js`:73 - correctness - -`seedDefaultWorkspaces` re-inserts `work` and `private` with `INSERT OR IGNORE` on every startup, unconditionally. Deleting a workspace is a supported UI action (`WorkspaceSelector.vue:173` -> `useWorkspace.deleteCurrentWorkspace` -> `api.deleteWorkspace`), and the only guard is that the workspace must have no root nodes. So a user who deletes `Work` sees it reappear at the next launch, and combined with `migrateUnassignedNodesToWork` any of its non-root/trashed nodes are pulled back into it. Seeding is a first-run concern, not a per-boot invariant. - -**Fix:** Seed only when the `workspaces` table is empty (or gate it behind the migration ledger from the version-tracking fix), rather than re-asserting the two ids on every startup. - -#### 12. No migration version tracking: every data migration re-runs on every startup and rewrites user data - -`electron/database/migrations.js`:97 - design - -`runMigrations` unconditionally calls all seven functions in `runDataMigrations` on each boot; there is no `schema_version` table, no applied-migrations ledger, and no `settings` marker. Each one re-scans `nodes` and, where its predicate matches, rewrites rows. Because the predicates describe a _state_ rather than a _version_, they undo deliberate user actions on the next launch: - -- `migrateShowLinksDefault` (line 112) runs `UPDATE nodes SET show_links = 1 WHERE show_links IS NULL` with no guard at all, so any node whose `show_links` was reset to NULL flips back to 1. -- `migrateUnassignedNodesToWork` (line 119) moves any node with `workspace_id IS NULL` into `'work'`. `deleteWorkspace` (workspaces.js:76) deliberately sets `workspace_id = NULL` on the deleted workspace's nodes, so the orphaning is silently reversed at the next restart. -- `migrateTasksProjectsStartDate` (line 136) re-populates `start_date` for any task/project whose start date the user cleared. -- `migrateOrganizationTextToLinks` (line 196) NULLs the `organization` column of every person node that has one. - The docs (docs/architecture/database.md:228) describe these as running "conditionally", which understates the behaviour. There is also zero test coverage: `grep -rn 'runMigrations|fixRootNodePaths|migrateUnassigned' src/__tests__ e2e` returns nothing. - -**Fix:** Add a `schema_migrations` table (or a `settings` key per migration id) written inside the same `_batch` as the migration body, and skip any migration already recorded. Backfill the ledger for existing installs by marking all current migrations as applied when the database already has the post-migration shape. Add integration tests that run each migration twice and assert the second run is a no-op. - -#### 13. moveNode has no ancestor guard: moving a node under its own descendant recurses forever - -`electron/database/nodes.js`:310 - correctness - -`moveNode` sets `parent_id` to whatever it is given and then calls `updateDescendantPaths(id)`. If `newParentId` is a descendant of `id`, the parent chain becomes cyclic and `updateDescendantPaths` walks A -> ... -> C -> A -> ... forever, each pass writing longer path strings. - -Verified against a real DB (A > B > C, then `db.moveNode(A.id, C.id)`): - -``` -moveNode THREW: RangeError Maximum call stack size exceeded -``` - -The `_batch` wrapper rolls the transaction back so the file is not corrupted, but the IPC call rejects with a stack-overflow RangeError instead of a meaningful error, and the same hole exists in `updateNode` (reparent branch, line 256) and `reorderNode` (cross-parent branch, line 375). - -This is reachable from the UI: `useTableDrag.js:200` and `useCardDrag.js:95` route an 'inside' drop to `onMove` -> `useNodeOperations.moveNode` (line 339), which calls `api.moveNode` with no ancestor check. Only `useGraphEvents.js:614` guards; the tree and cards drag paths do not. The invariant belongs in the layer that owns path/depth, not in each drag composable. - -**Fix:** Before writing, reject the move when `newParentId` is the node itself or lies in the node's subtree - e.g. read the target row with `getNodeRow(newParentId)` and check whether its `path` contains `id` as a segment - and throw a named error (or return null) instead of recursing. Add the same check to `updateNode`'s reparent branch and `reorderNode`'s cross-parent branch, and cover all three with tests. - -#### 14. decryptForRead has no error path; a stale-key note breaks every list query - -`electron/database/sensitiveSession.js`:106 - correctness - -function decryptForRead(value) { -if (!isEncryptedNote(value)) return value -if (!isUnlocked()) return value -touch() -return decryptNote(value, key) -} - -`decryptNote` throws (`'Wrong key or corrupted sensitive note'`) whenever the ciphertext was produced under a different sensitive-notes key. This is reachable: after the disable bug above strands a `SNENC1:` note, re-enabling sensitive notes generates a _fresh_ key, and the stranded note now fails to decrypt. `decryptForRead` is called from `Database._rowToNode`, which every node-returning query funnels through, so a single undecryptable row makes `getNodes`, `getChildren`, `getTree` and `search` throw for the whole result set — the tree simply fails to load, with the IPC promise rejecting rather than returning an error the UI understands. - -A session-level guard is also the only sensible place for it: `_rowToNode` cannot distinguish 'wrong key' from 'corrupt', and the module docstring already promises that a locked value 'is returned unchanged (as the ciphertext marker) ... so the renderer shows a locked placeholder'. - -**Fix:** Wrap the `decryptNote` call in try/catch and return `value` (the marker) on failure, matching the locked-session behaviour the docstring already describes. The renderer's `isLockedNote` then renders the locked placeholder instead of the whole query exploding. - -#### 15. setSettings performs one full-database serialize-and-write per key instead of using _batch - -`electron/database/settings.js`:58 - design - -`setSettings` loops over the entries calling `this.setSetting(key, value)`, each of which calls `ctx._run` -> `ctx._save()` -> `db.export()` + `fs.writeFileSync` of the entire database file. `index.js:_batch` (line 288) exists precisely to collapse this into a single write and to make the group atomic, and export.js:247/368, nodes.js and tree.js all use it. The real caller is the localStorage->database settings migration (`src/composables/useSettings.ts:375`), which passes every `graphcore-`-prefixed key at once — dozens of full-file writes on first launch, and a partial migration if any one of them throws. - -**Fix:** Wrap the loop: `return ctx._batch(() => { ... })`. Note `_save()` also re-runs the expensive `db.export()`, so the win is proportional to database size. - -#### 16. setCells uses INSERT OR REPLACE, so a partial cell update silently erases the columns it omits - -`electron/database/tables.js`:142 - correctness - -The JSDoc calls `setCells` an "upsert", but `INSERT OR REPLACE INTO node_table_cells (...)` deletes the conflicting row and inserts a fresh one, so every column the caller did not supply is reset. `useNodeTable.saveCell()` (src/composables/useNodeTable.js:117-130) builds `cellData` with only `row_index`, `col_index` and one of `value`/`formula` — no `style`. Line 139 then computes `styleJson = cell.style ? ... : null`, so the stored `style` becomes NULL. Concretely: bold a cell (`saveCellStyle` persists `{bold:true}`), then type a new value into it — the bold is gone. That `saveCellStyle` (line 154-172) goes out of its way to re-attach the existing `value` and `formula` shows the intended semantics are partial-update, and that `saveCell` simply forgot the mirror case. - -**Fix:** Make setCells a real upsert: `INSERT INTO node_table_cells (...) VALUES (...) ON CONFLICT(table_id, row_index, col_index) DO UPDATE SET ...`, updating only the keys present in the incoming cell object (use `'style' in cell` rather than truthiness). Wrap the loop in `ctx._batch(...)` so a mid-loop failure rolls back instead of leaving half the cells written. - -#### 17. getOrCreateTagNode writes ISO-8601 timestamps into a column every other write fills with SQLite CURRENT_TIMESTAMP - -`electron/database/tags.js`:61 - correctness - -`getOrCreateTagNode` builds its own INSERT and passes `new Date().toISOString()` for `created_at`/`updated_at`, while `createNode` and every UPDATE in the module group rely on the column default / `CURRENT_TIMESTAMP`. The two formats differ: - -``` -normal created_at= "2026-08-26 10:26:19" -tag created_at= "2026-08-26T10:26:19.253Z" -``` - -These are TEXT columns compared lexicographically. `'T'` (0x54) sorts after `' '` (0x20), so every tag node sorts ahead of every other node created the same day. Verified: - -``` -db.getRecent(5) -> [ 'mytag', 'Normal' ] // 'Normal' was created first -``` - -Every `ORDER BY updated_at DESC` query in search.js (`getRecent`, `getFavorites`, `getNodesByTag`) and tags.js (`getNodesLinkedToTag`) is affected, as is any `updated_at > ` comparison downstream. - -**Fix:** Drop the hand-rolled INSERT and create the tag through `ctx.createNode({ type: 'tag', title: trimmedName, workspace_id: workspaceId })`, or at minimum let the schema defaults supply `created_at`/`updated_at` so the column holds one format. - -#### 18. getNodesLinkedToTag and searchTagNodes are plumbed through IPC but no UI calls them - -`electron/database/tags.js`:83 - dead-code - -Both are bound in index.js (161-164), handled on `DB_GET_NODES_LINKED_TO_TAG` / `DB_SEARCH_TAG_NODES` (electron/ipc/database.js:168,170), bridged in preload.js:72-73, and implemented in `src/services/api.ts:852,856`. Grepping every `.vue`, `.ts` and `.js` under `src/` and `e2e/` outside the api definition files returns zero call sites for either name; `TagInput.vue` uses only `getTagNodes` and `getOrCreateTagNode`, and `useDataLoading.ts` only `getTagNodes`. - -`search.js` `getAllTags` (line 106) is dead in practice for the same reason: its only consumer is the fallback branch at `useDataLoading.ts:251`, guarded by `if (api.getTagNodes)` - and both api implementations define `getTagNodes` unconditionally (`src/services/api.ts:494` and `:845`), so the `else` can never run. The `?` optionality on `getTagNodes` in `src/types/api.ts:246` is what makes the branch look reachable. - -**Fix:** Delete `getNodesLinkedToTag` and `searchTagNodes` plus their channels/bridge/api entries, or expose them (e.g. a tag detail view and a tag autocomplete). Remove `getAllTags` and the unreachable legacy fallback in `useDataLoading.ts`, and drop the `?` from `getTagNodes` in `types/api.ts` so the type reflects reality. - -#### 19. restoreNode restores a node under a still-deleted parent, producing an invisible orphan - -`electron/database/tree.js`:124 - correctness - -```js -restoreNode(id) { - ctx._run('UPDATE nodes SET deleted_at = NULL WHERE id = ?', [id]) - return ctx.getNode(id) -} -``` - -No check that the parent is live, and no path/depth recomputation. When a child and then its parent are trashed, `deleteNode`'s reparent step skips the child (`AND deleted_at IS NULL`), so the child keeps pointing at the trashed parent. Restoring the child produces a live node whose parent is deleted - it appears in no root list and under no visible parent. Verified: - -``` -restored child parent_id 6 path "6" orphans [ 'CH' ] -``` - -It is only recoverable through the separate Lost & Found screen (`getOrphanedNodes` / `reparentToRoot`), which the user has no reason to visit after a restore. `emptyTrash` (line 133) already carries exactly this reasoning for its own survivors; `restoreNode` does not. - -**Fix:** Inside a `ctx._batch`, clear `deleted_at`, and if the node's `parent_id` names a row that is missing or still soft-deleted, either restore the ancestor chain or null the `parent_id`; then call `ctx._updateSubtreePath(id)` so path/depth match the restored position. - -#### 20. The agent loop is forked between main process and renderer, and the two have diverged - -`electron/ipc/agent.js` - design - -`electron/ipc/agent.js` and `src/services/agentService.js` are near-identical reimplementations of the same feature: both define `TOOL_GROUPS` with the same two entries, `getEnabledTools`, a tool executor (`executeAgentTool` / `executeTool`), `parseToolArgs`, `processToolCalls`, `runAgentLoop`, and the same 'No agent tools are enabled...' string. `api.ts` picks between them at runtime — line 683 dynamically imports `./agentService.js` for the web build, line 941 routes to the Electron IPC. - -They have already diverged in ways that matter: - -- Main has the `isGarbageResponse` / `fallbackResearch` path for models that cannot do tool calling; the renderer copy has none, so the web build silently produces garbage for those models. -- Max-iterations handling differs: main sends one more `chatRequest`; the renderer returns `null` and calls a separate `generateFinalSummary` that flattens the transcript into a `generate` prompt. -- `parseToolArgs` has different signatures in the two files (`parseToolArgs(toolCall)` vs `parseToolArgs(args)`) despite the same name. - -Only the shared prompt/tool data was factored out (`shared/agentConfig.json`); the control flow was copied. Per the project's own rule, a fork means every fix has to land twice, and here it already hasn't. - -**Fix:** Extract the loop into one transport-agnostic module parameterised by a `chat({messages, tools, ...}) => {content, tool_calls}` function and a tool-executor function, and have both `electron/ipc/agent.js` and `src/services/agentService.js` supply their own adapters. Add a test asserting the two entry points produce the same behaviour for the garbage-response and max-iterations cases. - -#### 21. DB_GET_ALL_SETTINGS hands the wrapped sensitive-notes key to the renderer; DB_SET/DELETE_SETTING let it destroy it - -`electron/ipc/database.js`:198 - design - -`electron/ipc/sensitiveNotes.js` persists the scrypt-wrapped sensitive-notes key as a plain settings row (`SETTINGS_KEY = 'sensitiveNotesWrappedKey'`, stored base64 via `db.setSetting`). `DB_GET_ALL_SETTINGS` is an unfiltered `db.getAllSettings()`, and `src/composables/useSettings.ts:58` caches the entire result in the renderer: - - settingsCache = await getElectronAPI()!.getAllSettings() - -This contradicts the stated design premise. `electron/database/sensitiveSession.js`'s header says 'The renderer never receives the key', and the whole point of the second layer is that a keychain-only database unlock must not expose sensitive content. Shipping the wrapped blob into renderer memory hands any renderer-side code execution (the app renders user markdown) the exact material needed for an offline scrypt attack on the recovery password. - -Symmetrically, `DB_SET_SETTING` / `DB_DELETE_SETTING` / `DB_SET_SETTINGS` are unguarded pass-throughs, so the renderer can overwrite or delete `sensitiveNotesWrappedKey` and permanently destroy the ability to decrypt every sensitive note, with no confirmation anywhere. - -**Fix:** Keep a small reserved-key denylist in the settings handlers: strip `sensitiveNotesWrappedKey` from `DB_GET_ALL_SETTINGS` / `DB_GET_SETTING` results, and reject writes and deletes to it from `DB_SET_SETTING`, `DB_SET_SETTINGS` and `DB_DELETE_SETTING`. Export the key name from `electron/ipc/sensitiveNotes.js` (it already exports `SENSITIVE_SETTINGS_KEY`) rather than re-literalling it. - -#### 22. No request timeout on either transport, so a hung LLM call wedges the IPC promise forever - -`electron/ipc/httpClient.js`:131 - correctness - -Neither `requestWithNode` nor `requestWithNet` sets a timeout or an abort path. Both return a Promise that settles only on `response.on('end')` or `request.on('error')`. A server that accepts the TCP connection and then stalls — a common failure mode for a local `ollama serve` mid-model-load, or an OpenAI-compatible proxy behind a dead upstream — leaves the promise pending indefinitely. That pending promise is the return value of `ipcMain.handle(OLLAMA_GENERATE ...)` / `OPENAI_GENERATE` / `AGENT_RESEARCH`, so the renderer's `await` never resolves and the UI spinner never clears with no way to cancel. I grepped `src/services/`, `electron/ipc/` and `electron/wikipedia.js`: there is no timeout, `AbortController` or `setTimeout` anywhere on this path. - -The agent loop compounds it — `runAgentLoop` issues up to `MAX_AGENT_ITERATIONS` sequential `chatRequest` calls, any one of which can hang the whole research action. - -**Fix:** Add a configurable timeout to `HttpClient` (default in the tens of seconds, longer for generate calls): `request.setTimeout(ms, () => { request.destroy(); reject(new Error(...)) })` on the Node path and the equivalent guard timer plus `request.abort()` on the `net` path. Cover it in src/**tests**/httpClient.test.js alongside the existing SSL-policy tests. - -#### 23. SENSITIVE_ENABLE never validates the password it wraps the key under - -`electron/ipc/sensitiveNotes.js`:43 - correctness - -The handler calls `session.enable(password)` with whatever string arrives, and `createSensitiveSession.enable` does no validation either — it just calls `wrapSensitiveKey(key, password)`. Two consequences: - -1. There is no check that the password is the _database recovery password_, even though the UI input is labelled `placeholder="Recovery password"`, the hint says 'Revealing them takes the recovery password', and docs/architecture/sensitive-notes.md states the feature 'reuses the recovery password'. A typo silently wraps the sensitive-notes key under a password the user does not know, and there is no confirm field. The notes then encrypt fine and become permanently unreadable — the failure only surfaces later, at the first unlock attempt. -2. There is no empty/blank rejection, unlike the sibling `keyManager.enable`, which starts with `if (!password || !password.trim()) throw new Error('A recovery password is required...')`. The UI's `:disabled="busy || !password"` is the only guard, and IPC is directly reachable. - -The handler already has the pieces available in principle — main.js constructs the `keyManager` and knows `dbPath` — but the sensitive-notes ctx is not given either. - -**Fix:** Add `verifyRecoveryPassword` (or `keyManager` + `dbPath`) to the sensitive-notes ctx in electron/main.js, and in SENSITIVE_ENABLE reject a blank password and confirm the supplied password unwraps the database password slot before generating and wrapping the sensitive-notes key. - -#### 24. Disable decrypt loop is not wrapped in db._batch: non-atomic and N full-file writes - -`electron/ipc/sensitiveNotes.js`:82 - correctness - -for (const { id } of encrypted) { -db.updateNode(id, { notes_sensitive: false }) -} - -Each `updateNode` bottoms out in `ctx._run`, which calls `this._save()` — a full `sql.js` `db.export()`, AES-256-GCM re-encryption of the entire database, and a whole-file write. For a user with N sensitive notes that is N complete database rewrites. `Database` already exposes `_batch(fn)`, which runs the body in one SQL transaction and defers to a single save (`electron/database/index.js:288`), and `deleteNode`/`updateNode`'s reparent branch already use it. - -Beyond cost, the loop is not atomic: if any `updateNode` throws partway the catch returns `{success:false}` with some notes decrypted and some not, and the caller has no way to know how far it got. - -**Fix:** Wrap the whole decrypt loop plus `db.deleteSetting(SETTINGS_KEY)` in `db._batch(() => { ... })` so it commits or rolls back as one transaction and writes the file once. - -#### 25. Comment claims the display-masking flag survives disable; the code clears it - -`electron/ipc/sensitiveNotes.js`:83 - docstring - -// Decrypt every sensitive note back to plaintext through the tested -// toggle-off path, then drop the key. The display-masking flag stays. -const encrypted = db._query("SELECT id FROM nodes WHERE notes LIKE 'SNENC1:%'") -for (const { id } of encrypted) { -db.updateNode(id, { notes_sensitive: false }) -} - -`notes_sensitive` IS the display-masking flag (docs/architecture/sensitive-notes.md: 'The `notes_sensitive` flag by itself is display masking'), and the loop explicitly sets it to false. My probe run confirmed `notes_sensitive: 0` on every touched row after the loop. So disabling the encryption layer also silently un-marks every note the user had marked sensitive — the opposite of what the comment promises, and a behaviour the docs never describe. - -**Fix:** Decide the intended behaviour and make code, comment and docs agree. If the flag is meant to survive, decrypt `notes` without touching `notes_sensitive`; if it is meant to be cleared, fix the comment and document the disable flow in docs/architecture/sensitive-notes.md. - -#### 26. CAPTURE_SET_CONFIG persists an accelerator before validating it, and drops the working hotkey on failure - -`electron/main.js`:352 - correctness - -```js -ipcMain.handle(CAPTURE_SET_CONFIG, (_event, { enabled, accelerator }) => { - db.setSetting('quickCaptureEnabled', enabled ? 'true' : 'false') - if (accelerator) db.setSetting('quickCaptureAccelerator', accelerator) - const ok = applyCaptureRegistration() - return { success: ok || !enabled, registered: ok } -}) -``` - -The new accelerator is written to settings before anything checks whether it can be registered. `applyCaptureRegistration` → `quickCapture.register()` first calls `unregister()` (quickCapture.js:67), releasing the accelerator that was working, and only then tries the new one. If the new accelerator is invalid or already owned by another app, `register()` returns false (or throws and is swallowed, quickCapture.js:73-76). - -Result of one bad entry in the settings field: the previously working hotkey is gone, the invalid string is persisted, and every subsequent app boot calls `applyCaptureRegistration()` with the bad value and silently fails — quick capture reports `enabled: true` in `CAPTURE_GET_CONFIG` while no hotkey exists. - -**Fix:** Try the registration first and only persist `quickCaptureAccelerator` when it succeeds; on failure re-register the previous accelerator and return `{ success: false, error: '...' }` so the UI can say which accelerator was rejected. - -#### 27. Two-phase quit save covers only the main window; detached windows lose unsaved edits - -`electron/main.js`:461 - correctness - -`before-quit` sends APP_BEFORE_QUIT to `mainWindow` only (main.js:461), and `setupSaveOnClose` is applied only to `mainWindow` (main.js:132). Detached windows created by `createDetachedWindow` (electron/ipc/window.js:82) get `setupExternalLinkHandling` but no save hook. - -A detached window mounts `DetachedView` (src/main.js:54-63), which renders `DetailPanel` — the same component whose debounced/pending edits `useAppLifecycle`'s onBeforeQuit handler exists to flush in the main window. The detached renderer never receives APP_BEFORE_QUIT (it isn't sent there) and `DetachedView` never registers `onBeforeQuit`, so pending edits in a detached node window are silently discarded on quit and on closing that window. - -Related hardening gap: the `APP_QUIT_SAVE_DONE` handler (main.js:427) is not scoped to a sender, so any renderer that acks would resolve the main window's pending save early. - -**Fix:** Run the handshake against every window that hosts an editor: call `setupSaveOnClose` in `createDetachedWindow`, fan APP_BEFORE_QUIT out to all such windows in `before-quit` and wait for all acks, and key `pendingSaveRequest` by `event.sender` so an ack only satisfies the window it came from. - -#### 28. onNotFound's fallback load to root is swallowed by useNavigation's own re-entrancy guard - -`src/App.vue`:427 - correctness - -App.vue's `onNotFound` handler calls back into the composable that is currently executing: - -```js -onNotFound: async () => { - currentContainerId.value = null - localStorage.removeItem(STORAGE_KEYS.CONTAINER_ID) - await navigation.loadChildren(null) -}, -``` - -In `src/composables/useNavigation.ts`, `loadChildren` sets the module-scoped `isLoadingChildren = true` (line 213) and only clears it in its `finally` block (line 303). `onNotFound` is invoked from inside the `catch` block (line 292), i.e. _before_ that `finally` runs. The nested `loadChildren(null)` therefore hits the guard at line 204-206 (`if (isLoadingChildren) return`) and returns immediately without loading anything. The `await` resolves instantly and gives the false impression the recovery happened. - -Net effect: the 404 recovery path is a no-op. `currentContainerId` is nulled, the persisted container id is dropped, but `children`/`breadcrumbs`/`currentContainer` are never refreshed, so the view keeps whatever stale (or empty) content it had and the user is stranded with no error surfaced (`handleError` for that path is called with `silent: true`). - -**Fix:** Do not re-enter `loadChildren` synchronously from inside its own catch. Either defer the call (`await nextTick()` / `queueMicrotask` before `navigation.loadChildren(null)`), or better, have `useNavigation` clear `isLoadingChildren` before invoking the `onNotFound`/`onError` callbacks so the recovery load is not blocked by the guard. Add a regression test that makes the first load reject with a 404 and asserts root children are actually loaded. - -#### 29. Undo of a completion leaves the auto-set end_date behind - -`src/commands/CompleteCommand.js`:18 - correctness - -`useNodeOperations.toggleComplete` (src/composables/useNodeOperations.ts:389-395) writes two fields when completing: `{ completed: true, end_date: }` if the node had no `end_date`. `CompleteCommand` records only `oldCompleted`/`newCompleted`, and `undo()` writes back `{ completed: this.oldCompleted }` only. After marking a task done and pressing undo, the node is uncompleted but keeps an `end_date`, which timeline/date rendering and later exports then show as a completion date for an open task. Redo is asymmetric in the other direction: `execute()` never re-applies the `end_date` the original action set. - -**Fix:** Capture `oldEndDate`/`newEndDate` in the command (as `EditCommand` does with full snapshots) and restore both fields in `undo()`/`execute()`; add a test asserting `end_date` returns to its pre-completion value. - -#### 30. Redo of a create mints a new node id, silently orphaning later commands in the redo stack - -`src/commands/CreateCommand.js`:21 - correctness - -`execute()` re-creates the node and rebinds `this.nodeId = created.id`, but every other command already on the redo stack still holds the old id. Sequence: create node (id 5) -> edit its title (EditCommand{nodeId:5}) -> Ctrl+Z twice (edit undone, node 5 hard-deleted) -> Ctrl+Shift+Z twice. The create redo yields id 9; the edit redo calls `api.updateNode(5, ...)`, whose SQL matches zero rows and whose `getNode(5)` returns null (electron/database/nodes.js:220-260 does not error on a missing id). The edit is silently lost, and the node position saved under the old id by `saveNodePosition` (App.vue:277) is likewise stranded. - -**Fix:** Have the undo stack remap ids after a create redo (e.g. `useUndoRedo` broadcasting an `idChanged(oldId, newId)` that commands apply to their stored ids), or make `undo()` soft-delete plus `restoreNode` so the id is stable. At minimum, add a regression test for undo-past-create-then-redo. - -#### 31. Redo deletes parents before children, flattening the trashed subtree's parent_id - -`src/commands/DeleteMultipleCommand.js`:12 - correctness - -`execute()` iterates `this.nodes` in stored order. Both producers build that array parents-first (`useNodeOperations.ts:265` `[node, ...descendants]`, and `:296-316` roots then descendants), while the original delete deliberately runs deepest-first (`:268` `[...descendants].reverse()`, `:319` sort by descending depth) with the comment "children first to maintain integrity". - -`db.deleteNode` reparents live children up to the deleted node's parent (`UPDATE nodes SET parent_id = ? WHERE parent_id = ? AND deleted_at IS NULL`). Deleting deepest-first never triggers that (children are already soft-deleted); deleting parents-first does. So a redo of a subtree delete leaves every trashed descendant pointing at the grandparent instead of its real parent. Undo repairs it via the recorded `parent_id`, but restoring those rows from the Trash view instead (useDataLoading.ts:277 -> `api.restoreNode`) then reattaches them to the wrong parent. - -**Fix:** Sort by descending `depth` inside `execute()` exactly as `deleteMultipleNodes` does (or have both call one shared helper), and add a test that redo leaves each trashed node's `parent_id` unchanged. - -#### 32. AppSidebar forks useSidebar's section-collapse state instead of consuming it - -`src/components/AppSidebar.vue`:39 - design - -AppSidebar re-implements collapse state locally: - -```js -const treeCollapsed = ref(localStorage.getItem('sidebar-tree-collapsed') === 'true') -const favoritesCollapsed = ref(localStorage.getItem('sidebar-favorites-collapsed') === 'true') -const tagsCollapsed = ref(localStorage.getItem('sidebar-tags-collapsed') === 'true') -watch(treeCollapsed, val => localStorage.setItem('sidebar-tree-collapsed', String(val))) -``` - -useSidebar.ts already owns exactly this: the same three localStorage keys (useSidebar.ts:50-53), the same refs (`treeCollapsed`, `favoritesCollapsed`, `recentCollapsed`, `tagsCollapsed`, lines 146-149), the same persistence watchers (lines 152-170), plus `toggleTreeCollapse` / `toggleFavoritesCollapse` / `toggleRecentCollapse` / `toggleTagsCollapse` (lines 224-250) and documented interface entries (lines 20-45). App.vue's `useSidebar(...)` destructure (lines 134-142) takes only `hovered`, `expandedIds`, `visible`, `onEnter`, `onLeave`, `toggleExpand`, `expandToPath` — none of the collapse API. Grepping src/ and e2e/ for `toggleTreeCollapse|toggleFavoritesCollapse|toggleRecentCollapse|toggleTagsCollapse|recentCollapsed` returns hits only inside useSidebar.ts and src/**tests**/useSidebar.test.js. So roughly 60 lines of composable code plus its documented public interface are dead, kept green only by their own unit test, while the component duplicates the logic. `recentCollapsed` / `toggleRecentCollapse` are doubly dead: AppSidebar has no Recent section at all. This is precisely the fork the project rules forbid — a fix to the persistence logic would now have to land twice. - -**Fix:** Delete the local refs and watchers from AppSidebar; pass the collapse refs (or toggle callbacks) down from App.vue's `useSidebar()` instance as props/events, or make AppSidebar call `useSidebar()` itself. Remove `recentCollapsed`/`toggleRecentCollapse` and their tests since no Recent section exists. - -#### 33. Quick-capture save swallows failures into an unhandled rejection with no user feedback - -`src/components/CaptureView.vue`:22 - correctness - -`save()` wraps the API call in `try { ... } finally { busy.value = false }` with no `catch`: - -```js -try { - await api.createNode({ type: 'note', title, parent_id: null, workspace_id: ... }) - text.value = '' - hide() -} finally { - busy.value = false -} -``` - -If `api.createNode` rejects (database locked, encrypted DB not unlocked, IPC error — all reachable states in this app), the rejection propagates out of the async handler bound to `@submit.prevent="save"`, becoming an unhandled promise rejection. The capture window stays open with the text intact, but nothing tells the user the note was not saved, and nothing is logged through the project's error channel. Every sibling view in this group that touches the API (PersonsView) routes failures through `useErrorHandler`; this one does not import it. The test file src/**tests**/captureView.test.js covers only success, empty submit and Escape — there is no red test for the failure path. - -**Fix:** Add a `catch` that reports through `useErrorHandler` (or renders an inline error state in the capture window) and keeps the window open, and add a test asserting the window is not hidden and the text is retained when `createNode` rejects. - -#### 34. select-multiple emits a bare node; the handler destructures an options object, so shift+click does nothing - -`src/components/CardsView.vue`:95 - correctness - -```js -} else if (e.shiftKey) { - emit('select-multiple', node) -} -``` - -The payload is a plain Node. ViewRenderer forwards `$event` verbatim (line 182) to `App.vue:962 @select-multiple="handleMultiSelect"`, and `useSelection.ts:185` is `function handleMultiSelect({ node, nodes, nodeIds, add, range })`. Destructuring a Node yields `node === undefined`, so the `if (!node) return` guard on line 194 fires and the selection never changes. Every other emitter uses the wrapper shape: `TableView.vue:75` emits `{ node, ...opts }`, `useGraphEvents.js:217/291/428` emit `{ node, add: true }` / `{ nodes, nodeIds }`. Shift+click multi-select is silently inert in cards view. - -**Fix:** Emit `{ node, range: true }` (and `{ node, add: true }` for the Cmd path if toggle-select is wanted), matching TableView and the graph. - -#### 35. Date-only strings parsed as UTC then compared against local midnight (off-by-one day) - -`src/components/CardsView.vue`:112 - correctness - -`getDateCountdown` (112), `formatDueDate` (135), `getDueDateClass` (149) and `getDueDateStatus` (164) each do: - -```js -const target = new Date(targetDate) -target.setHours(0, 0, 0, 0) -``` - -`node.due_date` / `start_date` / `end_date` are date-only strings (`YYYY-MM-DD`, written as `new Date().toISOString().split('T')[0]` in `useNodeOperations.ts:392`). `new Date('2026-08-26')` is parsed as UTC midnight; in any negative UTC offset that is 2026-08-25 locally, and `setHours(0,0,0,0)` then snaps it to the previous calendar day. A task due today shows as "1d overdue" and gets the `overdue` class. - -`src/utils/formatting.js` already contains the correct shared implementation for exactly this (`parseDateLocal` line 24, with the comment "Date-only strings are parsed as local dates, not UTC midnight", and `daysFromToday` line 40). `config/tableFormatters.js` delegates to it; CardsView reimplements the arithmetic four times instead. - -**Fix:** Replace all four helpers' arithmetic with `daysFromToday(node.due_date)` from `../utils/formatting.js`. - -#### 36. Child-card notes swallow the click and emit startEdit into the void - -`src/components/CardsView.vue`:481 - correctness - -The child-card `` is mounted with only `:notes`, `:sensitive` and `size`; no `:model-value`, no `:is-editing`, and no `@start-edit` / `@save` / `@cancel` listeners. But `CardNotes.vue:22` unconditionally binds - -```html -@click.stop="!sensitive && $emit('startEdit')" -``` - -So clicking a child card's notes emits `startEdit` with no parent handler, and the `.stop` modifier prevents the click from reaching the child card's own `@click.stop="handleChildCardClick(...)"` (line 445). The result is a region of the child card where clicking selects nothing and edits nothing. - -**Fix:** Either wire the child instance to the same edit handlers used by the top-level card (line 333), or add a prop such as `readonly` to CardNotes that suppresses both the emit and the `.stop`. - -#### 37. DetailPanel affordances are inert in the detached window (detach unhandled, three no-op handlers) - -`src/components/DetachedView.vue`:268 - correctness - -DetailPanel declares a 'detach' emit and renders the detach button whenever `isElectron` is true (DetailPanel.vue:672-676) — which is true inside the detached window too. DetachedView binds no @detach handler at all, so that button does nothing. Three further events are bound to empty arrows: @toggle-fullscreen="() => {}", @toggle-pin="() => {}" and @open-link-search="() => {}". The pin button is therefore visible and dead, and the links section's "add link" control (DetailPanel.vue:945 `@add-link="emit('open-link-search')"`) silently does nothing in a detached window — a user-visible action that fails without any feedback. - -**Fix:** Hide the affordances that cannot work in a detached window (pass a prop that suppresses the detach/pin/fullscreen buttons and the add-link control), or implement the handlers: reuse the link-search flow, and either ignore or forward 'detach' deliberately. - -#### 38. props.node watcher discards unsaved edits whenever the same node object is replaced - -`src/components/DetailPanel.vue`:290 - correctness - -The watcher does `editedNode.value = { ...newNode }` unconditionally on every `props.node` change, and only the _UI reset_ block is guarded by `isNewNode`. Several code paths replace `selectedNode` with a freshly fetched object for the SAME id while the panel is open: `useRefresh.js:81` (`selectedNode.value = await api.getNode(selectedNode.value.id)` after linking), `useAppLifecycle.js:58` (`selectedNode.value = { ...data.node }` on a detached-window broadcast), `useNodeActionsUI.ts:276`. Notes autosave is only 500 ms (`AUTOSAVE_DELAY_MS`), so the round-trip fires constantly while typing. Any keystrokes entered between the fetch and the assignment are silently reverted, and with the same node open in a main and a detached window, the remote copy overwrites what the user is typing. - -**Fix:** Only rehydrate editedNode when `newNode.id !== oldNode?.id`; for a same-id refresh, merge non-edited fields (or skip while the notes editor has focus / a save is pending), and cover it with a vitest that mutates editedNode then re-assigns props.node with the same id. - -#### 39. changeWorkspace has no error handling, unlike every other async handler in the file - -`src/components/DetailPanel.vue`:464 - correctness - -`async function changeWorkspace(newWorkspaceId) { editedNode.value.workspace_id = newWorkspaceId; await api.updateNode(...); emit('update', editedNode.value) }`. Every other async function here wraps its api call in try/catch + `handleError` (loadChildren, loadLinkedNodes, removeLink, exportMarkdown/JSON/CSV, toggleChildComplete, onChildReorder). A failed workspace move rejects into the DOM change handler as an unhandled rejection: the select visually shows the new workspace, the node never moved, and nothing is reported. It also emits the reactive proxy (`editedNode.value`) while `saveChanges()` at line 406 deliberately unwraps with `toRaw` for the same emit. - -**Fix:** Wrap the call in try/catch with `handleError(err, { context: 'Changing workspace' })`, revert `editedNode.value.workspace_id` on failure, and emit `toRaw(editedNode.value)` for consistency. - -#### 40. "Wrap with Parent" uses window.prompt(), which Electron does not implement - -`src/components/DetailPanel.vue`:475 - correctness - -`function wrapWithParent() { const title = prompt('New parent title:'); if (title) { emit('wrap-with-parent', ...) } }`. Chromium in Electron does not implement `window.prompt()` — it logs "prompt() is and will not be supported" and returns undefined — so the `if (title)` branch never runs and the button is inert in the packaged app. This is the only remaining `prompt()` call in `src/` (grep confirms: App.vue, useDemoWorkspace, useTagActions, useDataLoading, useNodeActionsUI, NodeSpreadsheet, WorkspaceSelector all use `confirm()`, which IS supported). The project already built the replacement: `src/composables/useGraphModals.js:20` declares `promptModal` with the comment "Prompt modal state (replacement for native prompt())". - -**Fix:** Route wrapWithParent through the existing prompt modal in useGraphModals (or emit `wrap-with-parent` with no title and let App open the modal), and add a test that asserts the emit fires. - -#### 41. Locked sensitive notes are editable and toggleable; the write fails silently - -`src/components/DetailPanel.vue`:796 - correctness - -`notesLocked` (line 66) is consulted only in the two _preview_ branches (815, 867). The Edit tab (``, line 796) and the split-view editor (842) render the raw `SNENC1:...` ciphertext in an editable CodeMirror, and every keystroke schedules `saveChanges()` 500 ms later. `toggleNotesSensitive()` (454) is likewise unguarded. `electron/database/index.js:_encodeNotesForWrite` throws "Sensitive notes are locked" for exactly these cases; `saveChanges()` only `emit('update', ...)`, and `useNodeActionsUI.ts:409` rethrows into a floating promise, so the user gets no feedback at all. docs/architecture/sensitive-notes.md:51 states "Turning the flag off, or editing a sensitive note, requires an unlocked session" — the renderer does not enforce the documented rule. - -**Fix:** Force `activeTab` to 'preview' (or render the unlock form in place of the editor) while `notesLocked` is true, disable the sensitive toggle when locked, and surface the rejected update through useErrorHandler instead of dropping it. - -#### 42. Radial sliders write to SQLite and localStorage on every input event, while the Apply button implies deferred application - -`src/components/GraphControls.vue`:171 - design - -Each of the five range inputs fires on `@input` with a whole new object: `emit('update:radialSettings', { ...radialSettings, nodeRepulsion: Number($event.target.value) })`. GraphView binds `@update:radial-settings="radialSettings = $event"`, and its deep watcher (GraphView.vue:421-428) then runs `Object.assign(_radialSettings, v)` — which triggers seven localStorage writes in useGraphSettings — plus `saveNodeSetting(..., 'graph_physics', JSON.stringify(v), ...)`, i.e. one api.updateNode IPC round trip and SQLite write per slider tick. A single drag produces dozens of DB writes. The dropdown also has an explicit "Apply" button (224), so the UI promises that changes are applied on demand while the value is in fact persisted continuously. - -**Fix:** Emit on `@change` instead of `@input` (or debounce the persistence watcher in GraphView), and make the Apply button the only thing that runs the layout so the control's contract matches its behaviour. - -#### 43. No test covers any component in this group - -`src/components/GraphView.vue` - design - -src/**tests** contains tests for the graph composables (useGraphEvents, useGraphLayout, useGraphSettings) but grepping src/**tests** and e2e/ for GraphView, GraphControls, GraphEditModal, GraphPromptModal, ViewRenderer or DetachedView returns nothing. The behaviour that lives in the components rather than the composables is therefore untested: the settings-persistence watcher matrix (the root-vs-container invariant broken above), the parent-change resync, the emit/prop contract between GraphView, ViewRenderer and App.vue, and the whole detached-window flow. Per the project rule a feature is done only when it is reachable from the UI and covered by tests. - -**Fix:** Add component tests for the GraphView settings watchers (root vs container writes) and the ViewRenderer event forwarding contract, plus an e2e case for the detached window; prove each red against the current code first. - -#### 44. relaxLocked / fitLocked / radialSettings watchers overwrite the workspace defaults with a container's values - -`src/components/GraphView.vue`:413 - correctness - -The block comment at lines 359-365 states the invariant explicitly: "These watchers must only write the workspace default (_xxx) when at root level. Inside a container they persist to that node. Otherwise the act of syncing a container's saved value (via the parent-change watcher below) would overwrite the workspace/home default with the container's value." layoutMode, showRootNode, showExternalLinks, maxDepth and visibleTypes all honour it with an `if (props.parent?.id) ... else ...`. The next three watchers do not: - -watch(relaxLocked, v => { -_relaxLocked.value = v -saveNodeSetting(props.parent?.id, 'graph_relax_locked', v ? 1 : 0, 'relax locked') -}) -watch(fitLocked, v => { _fitLocked.value = v; saveNodeSetting(props.parent?.id, 'graph_fit_locked', ...) }) -watch(radialSettings, v => { Object.assign(_radialSettings, v); saveNodeSetting(props.parent?.id, 'graph_physics', JSON.stringify(v), ...) }, { deep: true }) - -They write the workspace default unconditionally, and the parent-change watcher at lines 463-469 assigns exactly these three from the container node (`relaxLocked.value = Boolean(props.parent.graph_relax_locked)` etc.), which fires them. So merely navigating into a container overwrites the workspace-wide localStorage defaults (GRAPH_RELAX_LOCKED-, GRAPH_FIT_LOCKED-, the seven GRAPH_RADIAL_* keys) with that container's values — the exact failure the comment above forbids. - -**Fix:** Mirror the layoutMode/showRootNode pattern: `if (props.parent?.id) saveNodeSetting(...) else { _relaxLocked.value = v }`, and the same for fitLocked and radialSettings (Object.assign(_radialSettings, v) only at root). - -#### 45. initGraph is async with no re-entrancy guard; concurrent calls orphan a cytoscape instance - -`src/components/GraphView.vue`:604 - correctness - -initGraph() awaits graphUpdate.buildElementsWithLinks() before assigning `cy = graphInit.createCytoscapeInstance(...)`. Every caller guards with `if (cy) { cy.destroy(); cy = null }` first, which means a second caller during the await window sees cy === null and starts a second init. A workspace switch at root level does exactly this: watch(() => props.workspace) at 476 reassigns showExternalLinks/showRootNode, whose watchers at 504/512 destroy cy and call initGraph(), while the second workspace watcher at 652 sees cy === null (the first init has not resolved yet) and calls initGraph() again. Both resolve, both call cytoscape({container}), and the first instance is never destroyed — its canvases stay in the container DOM and its event handlers stay live, so the view shows duplicated rendering and leaks an instance per switch. The existing `isInitializing` flag is set but never checked by initGraph itself. - -**Fix:** Make initGraph re-entrancy safe: keep a token/generation counter, and after the await discard the result if a newer init started (or destroy any `cy` that exists before assigning the new instance). - -#### 46. MainToolbar drops the openaiSkipSslVerification v-model, so the SSL-skip toggle is inert - -`src/components/MainToolbar.vue`:106 - correctness - -App.vue binds `v-model:openai-skip-ssl-verification="openaiSkipSslVerification"` on (src/App.vue:882). MainToolbar declares no `openaiSkipSslVerification` prop (props end at line 109 with `ollamaEnabled`), no `update:openaiSkipSslVerification` emit (list ends line 146), and does not forward it to (lines 244-292). SettingsPanel _does_ declare the prop and emit (SettingsPanel.vue:40, :64) and forwards them to AISettings, which renders the actual checkbox (AISettings.vue:286-287). MainToolbar is the only mount point of SettingsPanel in the whole renderer. Net effect: the prop falls through as a raw HTML attribute onto `
`, SettingsPanel always receives the default `false`, and the user's toggle emit dies on the toolbar div. The checkbox appears unchecked on every open and the setting is never persisted, while `useAiNotes` reads `openaiSkipSslVerification` from useSettings and will use the stale stored value. - -**Fix:** Add `openaiSkipSslVerification: { type: Boolean, default: false }` to the props, `'update:openaiSkipSslVerification'` to the emits, and `:openai-skip-ssl-verification="openaiSkipSslVerification"` / `@update:openai-skip-ssl-verification="emit('update:openaiSkipSslVerification', $event)"` to the SettingsPanel binding. Then add a gate test that asserts MainToolbar forwards every prop and emit SettingsPanel declares, so this class of drop fails CI instead of shipping. - -#### 47. MainToolbar never forwards SettingsPanel's import-complete event, so the tree is not refreshed after an import - -`src/components/MainToolbar.vue`:146 - correctness - -App.vue listens `@import-complete="loadChildren()"` on (src/App.vue:904). MainToolbar's emits array (lines 111-146) does not include `'import-complete'`, and the binding (lines 265-291) has no `@import-complete` handler, even though SettingsPanel declares and re-emits it (SettingsPanel.vue:75, :200) from DataSettings (DataSettings.vue:24, :77). The event stops at SettingsPanel; App's `loadChildren()` never runs, so imported nodes do not appear until a manual reload or navigation. - -**Fix:** Add `'import-complete'` to MainToolbar's emits and `@import-complete="emit('import-complete', $event)"` to the SettingsPanel binding. - -#### 48. processMathFormulas rewrites the raw markdown, so $ inside code fences and plain currency text become KaTeX - -`src/components/MarkdownRenderer.vue`:70 - correctness - -`renderContent` calls `processMathFormulas(props.content)` on the raw markdown _before_ `marked.parse` (lines 200-203). The regexes at lines 72 and 84 are applied to the whole document with no awareness of fenced or inline code, and no awareness of escaped `\$`. Two concrete failures: (1) a ```python fence containing `cost = $x$` has its content replaced by KaTeX `` markup, which marked then escapes, so the reader sees raw KaTeX HTML as literal text in the code block — this contradicts the file's own comment at lines 202-204 claiming code fences render literally; (2) prose like `it costs $5 and $10 more` matches the inline rule and renders `5 and ` as a math formula. The file's own test suite (src/**tests**/markdown.test.js) has no math coverage at all, so neither case is gated. - -**Fix:** Tokenize math with a `marked` inline/block extension (the same mechanism already used for personMention at lines 36-52), so code spans and fences are structurally excluded, instead of regexing the source. Require `\$` escaping or a digit-adjacency guard for the inline rule. Add tests for a $ inside a fence and for currency prose. - -#### 49. Mermaid error path writes decoded user content straight into innerHTML, bypassing DOMPurify - -`src/components/MarkdownRenderer.vue`:187 - correctness - -`renderMermaidBlocks` catch branch does `el.innerHTML = `
${block.code}\n\nError: ${e.message}
``. `block.code` is `decodeHtml(code.trim())` (line 167), i.e. the user's note text with HTML entities already decoded, and it is injected as raw markup without `sanitizeHtml` and without `escapeHtml`. Every other HTML path in this file goes through `sanitizeHtml` (line 212) or `escapeHtml` (line 49). A note containing a ```mermaid fence with `` fails to parse (so the catch branch runs) and the payload executes in the renderer. Notes are user- and AI-generated content and can arrive via JSON/CSV import, so this is reachable without the author's cooperation. The success path (`el.innerHTML = svg`, line 181) also bypasses `sanitizeHtml` and relies solely on mermaid's internal sanitizer. - -**Fix:** Build the error node with `document.createElement('pre')` + `textContent`, or wrap the string in `escapeHtml(block.code)` / `sanitizeHtml(...)`. Run the mermaid SVG through `sanitizeHtml` too, and add a test in htmlSanitization.test.js that feeds a malformed mermaid fence containing markup and asserts no live element is created. - -#### 50. No test covers NodeContextMenu, its five sub-components, SpotlightSearch, AddNodeModal, OnboardingModal or KeyboardShortcutsModal - -`src/components/NodeContextMenu.vue` - correctness - -Grepping `src/__tests__/` and `e2e/` for these component names returns no component-level test at all. The only hits are `showAddNodeModal` as a mocked callback in `useGraphEvents.test.js` / `viewShortcuts.test.js` (which tests the composable, not the modal) and a single mention of onboarding in `e2e/helpers.js:38`, which only dismisses the dialog to get it out of the way. `UnlockScreen.vue` is the sole file in this group with a real test (`src/__tests__/securityUi.test.js`). - -The project rule is explicit: a feature is done when it is usable from the UI and covered by tests. This is the entire menu/modal surface of the app -- the emit contracts (13 emits on NodeContextMenu, 8 on SpotlightSearch), the `visible !== false` filtering, the Escape/keydown handling, and the document-listener add/remove lifecycles are all unverified. The Escape-propagation and recents-navigation defects above would each have been caught by a mount test. - -**Fix:** Add vitest component tests: NodeContextMenu emit payloads and per-type item visibility (`person` hides Add Child / Mark Complete), MenuGroup/MenuItem visibility filtering, SpotlightSearch's results vs recents vs empty branches and the `load-more` scroll threshold, AddNodeModal's Enter-creates-task path and title reset, and the two modals' document keydown listener attach/detach across `visible` transitions and unmount. - -#### 51. `menuRef` is declared and bound but never read - -`src/components/NodeContextMenu.vue`:24 - dead-code - -`const menuRef = ref(null)` (line 24) is bound in the template (`ref="menuRef"`, line 187) but grepping the whole tree (src/, electron/, e2e/, src/**tests**/) finds only those two occurrences -- nothing ever reads `menuRef.value`. - -It is dead in a way that also hides a real defect: `menuStyle` calls `calculateMenuPosition(props.x, props.y)` with no dimensions, so the util falls back to its `menuWidth = 260, menuHeight = 520` defaults. The actual menu height varies substantially with the number of linked nodes (`LinkedItemsList`) and workspaces (`WorkspaceList`), so with many linked items or workspaces the menu overflows the viewport bottom without flipping, and with a short menu it flips too eagerly. The measurement that `menuRef` was evidently added for was never wired. - -**Fix:** Either delete `menuRef` and the template binding, or wire it in: measure the rendered menu with `menuRef.value.getBoundingClientRect()` after it becomes visible and pass the real width/height to `calculateMenuPosition`. Do not leave the ref sitting unused. - -#### 52. AG Grid theme hardcodes a black palette while the app supports a light theme - -`src/components/NodeSpreadsheet.vue`:27 - consistency - -`COLORS` hardcodes `bg: '#000000'`, `bgHeader: '#080808'`, `bgOddRow: '#040404'`, `text: '#d0d0d0'`, `border: '#1a1a1a'`, and these are baked into `themeQuartz.withParams(...)` (line 55) and bound unconditionally via `:theme="darkTheme"` (line 569). - -The app has a real light theme: `src/themes/light.css` defines `[data-theme='light'] { --bg-primary: #ffffff; ... }` and `useTheme.js:34` sets `document.documentElement.setAttribute('data-theme', resolved)`. `NodeSpreadsheet.css` itself uses `var(--bg-secondary)`, `var(--text-tertiary)`, `var(--accent-subtle)` in 52 places, including the `:deep()` overrides for the row-index column and selection. So in light mode the grid body and headers stay black inside an otherwise white panel, and the row-index cells (which do follow the token) turn white against it. - -**Fix:** Read the theme tokens at runtime (e.g. `getComputedStyle(document.documentElement).getPropertyValue('--bg-primary')`) and rebuild the `themeQuartz` params in a computed that reacts to the active theme, or drive the grid entirely from the CSS variables already in NodeSpreadsheet.css. - -#### 53. "Skip SSL verification" toggle is inert: prop and update event are not wired through MainToolbar - -`src/components/SettingsPanel.vue`:174 - correctness - -AISettings renders the SSL checkbox (`AISettings.vue:284-297`) and emits `update:openaiSkipSslVerification`; SettingsPanel forwards it upward (SettingsPanel.vue:64, 164, 174). But MainToolbar — the only production parent of SettingsPanel — neither declares `openaiSkipSslVerification` in its props (MainToolbar.vue:76-107) nor in its emits (MainToolbar.vue:109-141), and does not bind or listen for it on `` (MainToolbar.vue:242-292). - -Consequence: `App.vue:882`'s `v-model:openai-skip-ssl-verification="openaiSkipSslVerification"` binds to a component that ignores it, so (a) the checkbox always shows the default `false` regardless of the persisted setting, and (b) toggling it never reaches `useSettings`, so the setting can never be changed from the UI. This is exactly the "emitted event no parent handles / prop never passed" case — a shipped, security-relevant control that does nothing. - -**Fix:** Add `openaiSkipSslVerification` to MainToolbar's props and emits and forward both the binding and `@update:openai-skip-ssl-verification` to SettingsPanel; add a mount test asserting the emit reaches App's setting. - -#### 54. import-complete never reaches App, so the tree is not reloaded after an import - -`src/components/SettingsPanel.vue`:200 - correctness - -DataSettings emits `import-complete` with the import result (DataSettings.vue:77); SettingsPanel re-emits it (SettingsPanel.vue:75, 200). MainToolbar does not declare `import-complete` in its emits and places no `@import-complete` listener on ``, so `App.vue:904`'s `@import-complete="loadChildren()"` never fires. After a JSON/CSV import the success toast appears but the visible tree still shows the pre-import state until the user navigates or reloads — the imported nodes look lost. - -**Fix:** Declare `import-complete` in MainToolbar's emits and forward it: `@import-complete="emit('import-complete', $event)"`. - -#### 55. Tooltip checkbox emits a node id where every consumer expects a node object - -`src/components/TableView.vue`:60 - correctness - -TableView emits `toggle-complete` with two incompatible payloads. The row checkbox emits the node object (`@click.stop="emit('toggle-complete', row.node)"`, line 280), but the tooltip callback emits a bare number: - -```js -onToggleComplete: nodeId => emit('toggle-complete', nodeId), -``` - -`useNodeTooltip.js:73` calls `onToggleComplete(parseInt(evt.target.dataset.nodeId))`, i.e. a number. ViewRenderer forwards `$event` unchanged to `App.vue:964 @toggle-complete="toggleComplete"`, which reaches `useNodeOperations.toggleComplete(node: Node)`; that function reads `node.completed`, `node.end_date` and calls `api.updateNode(node.id, updates)`. With a number, `node.completed` is `undefined` (so `newCompleted` is always `true`) and `node.id` is `undefined`, so the update targets no row. GraphView.vue:192 handles the same callback correctly by resolving the id to a node before emitting, which confirms TableView is the divergent one. - -**Fix:** Resolve the id to a node before emitting, e.g. `onToggleComplete: id => { const n = findNodeById(id); if (n) emit('toggle-complete', n) }` — `findNodeById` already exists at line 127. - -#### 56. Sensitive notes are rendered in plain text in the Notes column - -`src/components/TableView.vue`:289 - consistency - -The Notes cell renders note text unconditionally: - -```html -{{ truncateNotes(row.node.notes) }} -``` - -`notes_sensitive` is consulted only inside `shouldShowTooltip` (line 63), so it suppresses the hover preview but not the column itself or the `title` attribute. Every other surface masks the content whenever the node is flagged, independent of the `hideSensitive` toggle: `CardNotes.vue:25` shows a lock icon when `sensitive`, `useGraphElements.js:174` sets `hideSensitive: hideSensitive || node.notes_sensitive`, `useGraphEvents.js:342` bails on `nodeData.notes_sensitive`. `electron/database/index.js:365` decrypts notes for read once the sensitive session is unlocked, so real plaintext reaches this cell. - -**Fix:** Mask the cell when `row.node.notes_sensitive` is set (lock glyph, and omit the `title` attribute), matching CardNotes and the graph. - -#### 57. Trash "Delete" button is wired to the wrong handler and cannot permanently delete - -`src/components/TrashView.vue`:32 - correctness - -TrashView emits the whole item object: ``. ViewRenderer forwards it verbatim (`@delete="emit('delete', $event)"`, ViewRenderer.vue:295) and App.vue binds ViewRenderer's `delete` to `deleteNode` (App.vue:968). But `deleteNode` is typed and implemented as `async function deleteNode(nodeId: number)` (useNodeActionsUI.ts:196) and starts with `const node = await api.getNode(nodeId); if (!node) return`. Passing a Node object where a numeric id is expected means the lookup fails and the function returns silently — clicking Delete in the Trash view does nothing at all, with no error surfaced. Even in the best case `deleteNode` calls `api.deleteNode(id, false)` (soft delete), which is the wrong operation for an item that is already in the trash. - -The correct handler exists but is not wired: `permanentlyDelete(node: Node)` in useDataLoading.ts:286 calls `api.deleteNode(node.id, true)` and reloads the trash list. It is exported from useDataLoading (line 373) and declared in the return interface (line 69), but grepping src/, e2e/ and electron/ shows the only three hits are its own declaration, definition and export — App.vue's destructure at lines 176-197 pulls `restoreFromTrash` and `emptyAllTrash` but not `permanentlyDelete`. This violates the project rule that a capability existing in a composable but not reachable from the UI is unfinished work. There is also no vitest or Playwright coverage for TrashView at all. - -**Fix:** Give TrashView a distinct event for permanent deletion (e.g. `permanent-delete`), forward it through ViewRenderer, and bind it in App.vue to `permanentlyDelete` from `useDataLoading`. Do not reuse the generic `delete` channel, whose established payload across every other view is a numeric node id. Add a test that mounts TrashView, clicks Delete, and asserts `api.deleteNode(id, true)` was called. - -#### 58. Color picker renders nothing unless a color is already set, so it can never set one - -`src/components/detail/ColorPickerSection.vue`:18 - correctness - -`
`. Both consumers pass a node's stored color: PersonDetailForm.vue:287 (`default-color="#3498db"`) and OrganizationDetailForm.vue:216 (`default-color="#e67e22"`). For the common case of `editedNode.color == null` the component renders nothing, so `onColorUpdate` in both forms is unreachable and a person/organization can never be given a colour from the detail panel. `clearColor()` makes it a one-way door: it emits `null`, after which the picker disappears permanently. Note the detail panel is the only surface for person/org nodes — MetadataGridSection (which has a working colour field) is rendered only in the non-person/non-organization branch. - -**Fix:** Always render the input, seeded with `color || defaultColor`, and show the clear button only when an explicit colour is set. - -#### 59. Sensitive-note masking is absent from person/organization notes; the props that would provide it are never passed - -`src/components/detail/NotesSection.vue`:13 - correctness - -NotesSection declares `showSensitive` and `notesSensitive` props plus an `unlock-button` slot, and uses them at line 79. Grep across src/ and e2e/ shows the only two consumers — PersonDetailForm.vue:189 and OrganizationDetailForm.vue:133 — pass neither prop nor the slot, so `notesSensitive` is always false: a person or organization node with `notes_sensitive` set renders its notes in the clear, and when the note is encrypted the raw `SNENC1:` ciphertext is rendered as markdown and is editable (see the DetailPanel finding). Independently, the split branch (lines 87-102) ignores `notesSensitive` even if it were passed, so masking would still leak there. There is also no way to toggle `notes_sensitive` for a person/org from the detail panel at all — the `sensitive-btn` exists only in DetailPanel's non-person branch. - -**Fix:** Either pass the sensitive state (and lock state) from both forms and apply it in the split branch too, or delete the dead props and slot and route person/org notes through the same guarded rendering DetailPanel uses. - -#### 60. Deep watcher launches overlapping async org loads with no cancellation - -`src/components/detail/PersonDetailForm.vue`:165 - correctness - -`watch(() => props.linkedNodes, () => { loadLinkedOrganizations() }, { deep: true })` fires on any change to linkedNodes — including tag-only changes — and starts a run that awaits `api.getNode` once per ancestor plus one `getOrgPath` walk per leaf org. The promise is not awaited and no run token is kept, so on rapid link/unlink (or a node switch racing the id watcher at line 154) an older invocation can resolve last and write a stale `linkedOrganizations.value`. `getOrgPath` (line 45) and the ancestor walk (line 86) also loop on `parent_id` with no cycle guard, unlike `src/utils/nodeColor.js:resolveNodeColor` which explicitly keeps a `seen` set "in case parent links are malformed" — a cycle hangs the renderer here. - -**Fix:** Capture a request id (or the node id) at entry and drop the result if it changed before the awaits resolve; add the same `seen` cycle guard used in nodeColor.js to both parent walks. - -#### 61. Import always targets the 'work' workspace: currentWorkspace is never passed down - -`src/components/settings/DataSettings.vue`:72 - correctness - -DataSettings passes `props.currentWorkspace` to `api.importJSON(data, null, props.currentWorkspace)` / `api.importCSV(text, null, props.currentWorkspace)`, and `electron/database/export.js:241` really does use `workspaceId` (defaulting to 'work') to place imported nodes. - -The value never arrives. `App.vue:892` passes `:current-workspace="currentWorkspace"` to MainToolbar, but MainToolbar declares no `currentWorkspace` prop (props list at MainToolbar.vue:76-107) and does not pass `:current-workspace` to `` (MainToolbar.vue:242-292). MainToolbar has no `inheritAttrs:false` / `v-bind="$attrs"`, so the attribute lands on the toolbar root `
` and stops there. SettingsPanel therefore falls back to its own default `currentWorkspace: { type: String, default: 'work' }` (SettingsPanel.vue:44) and DataSettings imports into 'work' no matter which workspace the user is in. The hardcoded default is what hides the break — with `required: true` this would have been caught. No test covers the forwarding (there is no MainToolbar or DataSettings test). - -**Fix:** Declare `currentWorkspace` on MainToolbar and forward it to ``; drop the `'work'` default in SettingsPanel/DataSettings in favour of `required: true` so a missing value fails loudly, and add a test asserting the workspace id reaches api.importJSON. - -#### 62. `testConnection`/`listModels` are unreachable from the UI while AISettings.vue uses a parallel per-provider composable - -`src/composables/useAiNotes.js`:323 - design - -`useAiNotes` returns `testConnection()` and `listModels()`, which delegate cleanly to the provider adapter (`activeProvider.value.testConnection()` / `.listModels()`) — the abstraction `aiProviders.js` exists to provide, and which `src/__tests__/aiProviders.test.js` has a gate test defending ("no per-provider branching left in useAiNotes"). But the only components calling `useAiNotes()` are `NotesAIToolbar.vue` (takes `isGenerating, error, presetPrompts, improveNotes, research`) and `AISettings.vue` (takes only the prompt-management functions). For connection testing and model listing, AISettings.vue instead uses `useAIProviderConnection`, which hard-branches on the provider id and duplicates the whole surface twice — `testOllamaConnection`/`testOpenaiConnection`, `fetchOllamaModels`/`fetchOpenaiModels`, `ollamaModels`/`openaiModels`, `ollamaConnectionStatus`/`openaiConnectionStatus`, and per-provider `api.ollamaTestConnection`/`api.openaiTestConnection` calls. So the adapter-based path is dead (tests only), the branching path is the live one, and the anti-branching gate test passes while the branching sits one file over. `isConfigured`, `isEnabled`, `provider`, `generatedContent`, and `defaultPrompts` are likewise returned but consumed by no component. - -**Fix:** Pick one path: either drive AISettings.vue's connection test and model list through `useAiNotes().testConnection()/listModels()` (extending the adapter interface with whatever per-provider status the UI needs) and delete `useAIProviderConnection`, or delete the unreachable `testConnection`/`listModels`/`isConfigured`/`defaultPrompts` from `useAiNotes` and widen the gate test to cover `useAIProviderConnection`. - -#### 63. "Clear recent" filter compares SQLite timestamps against an ISO-8601 string, so cleared items never come back - -`src/composables/useDataLoading.ts`:194 - correctness - -clearRecent() stores `new Date().toISOString()` ("2026-08-26T12:34:56.789Z") in localStorage, but `updated_at` comes straight out of SQLite as `CURRENT_TIMESTAMP` ("2026-08-26 12:34:56") — electron/database/schema.js:80 and _rowToNode (electron/database/index.js:369) pass it through untransformed. loadRecentItems then does a lexicographic comparison: - -recentItems.value = validItems.filter((item: Node) => item && item.updated_at > clearedAt) - -For the same calendar date the strings are identical through index 9, then compare ' ' (0x20) against 'T' (0x54), so the DB value is ALWAYS "less than" the cleared marker. Result: after pressing "clear" in SpotlightSearch, every node touched for the rest of that UTC day is filtered out of Recent, and only items updated on a later date reappear. Secondarily, `api.getRecent(10, wsId)` applies `LIMIT 10` in SQL and the cleared-at filter is applied afterwards in the renderer, so the list can be short or empty even when 10 newer items exist. - -**Fix:** Normalize both sides before comparing — either store the marker in the DB's format (`new Date().toISOString().slice(0, 19).replace('T', ' ')`) or parse both with `Date.parse()` and compare numbers. Also fetch more than `limit` rows (or push the cleared-at cutoff into the query) so the post-filter cannot starve the list. Add a test with a realistic 'YYYY-MM-DD HH:MM:SS' updated_at; there is currently no test covering clearRecent at all. - -#### 64. undoClearRecent, permanentlyDelete and the returned showLostFound ref are not reachable from any UI - -`src/composables/useDataLoading.ts`:211 - dead-code - -Grepped src/, electron/, e2e/ and src/**tests**/ for each name outside useDataLoading.ts: `undoClearRecent` (line 211) — zero hits, so `previousRecentClearedAt` is write-only state and the "clear recent" action is irreversible from the UI even though the undo is implemented. `permanentlyDelete` (line 286) — zero hits; the trash UI uses restoreFromTrash/emptyAllTrash only. `showLostFound` (line 95, returned at line 351) — App.vue does not destructure it and declares its own `const showLostFound = ref(false)` at App.vue:98, so the composable's copy is a second, unused source of truth for the same panel. - -**Fix:** Wire undoClearRecent into the SpotlightSearch "clear" affordance (a toast with Undo) and permanentlyDelete into the trash list, or delete all three plus their UseDataLoadingReturn entries and `previousRecentClearedAt`. If showLostFound stays, App.vue should use it rather than shadow it. - -#### 65. Eight element data fields built for every node are never read by anything - -`src/composables/useGraphElements.js`:181 - dead-code - -`buildElements()` writes `label`, `tooltip`, `bgColor`, `textColor`, `isTag`, `isCompleted`, `totalNodes`, and `isCurrentContainer` into every node's `data`. I grepped src/, electron/, e2e/ and src/**tests**/ for each: nothing reads them. - -- `label` (line 169, `decodeHtmlEntities(node.title)`): the cytoscape stylesheet sets `label: ''` (useGraphInit.js:77) and the HTML-label template uses `escapeHtml(n.title)` from `nodeData`, never `d.label`. -- `tooltip` (line 172): the only consumer of `buildTooltipHTML` at render time is `useNodeTooltip.js:39`, which rebuilds it from the node. `data('tooltip')` is never read. This runs full tooltip HTML generation for every node on every graph rebuild for nothing. -- `bgColor`/`textColor` (lines 178-179): zero hits outside this file. The template uses `d.borderColor` and `d.customBgTint` only. `darkenColor()` therefore exists solely to feed dead fields (it is exported but has no importer either). -- `isTag`, `isCompleted`, `totalNodes`, `isCurrentContainer`: no reader, and no stylesheet selector uses them (the style array only matches `node[?isPerson]` and `edge[isLink]`). - -The same dead `bgColor`/`textColor`/`isCompleted` block is duplicated in `fetchLinkedNodes` at lines 405-408 and 416-420. - -**Fix:** Delete the eight fields from both element builders, delete `darkenColor` and the `buildTooltipHTML`/`decodeHtmlEntities` imports it made necessary, and keep only the fields the label template and stylesheet actually consume (`nodeData`, `type`, `isPerson`, `borderColor`, `customBgTint`, `hasChildren`, `childCount`, `isCollapsed`, `shouldGlow`, `showDetails`, `isSelected`, `isLinkedExternal`). - -#### 66. getNodeDimensions returns zoom-scaled pixels while the grid layout treats them as model units - -`src/composables/useGraphLayout.js`:39 - correctness - -`getNodeDimensions()` returns `rect.width`/`rect.height` straight from `getBoundingClientRect()` — screen pixels, i.e. model size multiplied by the current cytoscape zoom. Its own fallback branches return model units (`node.boundingBox()`, `parseFloat(style.width)`), so the three return paths are in two different coordinate systems. `runTetrisGridLayout()` then feeds those values into `node.position({x, y})`, which is model coordinates. - -The sibling function knows about this: `syncNodeDimensions()` (line 24-26) explicitly divides by `cy.zoom()` with the comment "Divide by zoom to get unscaled dimensions", and `reLayout()`'s grid branch works around it by forcing `cy.zoom(1)` / `cy.pan({x:0,y:0})` first (lines 398-400, "Reset zoom and spread nodes apart to ensure accurate DOM measurements"). - -`runGridLayout()` (line 766) has no such reset. It is called from `useGraphInit.applyInitialLayout()` line 185, on an instance that was just constructed with `layout: LAYOUTS.grid` (`{name:'preset', fit:true, padding:20}`) — `fit:true` has already changed the zoom away from 1. So the initial grid layout spaces nodes by zoom-multiplied widths. - -**Fix:** Divide by `cy.zoom()` in `getNodeDimensions()` (taking the cy instance or `node.cy()`) so all three return paths are model units, and drop the `cy.zoom(1)` workaround in `reLayout()`. Add a test that runs `runGridLayout()` at zoom 0.5 and 2 and asserts identical resulting positions. - -#### 67. getLayoutOptions('grid') returns a no-op preset layout, so grid mode silently fails wherever isGridMode() is not special-cased - -`src/composables/useGraphLayout.js`:243 - correctness - -`LAYOUTS.grid` is `{name: 'preset', ...}` — running it repositions nothing. The real grid arrangement lives in `runTetrisGridLayout`, reachable only via `reLayout()`'s `mode === 'grid'` branch or via `runGridLayout()`. - -Every consumer of `getLayoutOptions()` therefore has to know to check `isGridMode()` first. `useGraphInit.applyInitialLayout` does (line 184-185). `useGraphUpdate.handleNewNodes` does not: line 191-196 runs `cy.layout(getLayoutOptions()).run()` unconditionally when `!hasPos`. In grid mode that executes `preset`, so after a graph update with no saved positions the nodes stay wherever `findSmartPosition` scattered them and no grid is ever applied. - -This is a contract problem in this file, not just a caller bug: a function named `getLayoutOptions` returns options that do not implement the requested layout. - -**Fix:** Either expose a single `applyCurrentLayout(cy)` entry point that dispatches grid vs cytoscape layouts internally and have all callers use it, or make `getLayoutOptions('grid')` return a real cytoscape layout object whose `run()` invokes the tetris grid. Cover it with a test that puts the composable in grid mode and asserts node positions change after the update path. - -#### 68. Entire edit-modal and prompt-modal subsystem is unreachable - -`src/composables/useGraphModals.js`:42 - dead-code - -`showEditModal(node)` is the only writer that can set `editModal.value.visible = true`, and grepping the whole tree (`src/`, `electron/`, `e2e/`, `src/__tests__/`) finds it referenced nowhere except its own definition and the composable's return object. GraphView.vue destructures `editModal, hideEditModal, saveEditModal, goToParentFromModal, wrapWithParentFromModal` (lines 218-232) but never calls `showEditModal`, and nothing else assigns `editModal.value.visible`. Consequently `` (GraphView.vue:803) can never render, and `hideEditModal`, `saveEditModal`, `handleEditModalKeydown`, `goToParentFromModal`, `wrapWithParentFromModal` are all unreachable. The prompt modal is dead by transitivity: `showPrompt()` is called from exactly one place — `wrapWithParentFromModal` (line 196) — which itself starts with `if (!editModal.value.node) return`, a condition that is always true. So `promptModal`, `submitPrompt`, `cancelPrompt`, `handlePromptKeydown`, `promptInputRef` and `` (GraphView.vue:814) are dead too. That leaves `isAnyModalVisible()` (line 207) effectively reducing to `addNodeModal.value.visible`, and the tooltip guard `!editModal.value.visible` in GraphView.vue:212 a constant. This violates the project rule that code must be connected or deleted. - -**Fix:** Either wire `showEditModal` to a real GraphView interaction (e.g. the node context menu / an edit shortcut) and bind `GraphEditModal`'s exposed refs, or delete `showEditModal`, `hideEditModal`, `saveEditModal`, `handleEditModalKeydown`, `goToParentFromModal`, `wrapWithParentFromModal`, the whole prompt-modal block, and the two now-unused modal components from GraphView.vue. - -#### 69. `insertBetween` link branch destroys the original link with no rollback on failure - -`src/composables/useGraphOperations.js`:66 - correctness - -The `isLink` branch runs four sequential awaits: `api.unlinkNodes(parentId, childId)`, then `api.createNode(...)`, then two `api.linkNodes(...)` calls. There is no transaction and no compensating action. Failure scenario: `unlinkNodes` succeeds, then `createNode` rejects (validation error, DB locked, workspace lookup failure). The user's original link between `parentId` and `childId` is now permanently gone, and the catch block only surfaces a transient toast (`handleError(e, { context: 'Inserting node' })`) — the user is told "inserting failed" while the pre-existing data has actually been destroyed. The same applies if the second `linkNodes` fails, leaving an orphan node linked on one side only. - -**Fix:** Create the new node first, then link both sides, and only unlink the original edge once both links succeeded — or wrap the sequence in a service-level transaction and re-create the original link in the catch. - -#### 70. Local `isTextInput` duplicates and diverges from the shared `ownsTextInput` ownership rule - -`src/composables/useKeyboardShortcuts.js`:102 - consistency - -The file imports `ownsTextInput` and aliases it as `isEditableElement`, then defines a second, weaker predicate: - -```js -function isTextInput(target) { - if (target.tagName === 'TEXTAREA' || target.isContentEditable) return true - if (target.tagName === 'INPUT') { ... } - return false -} -``` - -The Space and Enter branches use `isTextInput`, everything below uses `ownsTextInput`. Unlike `ownsTextInput`, `isTextInput` does not recognise `SELECT`, a `[contenteditable]` _ancestor_, or a `.cm-editor` (CodeMirror) subtree. Consequence: pressing Space with a `