Repository navigation
v2.1.0 - Architectural Cleanup - #53
Merged
Merged
Conversation
…omponent extractions Phases P0-P3 of the architectural deep dive documented in docs/cleanup/. P0 — Hygiene & Safety Net: - Delete dead code: mcp-native/, pkg-native/_node25.node, check-tool-parity.ts, tool-parity.test.ts, stale dist artifacts - Fix CI type-check: now covers electron code (was renderer-only); surface and fix all 142 pre-existing electron type errors via generic IpcHandler - Make IpcHandler generic + split IpcHandleHandler/IpcOnHandler for proper event typing (no ) - Reconcile vitest native binding shim (was ignoring env var) - Align esbuild target node20→node22 (matches Electron 41 + pkg binary) - Fix stale AGENTS.md ABI-boundary claim (unblocks P1) P1 — SQL Consolidation (~216 LOC removed from MCP, ~400 LOC algorithm dedup): - All 7 MCP tool files (tags, projects, dashboards, notes, tasks, flow, graph) now delegate to electron/db/queries.ts + graph-queries.ts instead of duplicating SQL. The 'ABI boundary' was fictional — codebase.ts already proved cross-bundle imports work. - mcp/db.ts snapshot delegates to q.getFullSnapshot - queries.ts gains governance header documenting single-source-of-truth status P2 — IPC handlers.ts decomposition (1054→170 LOC, -884 lines): - Split god-file into 12 per-domain registrars + result-helpers.ts - Extract inlined PDF HTML template→lib/pdf-template.ts - Extract inlined OG scraper→lib/url-metadata.ts - handlers.ts is now a thin orchestrator calling registerXxxHandlers() P3 — Component extractions: - Consolidate graph helpers (delete graphUtils.ts, merge into analyticsUtils) - Move editorTheme.ts from agent/ to lib/ (was cross-feature dependency) - Finish chat-panel/ + project-overview/ folder refactors (index.tsx) - Extract useIpcErrorToasts from page.tsx inline hook; fix mid-file imports - Create <ModalShell> on ui/dialog.tsx; migrate MoveNoteModal + DashboardApiModal - Extract shared CairnRefChip + CAIRN_NOTE/TASK_ACTIONS from both message bubbles (was byte-identical duplication, ~155 LOC saved) Verification: type-check:all, lint --max-warnings 0, compile, 430 tests, smoke test all pass. Net: +600 / -3677 LOC across 44 modified + 28 new files.
…ks, dedup tick grid - Unify 4 duplicate priority color/weight definitions across Timeline, Table, Sankey onto shared PRIORITY_COLOR + PRIORITY_SORT_ORDER from analyticsUtils - Remove dead props: containerH? on CanvasTooltip, plotH on SvgTimeAxis - Extract useRelativePointer(ref) hook — replaces 3 inline getBoundingClientRect calls - Extract useNow() hook — replaces 3 Date.now() calls that needed eslint purity disables - Replace RidgelineCanvas inlined tick grid with <SvgTimeAxis> (−22 LOC) - Unify empty states: Timeline + Matrix now use <CanvasEmptyState> - Merge dual imports in BeeswarmCanvas - Revive PRIORITY_WEIGHT (was dead export) + add PRIORITY_SORT_ORDER All 4 react-hooks/purity eslint disables removed. type-check:all, lint, compile, 430 tests pass.
…w decomposition P5-1: Relocate PiAgentMessage/PiSubagentMessage/TerminalSession/PiSessionSummary types from store/slices/terminal-sessions.ts → types/index.ts (688→620 LOC) P5-2: Extract UpdateBanner + ErrorToasts from page.tsx inline → layout/app-chrome.tsx (410→341 LOC) P5-4: Decompose project-overview/index.tsx (664→427 LOC) — extracted 10 free function sub-components into primitives.tsx (69 LOC) + sections.tsx (214 LOC) All checks green: type-check:all, lint --max-warnings 0, compile, 430 tests.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR updates build and test scripts, decomposes Electron IPC registration into domain modules, moves MCP tool persistence onto shared query helpers, extracts shared renderer UI and types, standardizes analytics helpers, splits several large UI components, and refreshes repository documentation and release notes. ChangesCleanup consolidation
Estimated code review effort🎯 5 (Critical) | ⏱️ ~105 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (13)
src/components/shared/cairn-ref-chip.tsx-75-76 (1)
75-76:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winNormalize chip colors to token-only values and
color-mixalpha syntax.Line 75/76 use slash-alpha classes, and Line 83/87 include a hardcoded hex fallback (
#22c55e). Both conflict with the project’s theming rules.Suggested class updates
- "hover:border-[var(--accent)]/50 hover:bg-[color-mix(in_srgb,var(--accent)_4%,var(--surface-2))]", + "hover:border-[color-mix(in_srgb,var(--accent)_50%,transparent)] hover:bg-[color-mix(in_srgb,var(--accent)_4%,var(--surface-2))]", - !ok && "border-[var(--danger)]/30 opacity-60 pointer-events-none", + !ok && "border-[color-mix(in_srgb,var(--danger)_30%,transparent)] opacity-60 pointer-events-none", - : "bg-[color-mix(in_srgb,var(--success,`#22c55e`)_12%,transparent)]", + : "bg-[color-mix(in_srgb,var(--success)_12%,transparent)]", - : <SquareCheck size={10} className="text-[color-mix(in_srgb,var(--success,`#22c55e`)_90%,var(--text-primary))]" /> + : <SquareCheck size={10} className="text-[color-mix(in_srgb,var(--success)_90%,var(--text-primary))]" />As per coding guidelines, “All colour values must use CSS variables” and “Alpha variants must use
color-mix(in srgb, var(--token) X%, transparent)syntax.”Also applies to: 83-87
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/shared/cairn-ref-chip.tsx` around lines 75 - 76, In the cairn-ref-chip.tsx component, normalize all color values to use CSS variables and the proper alpha syntax. On line 75, replace the slash-alpha syntax in "hover:border-[var(--accent)]/50" with the color-mix pattern using color-mix(in srgb, var(--accent) 50%, transparent). Also update the "hover:bg-[color-mix(in_srgb,var(--accent)_4%,var(--surface-2))]" syntax to use proper color-mix with transparent instead of color-mixing with another color variable. Additionally, on lines 83-87, replace any hardcoded hex color values (such as `#22c55e`) with their corresponding CSS variable tokens and ensure they follow the same color-mix(in srgb, var(--token) X%, transparent) pattern for any alpha applications.Source: Coding guidelines
src/components/layout/app-chrome.tsx-23-24 (1)
23-24:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReplace raw and slash-alpha colors with tokenized
color-mix(...)classes.Line 33 uses
text-white, and Lines 23/61 use slash-alpha borders (.../30). These conflict with the repo’s color-token and alpha-variant rules.Suggested class updates
- className="flex items-center gap-3 px-4 py-2 bg-[var(--accent-dim)] border-b border-[var(--accent)]/30 flex-shrink-0" + className="flex items-center gap-3 px-4 py-2 bg-[var(--accent-dim)] border-b border-[color-mix(in_srgb,var(--accent)_30%,transparent)] flex-shrink-0" - className="px-3 py-1 rounded-md text-xs font-medium bg-[var(--accent)] text-white hover:bg-[var(--accent-hover)] transition-colors" + className="px-3 py-1 rounded-md text-xs font-medium bg-[var(--accent)] text-[var(--text-on-accent)] hover:bg-[var(--accent-hover)] transition-colors" - className="flex items-start gap-2 px-3 py-2.5 rounded-lg border border-[var(--danger)]/30 bg-[var(--background)] shadow-lg max-w-xs pointer-events-auto" + className="flex items-start gap-2 px-3 py-2.5 rounded-lg border border-[color-mix(in_srgb,var(--danger)_30%,transparent)] bg-[var(--background)] shadow-lg max-w-xs pointer-events-auto"As per coding guidelines, “All colour values must use CSS variables” and “Alpha variants must use
color-mix(in srgb, var(--token) X%, transparent)syntax.”Also applies to: 33-33, 61-61
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/layout/app-chrome.tsx` around lines 23 - 24, Replace all raw and slash-alpha color syntax with tokenized color-mix classes in the app-chrome.tsx file. On line 23, replace the border-[var(--accent)]/30 class with a color-mix syntax using the accent token with appropriate opacity (e.g., color-mix(in srgb, var(--accent) 70%, transparent)). On line 33, replace the text-white class with a tokenized color class using a CSS variable token instead of the raw white color. On line 61, similarly replace any slash-alpha border syntax with the color-mix approach. Ensure all color values reference CSS variables via var() and alpha transparency is expressed using the color-mix(in srgb, var(--token) X%, transparent) syntax as per the coding guidelines.Source: Coding guidelines
src/hooks/useIpcErrorToasts.ts-33-38 (1)
33-38:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClear toast timers when entries are evicted and on unmount.
Line 33 drops old toasts from state, but their timers remain active; Line 37 also removes only the event listener. This leaves avoidable pending callbacks and temporary map growth during error bursts.
Proposed fix
useEffect(() => { function onIpcError(e: Event) { const message = (e as CustomEvent<{ message: string }>).detail.message; const id = ++_toastSeq; - setToasts((prev) => [...prev.slice(-4), { id, message }]); // keep at most 5 + setToasts((prev) => { + const next = [...prev, { id, message }]; + const evicted = next.slice(0, -5); + for (const t of evicted) { + clearTimeout(timers.current.get(t.id)); + timers.current.delete(t.id); + } + return next.slice(-5); // keep at most 5 + }); timers.current.set(id, setTimeout(() => dismiss(id), 5000)); } window.addEventListener("cairn:ipc-error", onIpcError); - return () => window.removeEventListener("cairn:ipc-error", onIpcError); + return () => { + window.removeEventListener("cairn:ipc-error", onIpcError); + for (const handle of timers.current.values()) clearTimeout(handle); + timers.current.clear(); + }; }, [dismiss]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/hooks/useIpcErrorToasts.ts` around lines 33 - 38, The useIpcErrorToasts hook leaks timer callbacks when toasts are evicted and on component unmount. In the setToasts state update on line 33, when old entries are dropped from the array (keeping only the last 5), their associated timers remain active in the timers.current map. Additionally, the cleanup function on line 37 only removes the event listener but does not clear the remaining timers. Fix this by: (1) identifying which toast IDs are being removed when keeping only the last 4 toasts and clearing their timers from timers.current before updating state, and (2) updating the cleanup function to iterate through all remaining timers in timers.current, clearing each timeout and removing the entry from the map before removing the event listener.src/components/layout/project-overview/sections.tsx-106-108 (1)
106-108:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReplace pixel-based Tailwind font-size classes with rem-based equivalents.
These
text-[Npx]classes won’t scale with user font settings and violate the typography rule.Proposed fix
- <div key={c.id} className="text-[10.5px] text-[var(--text-tertiary)] truncate leading-snug">{c.title}</div> + <div key={c.id} className="text-[0.714rem] text-[var(--text-tertiary)] truncate leading-snug">{c.title}</div> ))} - {cards.length === 0 && <div className="text-[10.5px] text-[var(--text-tertiary)]">—</div>} + {cards.length === 0 && <div className="text-[0.714rem] text-[var(--text-tertiary)]">—</div>} @@ - <div className="text-[11.5px] text-[var(--text-tertiary)] line-clamp-2 leading-relaxed mb-3"> + <div className="text-[0.714rem] text-[var(--text-tertiary)] line-clamp-2 leading-relaxed mb-3"> {note.contentText.slice(0, 120) || "Empty note"} </div>As per coding guidelines,
**/*.{tsx,jsx,css,scss}font sizes must use rem-based Tailwind classes and must not usetext-[Npx]classes.Also applies to: 155-156
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/layout/project-overview/sections.tsx` around lines 106 - 108, Replace all pixel-based Tailwind font-size classes with rem-based equivalents in the sections.tsx file. Specifically, change the text-[10.5px] classes used in the card title rendering section and any other instances (including around lines 155-156) to use rem-based Tailwind typography classes instead of the px-based custom values. This ensures font sizes scale properly with user font settings as per the coding guidelines for typography in tsx/jsx files.Source: Coding guidelines
src/components/graph/analyticsUtils.ts-18-18 (1)
18-18:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse a CSS-token fallback instead of a hardcoded color.
"#888"breaks the project’s color-token rule for TS/TSX/CSS files. Use a CSS variable token as the non-DOM fallback.Proposed fix
export function resolveCssVar(varName: string): string { - if (typeof document === "undefined") return "`#888`"; + if (typeof document === "undefined") return "var(--text-tertiary)"; return getComputedStyle(document.documentElement) .getPropertyValue(varName.replace(/^var\((.+)\)$/, "$1")) .trim(); }As per coding guidelines,
**/*.{css,scss,tsx,ts,jsx,js}: All colour values must use CSS variables ... instead of ... hardcoded values.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/graph/analyticsUtils.ts` at line 18, The hardcoded color value "`#888`" in the analyticsUtils.ts file violates the project's color-token rule which requires all color values to use CSS variables instead of hardcoded values. Replace the hardcoded "`#888`" value with an appropriate CSS variable token from your project's design system or theme that serves as a fallback color. This ensures consistency across all color usage in the codebase and makes it easier to maintain and update the color scheme globally.Source: Coding guidelines
src/components/graph/analyticsUtils.ts-53-57 (1)
53-57:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix the sort-direction doc to match the map.
The comment says ascending priority is
low → urgent, buturgent: 0 ... low: 3means ascending currently sortsurgent → low. Please align the doc string with actual behavior.Proposed fix
- * Sort key for ascending sort (low → urgent). Convenience for canvases that + * Sort key for ascending sort (urgent → low). Convenience for canvases that🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/graph/analyticsUtils.ts` around lines 53 - 57, The JSDoc comment for the PRIORITY_SORT_ORDER constant in analyticsUtils.ts incorrectly describes the sort direction. The comment states "ascending sort (low → urgent)" but the actual mapping has urgent: 0 and low: 3, which means ascending sort actually goes urgent → low, not low → urgent. Update the comment above the PRIORITY_SORT_ORDER object to correctly state that ascending priority sort follows the order urgent → low to match the actual numeric values in the Record.src/components/graph/MatrixCanvas.tsx-93-95 (1)
93-95:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winWrap
CanvasEmptyStatein a positioned container in the early-return branch.
CanvasEmptyStateis absolutely positioned; returning it bare here can anchor it to an unintended ancestor and misplace the empty state.Proposed fix
if (activeTags.length === 0) { - return <CanvasEmptyState message="No tagged items in this scope." />; + return ( + <div className="flex-1 relative overflow-hidden min-h-0"> + <CanvasEmptyState message="No tagged items in this scope." /> + </div> + ); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/graph/MatrixCanvas.tsx` around lines 93 - 95, In the early-return branch where activeTags.length equals zero, the CanvasEmptyState component is being returned without a positioned container. Since CanvasEmptyState uses absolute positioning, it needs to be wrapped in a parent container with appropriate positioning (such as a div with relative positioning) to ensure it anchors to the correct ancestor. Replace the bare return of CanvasEmptyState with a return statement that wraps it in a positioned container element to prevent unintended positioning issues.electron/mcp/tools/dashboards.ts-25-35 (1)
25-35:⚠️ Potential issue | 🟡 Minor
update_dashboardis missingupdatedAtin return payload — add for consistency with similar tools.Unlike
create_dashboard(which returnscreatedAt) and other update tools likepatch_note(which returnupdatedAt),update_dashboardcurrently returns only{ id, title }. Theq.updateNotefunction already retrieves and returns the updated note withupdatedAt, but it is discarded. Add it to the return for consistency:Suggested fix
- return { id: noteId, title: (title as string) ?? note.title }; + const updated = q.getNoteById(db, noteId as string); + return { id: noteId, title: (title as string) ?? note.title, updatedAt: updated?.updatedAt };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/mcp/tools/dashboards.ts` around lines 25 - 35, The update_dashboard function is missing updatedAt in its return payload for consistency with other update tools like patch_note and create_dashboard. Modify the function to capture the return value from q.updateNote instead of discarding it, then extract the updatedAt field from the returned note object and include it in the final return statement alongside id and title to maintain consistency with similar dashboard and note update operations.electron/ipc/db-handlers.ts-235-240 (1)
235-240:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
app:revealAssetspoints to a different directory than uploads.Line 236 opens
<workspace>/assets, but uploads on Line 186+ are written toattachments(or ObsidianattachmentFolderPath). Users won’t see newly uploaded files in the revealed folder.Suggested fix
registerIpcHandle("app:revealAssets", () => handle(async () => { - const assetDir = path.join(ctx.workspacePath, "assets"); + const assetDir = path.join(ctx.workspacePath, "attachments"); fs.mkdirSync(assetDir, { recursive: true }); // shell.openPath returns a Promise<string>; non-empty = error message. const errMsg = await shell.openPath(assetDir); if (errMsg) console.error("[cairn:revealAssets]", errMsg); }));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/ipc/db-handlers.ts` around lines 235 - 240, The app:revealAssets IPC handler is opening the incorrect directory. It currently opens the "assets" folder within the workspace path, but file uploads are written to an "attachments" folder (or the configured Obsidian attachmentFolderPath). Fix the assetDir path construction in the registerIpcHandle for "app:revealAssets" to use the same attachment folder path that is used for uploads elsewhere in the codebase (referenced around line 186), so that users see the actual uploaded files when the folder is revealed.electron/ipc/settings-handlers.ts-17-17 (1)
17-17:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse
?? nullinstead of|| nullin cached setting reads.Line 32 currently turns a persisted
0font scale intonull. Using nullish coalescing preserves valid falsy values.Suggested fix
- registerIpcHandle("app:getAiSettings", () => handle(() => getCachedConfig().aiConfig || null)); + registerIpcHandle("app:getAiSettings", () => handle(() => getCachedConfig().aiConfig ?? null)); @@ - registerIpcHandle("app:getAgentSettings", () => handle(() => getCachedConfig().agentConfig || null)); + registerIpcHandle("app:getAgentSettings", () => handle(() => getCachedConfig().agentConfig ?? null)); @@ - registerIpcHandle("app:getTheme", () => handle(() => getCachedConfig().theme || null)); + registerIpcHandle("app:getTheme", () => handle(() => getCachedConfig().theme ?? null)); @@ - registerIpcHandle("app:getFontScale", () => handle(() => getCachedConfig().fontScale || null)); + registerIpcHandle("app:getFontScale", () => handle(() => getCachedConfig().fontScale ?? null));Also applies to: 22-22, 27-27, 32-32
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/ipc/settings-handlers.ts` at line 17, Replace the logical OR operator `|| null` with the nullish coalescing operator `?? null` in all cached setting read operations. This applies to multiple instances in the file where registerIpcHandle is called with getCachedConfig() property accessors (such as getCachedConfig().aiConfig, and others referenced on lines 22, 27, and 32). The change ensures that valid falsy values like 0 are preserved rather than being converted to null.electron/lib/pdf-template.ts-36-37 (1)
36-37:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReplace direct hex color literals with CSS variable tokens in the template CSS.
Line 36, Line 37, and Line 53 still use hardcoded colors (
#ffffff,#1a1917,#374151). Route these through semantic CSS variables and reference onlyvar(--...)in declarations for consistency with the repo styling contract.
As per coding guidelines,**/*.{css,scss,tsx,ts,jsx,js}requires all colour values to use CSS variables instead of hardcoded values.Also applies to: 53-53
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/lib/pdf-template.ts` around lines 36 - 37, In the pdf-template.ts file, replace the hardcoded hex color values with CSS variable tokens. Specifically, change the background property from `#ffffff` to use an appropriate CSS variable reference like var(--...), the color property from `#1a1917` to use a text color CSS variable, and the color at line 53 from `#374151` to use the corresponding CSS variable. Ensure all color declarations in the template CSS use only var(...) syntax instead of direct hex literals to maintain consistency with the repository's styling contract.Source: Coding guidelines
electron/ipc/url-metadata.ts-17-18 (1)
17-18:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate IPC payload inside
handle()to preserve the wrapped IPC result contract.Line 17 destructures
{ url }before enteringhandle(). If payload shape is wrong, it throws outside the wrapper and can bypass the standard{ error }response path expected by preloadinvoke<T>().Proposed patch
export function registerUrlMetadataHandler(): void { - registerIpcHandle("db:flow:url:fetch", (_e, { url }: { url: string }) => - handle(() => fetchUrlMetadata(url)) + registerIpcHandle("db:flow:url:fetch", (_e, payload: unknown) => + handle(() => { + const url = + payload && + typeof payload === "object" && + "url" in payload && + typeof (payload as { url?: unknown }).url === "string" + ? (payload as { url: string }).url + : null; + if (!url) throw new Error("Invalid payload: expected { url: string }"); + return fetchUrlMetadata(url); + }) ); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/ipc/url-metadata.ts` around lines 17 - 18, The payload destructuring for `{ url }` in the IPC handler registered for "db:flow:url:fetch" occurs before the handle() wrapper, which means if the incoming payload has an unexpected shape, the error will be thrown outside the wrapper and bypass the standard error response contract. Move the URL parameter validation and destructuring inside the handle() function so that any validation errors are properly caught and formatted as the expected { error } response by the wrapper function.electron/lib/url-metadata.ts-64-77 (1)
64-77:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winImplement the documented hostname fallback (or fix the comment).
Line 64 says fallback includes hostname, but Line 76 falls back to an empty string. If metadata is missing, return
parsed.hostnameto match the documented behavior.Proposed patch
- title: (ogTitle ?? htmlTitle ?? "").trim().slice(0, 200), + title: (ogTitle ?? htmlTitle ?? parsed.hostname).trim().slice(0, 200),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/lib/url-metadata.ts` around lines 64 - 77, The code comment on line 64 documents a hostname fallback for the title, but the actual implementation in the return statement falls back to an empty string instead. Update the title property in the return object to use the hostname as the final fallback by changing the expression from (ogTitle ?? htmlTitle ?? "") to (ogTitle ?? htmlTitle ?? parsed.hostname ?? ""), ensuring the documented hostname fallback behavior is actually implemented.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@electron/ipc/db-handlers.ts`:
- Around line 70-75: In the registerIpcHandle("db:project:delete") handler, the
order of operations needs to be reversed. Currently deleteProjectNotesDir is
called before q.deleteProject, which means if the database deletion fails, the
filesystem files are already gone. Move the q.deleteProject(ctx.db, id) call to
execute before deleteProjectNotesDir(ctx.workspacePath, project.name) so that if
the database operation fails, the project files remain intact and can be
recovered.
- Around line 51-68: The default column creation in the "db:project:create" IPC
handler performs multiple separate database writes without a transaction, which
means a failure during the loop in the withDefaultColumns block can leave some
columns created while others are not. Wrap the default column creation logic
(the map call that invokes q.createColumn multiple times) in a database
transaction so that all columns are created atomically—either all succeed
together or all fail together if an error occurs mid-loop. This ensures the
project and its default columns are created in a consistent state.
In `@electron/ipc/flow-handlers.ts`:
- Around line 122-126: The node validation in the area checking "SELECT * FROM
idea_flow_nodes WHERE id = ?" only verifies node existence but does not validate
that the node is of type `ai_summary`. This allows summary data to be written to
any node type at line 220. After the existing node existence check (if
!nodeRow), add an additional validation that confirms the node type is
`ai_summary` and return an error if it is not, similar to the existing "Node not
found" error pattern.
- Around line 101-117: The registerIpcHandle call for "db:flow:node:summarize"
is not wrapped in handle(...) and returns a raw error object from
resolveAiConfig instead of using the err(...) function, breaking IpcResult<T>
consistency. Wrap the entire handler logic in the handle(...) function and
replace the direct return of the resolved error object with a call to err(...)
to ensure proper error handling. Apply the same fix to the other handlers
mentioned at lines 122-123 and 213-217 that have similar patterns.
In `@electron/ipc/pdf-export.ts`:
- Line 66: Replace the synchronous fs.writeFileSync call in the pdf-export
handler with the async variant fs.promises.writeFile to prevent blocking the
main thread. Since the function is already async and awaits other operations
like printToPDF, add an await operator before the fs.promises.writeFile call to
maintain consistent async/await patterns and ensure the application remains
responsive when writing larger PDF files.
- Around line 45-61: The BrowserWindow creation and subsequent operations
(loadURL and printToPDF calls) lack proper resource cleanup when errors occur,
causing potential window leaks. Wrap the BrowserWindow instantiation and all
operations (the new BrowserWindow call, the loadURL invocation, and the
printToPDF call) in a try block, then move the printWin.destroy() call to a
finally block to ensure it always executes regardless of whether loadURL or
printToPDF throws an exception. This guarantees the hidden window is properly
destroyed and resources are freed even when errors occur during PDF generation.
In `@electron/lib/config-cache.ts`:
- Around line 18-19: The CachedConfig type definition now includes maxSteps and
temperature fields (lines 18-19), but the "agent" type save branch (lines 60-67)
does not persist these values when writing the cache. Locate the code block that
handles saving when type === "agent" and add logic to copy the maxSteps and
temperature properties from agentConfig to the cache object being saved,
ensuring these values are not silently lost after save and reload operations.
In `@electron/lib/url-metadata.ts`:
- Around line 29-45: The fetchUrlMetadata function validates only the protocol
but not the target host, allowing SSRF attacks via localhost and private IP
ranges. Add host validation after the URL parsing in the try block to reject
non-public addresses, including localhost (127.0.0.1, ::1), private IP ranges
(10.0.0.0/8, 172.16.0.0/12, 192.168.0.0/16), and link-local addresses. Use the
hostname property from the already-parsed URL object to check against these
blocked ranges, and throw an appropriate error if the host is not public.
Additionally, after receiving the response from net.fetch, validate that any
redirect target URLs also meet the same host restrictions before following them.
In `@electron/mcp/tools/flow.ts`:
- Around line 145-149: The create_idea_flow_edge function retrieves the flowId
from the source node but only checks that the target node exists without
verifying it belongs to the same flow. This allows creating edges between nodes
from different flows, breaking flow consistency. Modify the target node
existence check (the SELECT query at line 148) to also verify that the target
node's flow_id matches the flowId obtained from the source node. Include both
the node id and flow_id in the WHERE clause to ensure both source and target
nodes are in the same flow before allowing the edge to be created.
- Around line 94-102: The code in the flow edge creation block is reporting edge
IDs to the createdEdges array without verifying that the q.createFlowEdge() call
actually succeeded in creating the edge in the database. If the edge creation is
ignored due to a duplicate or error condition, the reported edgeId becomes a
phantom ID that doesn't actually exist in idea_flow_edges. Modify the logic
around the q.createFlowEdge(db, {...}) call to check if the edge was
successfully created, and only push the edge details to createdEdges if the
creation actually succeeded. This ensures only valid edge IDs are returned to
the caller.
In `@electron/mcp/tools/tasks.ts`:
- Around line 211-223: The early-return branch for `dueDate === null` has two
issues: it returns the stale result from `q.clearCardDueDate` without accounting
for the subsequent `q.updateCard` changes, and it bypasses the blocker-clearing
logic that checks for done columns. Fix this by returning the result of
`q.updateCard` instead of `updated`, and add a check after the update to invoke
the blocker-clearing logic (similar to the pattern at lines 250-255) if columnId
is being updated to a done column.
---
Minor comments:
In `@electron/ipc/db-handlers.ts`:
- Around line 235-240: The app:revealAssets IPC handler is opening the incorrect
directory. It currently opens the "assets" folder within the workspace path, but
file uploads are written to an "attachments" folder (or the configured Obsidian
attachmentFolderPath). Fix the assetDir path construction in the
registerIpcHandle for "app:revealAssets" to use the same attachment folder path
that is used for uploads elsewhere in the codebase (referenced around line 186),
so that users see the actual uploaded files when the folder is revealed.
In `@electron/ipc/settings-handlers.ts`:
- Line 17: Replace the logical OR operator `|| null` with the nullish coalescing
operator `?? null` in all cached setting read operations. This applies to
multiple instances in the file where registerIpcHandle is called with
getCachedConfig() property accessors (such as getCachedConfig().aiConfig, and
others referenced on lines 22, 27, and 32). The change ensures that valid falsy
values like 0 are preserved rather than being converted to null.
In `@electron/ipc/url-metadata.ts`:
- Around line 17-18: The payload destructuring for `{ url }` in the IPC handler
registered for "db:flow:url:fetch" occurs before the handle() wrapper, which
means if the incoming payload has an unexpected shape, the error will be thrown
outside the wrapper and bypass the standard error response contract. Move the
URL parameter validation and destructuring inside the handle() function so that
any validation errors are properly caught and formatted as the expected { error
} response by the wrapper function.
In `@electron/lib/pdf-template.ts`:
- Around line 36-37: In the pdf-template.ts file, replace the hardcoded hex
color values with CSS variable tokens. Specifically, change the background
property from `#ffffff` to use an appropriate CSS variable reference like
var(--...), the color property from `#1a1917` to use a text color CSS variable,
and the color at line 53 from `#374151` to use the corresponding CSS variable.
Ensure all color declarations in the template CSS use only var(...) syntax
instead of direct hex literals to maintain consistency with the repository's
styling contract.
In `@electron/lib/url-metadata.ts`:
- Around line 64-77: The code comment on line 64 documents a hostname fallback
for the title, but the actual implementation in the return statement falls back
to an empty string instead. Update the title property in the return object to
use the hostname as the final fallback by changing the expression from (ogTitle
?? htmlTitle ?? "") to (ogTitle ?? htmlTitle ?? parsed.hostname ?? ""), ensuring
the documented hostname fallback behavior is actually implemented.
In `@electron/mcp/tools/dashboards.ts`:
- Around line 25-35: The update_dashboard function is missing updatedAt in its
return payload for consistency with other update tools like patch_note and
create_dashboard. Modify the function to capture the return value from
q.updateNote instead of discarding it, then extract the updatedAt field from the
returned note object and include it in the final return statement alongside id
and title to maintain consistency with similar dashboard and note update
operations.
In `@src/components/graph/analyticsUtils.ts`:
- Line 18: The hardcoded color value "`#888`" in the analyticsUtils.ts file
violates the project's color-token rule which requires all color values to use
CSS variables instead of hardcoded values. Replace the hardcoded "`#888`" value
with an appropriate CSS variable token from your project's design system or
theme that serves as a fallback color. This ensures consistency across all color
usage in the codebase and makes it easier to maintain and update the color
scheme globally.
- Around line 53-57: The JSDoc comment for the PRIORITY_SORT_ORDER constant in
analyticsUtils.ts incorrectly describes the sort direction. The comment states
"ascending sort (low → urgent)" but the actual mapping has urgent: 0 and low: 3,
which means ascending sort actually goes urgent → low, not low → urgent. Update
the comment above the PRIORITY_SORT_ORDER object to correctly state that
ascending priority sort follows the order urgent → low to match the actual
numeric values in the Record.
In `@src/components/graph/MatrixCanvas.tsx`:
- Around line 93-95: In the early-return branch where activeTags.length equals
zero, the CanvasEmptyState component is being returned without a positioned
container. Since CanvasEmptyState uses absolute positioning, it needs to be
wrapped in a parent container with appropriate positioning (such as a div with
relative positioning) to ensure it anchors to the correct ancestor. Replace the
bare return of CanvasEmptyState with a return statement that wraps it in a
positioned container element to prevent unintended positioning issues.
In `@src/components/layout/app-chrome.tsx`:
- Around line 23-24: Replace all raw and slash-alpha color syntax with tokenized
color-mix classes in the app-chrome.tsx file. On line 23, replace the
border-[var(--accent)]/30 class with a color-mix syntax using the accent token
with appropriate opacity (e.g., color-mix(in srgb, var(--accent) 70%,
transparent)). On line 33, replace the text-white class with a tokenized color
class using a CSS variable token instead of the raw white color. On line 61,
similarly replace any slash-alpha border syntax with the color-mix approach.
Ensure all color values reference CSS variables via var() and alpha transparency
is expressed using the color-mix(in srgb, var(--token) X%, transparent) syntax
as per the coding guidelines.
In `@src/components/layout/project-overview/sections.tsx`:
- Around line 106-108: Replace all pixel-based Tailwind font-size classes with
rem-based equivalents in the sections.tsx file. Specifically, change the
text-[10.5px] classes used in the card title rendering section and any other
instances (including around lines 155-156) to use rem-based Tailwind typography
classes instead of the px-based custom values. This ensures font sizes scale
properly with user font settings as per the coding guidelines for typography in
tsx/jsx files.
In `@src/components/shared/cairn-ref-chip.tsx`:
- Around line 75-76: In the cairn-ref-chip.tsx component, normalize all color
values to use CSS variables and the proper alpha syntax. On line 75, replace the
slash-alpha syntax in "hover:border-[var(--accent)]/50" with the color-mix
pattern using color-mix(in srgb, var(--accent) 50%, transparent). Also update
the "hover:bg-[color-mix(in_srgb,var(--accent)_4%,var(--surface-2))]" syntax to
use proper color-mix with transparent instead of color-mixing with another color
variable. Additionally, on lines 83-87, replace any hardcoded hex color values
(such as `#22c55e`) with their corresponding CSS variable tokens and ensure they
follow the same color-mix(in srgb, var(--token) X%, transparent) pattern for any
alpha applications.
In `@src/hooks/useIpcErrorToasts.ts`:
- Around line 33-38: The useIpcErrorToasts hook leaks timer callbacks when
toasts are evicted and on component unmount. In the setToasts state update on
line 33, when old entries are dropped from the array (keeping only the last 5),
their associated timers remain active in the timers.current map. Additionally,
the cleanup function on line 37 only removes the event listener but does not
clear the remaining timers. Fix this by: (1) identifying which toast IDs are
being removed when keeping only the last 4 toasts and clearing their timers from
timers.current before updating state, and (2) updating the cleanup function to
iterate through all remaining timers in timers.current, clearing each timeout
and removing the entry from the map before removing the event listener.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8745da03-25de-4307-b4f4-c3863e4b8653
📒 Files selected for processing (88)
.github/workflows/ci.ymlAGENTS.mdchangelogs/cleanup-p0.mdchangelogs/cleanup-p1.mdchangelogs/cleanup-p2.mdchangelogs/cleanup-p3.mdchangelogs/cleanup-p4.mdchangelogs/cleanup-p5.mdchangelogs/v2.1.0.mddocs/cleanup/README.mddocs/cleanup/findings.mddocs/cleanup/implementation-plan.mdelectron/db/graph-queries.tselectron/db/queries.tselectron/ipc/agent.tselectron/ipc/ai-handlers.tselectron/ipc/chat-db-handlers.tselectron/ipc/db-handlers.tselectron/ipc/flow-handlers.tselectron/ipc/graph-handlers.tselectron/ipc/handlers.tselectron/ipc/llama-handlers.tselectron/ipc/migration-handlers.tselectron/ipc/mobile-handlers.tselectron/ipc/pdf-export.tselectron/ipc/pi-agent.tselectron/ipc/pi-session-handlers.tselectron/ipc/registry.tselectron/ipc/result-helpers.tselectron/ipc/settings-handlers.tselectron/ipc/tool-parity.test.tselectron/ipc/url-metadata.tselectron/lib/config-cache.tselectron/lib/context-audit.test.tselectron/lib/llama-server.tselectron/lib/mobile-server.tselectron/lib/pdf-template.tselectron/lib/tools.tselectron/lib/url-metadata.tselectron/mcp/db.tselectron/mcp/tools/dashboards.tselectron/mcp/tools/flow.tselectron/mcp/tools/graph.tselectron/mcp/tools/notes.tselectron/mcp/tools/projects.tselectron/mcp/tools/tags.tselectron/mcp/tools/tasks.tspackage.jsonscripts/build.jsscripts/check-tool-parity.tssrc/app/page.tsxsrc/components/agent/AgentChatPane.tsxsrc/components/agent/AgentMessageBubble.tsxsrc/components/agent/FileEditorInner.tsxsrc/components/agent/SessionPane.tsxsrc/components/chat/chat-panel/ChatMessageBubble.tsxsrc/components/chat/chat-panel/index.tsxsrc/components/graph/AnalyticsShared.tsxsrc/components/graph/BeeswarmCanvas.tsxsrc/components/graph/BulletCanvas.tsxsrc/components/graph/ForceGraphCanvas.tsxsrc/components/graph/MatrixCanvas.tsxsrc/components/graph/RadialTreeCanvas.tsxsrc/components/graph/RidgelineCanvas.tsxsrc/components/graph/SankeyCanvas.tsxsrc/components/graph/TableCanvas.tsxsrc/components/graph/TimelineCanvas.tsxsrc/components/graph/analyticsHooks.tssrc/components/graph/analyticsUtils.tssrc/components/graph/graph-ai-utils.test.tssrc/components/graph/graphUtils.tssrc/components/layout/app-chrome.tsxsrc/components/layout/project-overview/index.tsxsrc/components/layout/project-overview/primitives.tsxsrc/components/layout/project-overview/sections.tsxsrc/components/notes/DashboardApiModal.tsxsrc/components/notes/markdown-editor.tsxsrc/components/notes/notes-view/MoveNoteModal.tsxsrc/components/shared/cairn-ref-chip.tsxsrc/components/ui/modal-shell.tsxsrc/hooks/useIpcErrorToasts.tssrc/lib/editor-theme.tssrc/store/slices/terminal-sessions.tssrc/types/index.tstsconfig.electron.jsontsconfig.mcp.jsonvitest-sqlite-shim.cjsvitest.config.ts
💤 Files with no reviewable changes (5)
- src/components/graph/graphUtils.ts
- scripts/check-tool-parity.ts
- tsconfig.mcp.json
- electron/ipc/tool-parity.test.ts
- src/components/graph/AnalyticsShared.tsx
…hanges - Replace all 'inlined SQL / ABI boundary' claims with correct guidance: queries.ts is the single SQL source, imported from MCP tools freely - Update IPC structure: handlers.ts is now a thin orchestrator with 12 per-domain registrars instead of 'All db:* and app:* IPC channels' - Update type-check commands: type-check → type-check:all everywhere (CI, CONTRIBUTING, README, PR template) - Delete references to deleted files: graphUtils.ts (merged into analyticsUtils.ts), tool-parity.test.ts, editorTheme.ts (moved to lib/editor-theme.ts), stale mcp-native/ notes - Update graph shared modules list: resolveCssVar + useRelativePointer + useNow are now part of the shared analytics toolkit - Update store slices list: add terminal-sessions, coding-agents - Update project structure tree: reflect new electron/ipc/ files, shared/ folder, chat-panel/index.tsx, hooks/useIpcErrorToasts - Update PR checklist: ABI constraint replaced with 'new SQL goes in queries.ts; never construct Database outside 2 bootstrap sites'
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/hooks/useIpcErrorToasts.ts (1)
29-57: 💤 Low valueCapture
timers.currentat effect start to satisfy React lint rule.The cleanup function references
timers.current, which could theoretically change by cleanup time. While safe here (the Map reference never changes), capturing it follows React best practices and silences the lint warning.♻️ Proposed fix
useEffect(() => { + const timerMap = timers.current; function onIpcError(e: Event) { const message = (e as CustomEvent<{ message: string }>).detail.message; const id = ++_toastSeq; setToasts((prev) => { const next = [...prev.slice(-4), { id, message }]; // keep at most 5 // Clear timers for evicted toasts to prevent leaks const evicted = prev.slice(0, -4); for (const t of evicted) { - const timer = timers.current.get(t.id); + const timer = timerMap.get(t.id); if (timer) { clearTimeout(timer); - timers.current.delete(t.id); + timerMap.delete(t.id); } } return next; }); - timers.current.set(id, setTimeout(() => dismiss(id), 5000)); + timerMap.set(id, setTimeout(() => dismiss(id), 5000)); } window.addEventListener("cairn:ipc-error", onIpcError); return () => { window.removeEventListener("cairn:ipc-error", onIpcError); // Clear all remaining timers on unmount - for (const timer of timers.current.values()) { + for (const timer of timerMap.values()) { clearTimeout(timer); } - timers.current.clear(); + timerMap.clear(); }; }, [dismiss]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/hooks/useIpcErrorToasts.ts` around lines 29 - 57, In the useIpcErrorToasts hook, capture the timers.current reference at the start of the useEffect to satisfy React lint rules. Add a local variable assignment to store timers.current at the beginning of the effect, before the onIpcError function definition, then use this captured variable in the cleanup function's for loop and clear operation instead of directly accessing timers.current.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CONTRIBUTING.md`:
- Around line 500-501: The documentation references electron/db/client.ts as the
required location for constructing Database instances in the Electron context
(lines 500-501), but this file is not included in the directory tree documented
at lines 100-105. Add electron/db/client.ts to the directory tree listing under
the electron/db/ directory section to keep the documentation consistent and
complete. Ensure it is placed alongside other files in that directory structure.
In `@electron/mcp/tools/dashboards.ts`:
- Around line 32-34: The return statement on line 34 returns the stale
`note.updatedAt` from the pre-update snapshot instead of using the freshly
updated timestamp. Capture the return value from the q.updateNote call on line
32 by assigning it to a variable (for example, `const updatedNote =
q.updateNote(...)`), and then use the `updatedAt` property from the returned
updatedNote object in the return statement instead of using the original
`note.updatedAt`. This ensures MCP clients receive the actual updated timestamp
from the database write operation.
In `@electron/mcp/tools/tasks.ts`:
- Line 213: The variable `now` is assigned the result of calling `ts()` but is
never referenced or used anywhere in the code. Remove the entire line containing
the assignment `const now = ts();` as it is unused code that can be safely
deleted.
---
Nitpick comments:
In `@src/hooks/useIpcErrorToasts.ts`:
- Around line 29-57: In the useIpcErrorToasts hook, capture the timers.current
reference at the start of the useEffect to satisfy React lint rules. Add a local
variable assignment to store timers.current at the beginning of the effect,
before the onIpcError function definition, then use this captured variable in
the cleanup function's for loop and clear operation instead of directly
accessing timers.current.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a13d8a28-7c5f-4f7e-be15-c69888b95a94
📒 Files selected for processing (18)
.github/PULL_REQUEST_TEMPLATE.mdAGENTS.mdCONTRIBUTING.mdREADME.mdelectron/ipc/db-handlers.tselectron/ipc/flow-handlers.tselectron/ipc/pdf-export.tselectron/ipc/settings-handlers.tselectron/lib/config-cache.tselectron/lib/url-metadata.tselectron/mcp/tools/dashboards.tselectron/mcp/tools/flow.tselectron/mcp/tools/tasks.tssrc/components/graph/MatrixCanvas.tsxsrc/components/graph/analyticsUtils.tssrc/components/layout/app-chrome.tsxsrc/components/layout/project-overview/sections.tsxsrc/hooks/useIpcErrorToasts.ts
✅ Files skipped from review due to trivial changes (3)
- README.md
- .github/PULL_REQUEST_TEMPLATE.md
- AGENTS.md
🚧 Files skipped from review as they are similar to previous changes (10)
- src/components/graph/MatrixCanvas.tsx
- electron/ipc/settings-handlers.ts
- electron/ipc/pdf-export.ts
- src/components/layout/app-chrome.tsx
- electron/lib/url-metadata.ts
- src/components/graph/analyticsUtils.ts
- electron/ipc/flow-handlers.ts
- electron/mcp/tools/flow.ts
- src/components/layout/project-overview/sections.tsx
- electron/ipc/db-handlers.ts
…n fixes Inline findings (all verified against current code): VALID + FIXED: - db-handlers: reverse project delete order (DB first, then files) so if DB fails, .md files survive for recovery - db-handlers: wrap default columns creation in a transaction - db-handlers: fix revealAssets folder name (assets→attachments) - flow-handlers: validate node type is ai_summary before summarizing - flow-handlers: wrap resolveAiConfig error in err() for IpcResult consistency - pdf-export: wrap BrowserWindow in try/finally for cleanup on error - pdf-export: replace fs.writeFileSync with async fs.promises.writeFile - config-cache: persist maxSteps/temperature in agent save branch - mcp/tools/flow.ts: validate target node belongs to same flow before creating edge (cross-flow edge prevention) - mcp/tools/flow.ts: only report edge IDs that actually exist in DB (check INSERT OR IGNORE result) - mcp/tools/tasks.ts: fix dueDate=null branch to return updated card (not stale clearCardDueDate result) + blocker clearing on done-column move - dashboards.ts: include updatedAt in update_dashboard return - url-metadata: implement documented hostname fallback in title - analyticsUtils: fix PRIORITY_SORT_ORDER JSDoc (was 'low→urgent', should be 'urgent→low') - MatrixCanvas: wrap CanvasEmptyState in relative container - app-chrome: replace slash-alpha borders with color-mix; replace text-white with var(--background) - sections.tsx: replace text-[10.5px] with text-[0.75rem] - useIpcErrorToasts: clear timers for evicted toasts + all timers on unmount - settings-handlers: use ?? instead of || for fontScale (0 is valid) SKIPPED (with reasons): - SSRF host validation in url-metadata.ts: out of scope — the feature fetches arbitrary user-provided URLs by design; the Electron main process has no CORS restrictions intentionally. Adding host allowlists would break the feature (users link to localhost dev servers). The existing 8s timeout + 50KB limit are the right mitigations. - pdf-template.ts hardcoded hex colors: intentional — the PDF is a self-contained document rendered in a hidden BrowserWindow with no access to the app's CSS variables (it's loaded via data: URL). The hex values are a fixed light-theme palette for PDF output regardless of app theme. - analyticsUtils #888 fallback: intentional — this is the SSR fallback for when document is undefined (server-side rendering / vitest). CSS variables don't exist in that context. - cairn-ref-chip slash-alpha + hex: these were pre-existing in the original AgentMessageBubble and ChatMessageBubble; the extraction moved them verbatim. Fixing them is a valid follow-up but would change visual behaviour and should be done separately.
…ebar Extracted the metadata sidebar (priority, column, assignee, blockers, due date, created/updated meta, action buttons) into card-detail-sidebar.tsx (275 LOC). The main modal keeps title, description editor, tags, and linked notes. Also fixed slash-alpha border syntax → color-mix in the sidebar. type-check:all, lint, 430 tests pass.
card-detail.tsx: 550→232 LOC (−58%) — extracted CardDetailSidebar (sidebar metadata: priority, column, assignee, blockers, due date, actions) notes-view.tsx: 1033→532 LOC (−49%) — extracted 5 sub-components: - buildFolderTree.ts (46 LOC) — pure folder tree builder + FolderNode type - NoteListItem.tsx (75 LOC) — memo'd note row with dropdown menu - ArchivedNoteListItem.tsx (41 LOC) — archived note row - FolderTreeNode.tsx (130 LOC) — recursive folder tree node - FolderPickerDialog.tsx (221 LOC) — folder picker dialog + recursive node Also fixed text-[10.5px] → text-[0.75rem] in archived toggle, and slash-alpha border syntax in blocker chips. type-check:all, lint, 430 tests pass.
The DatePicker popover was position:absolute, which gets clipped by parent containers with overflow-y-auto (e.g. the card-detail sidebar inside the Dialog modal). Changed to position:fixed with viewport-aware positioning calculated from the trigger button's getBoundingClientRect. The popover now uses z-[60] (above the modal's z-50) and clamps to the viewport width so it doesn't overflow the right edge. This was a pre-existing issue — the card detail modal's sidebar has overflow-y-auto which clips absolutely-positioned children. The fix applies to all DatePicker uses, not just the card detail.
The sidebar in card-detail is on the right side of the dialog, so the DatePicker trigger is near the window's right edge. The previous fix (absolute→fixed) still clamped left from the right window edge, making the popover appear far from the trigger. Now detects available room and grows leftward (right-aligned to trigger's right edge) when there isn't enough space on the right.
The root cause: Radix Dialog's DialogContent uses transform: translate(-50%, -50%) for centering, which creates a CSS containing block that makes position:fixed descendants positioned relative to the dialog (not the viewport). Combined with the sidebar's overflow-y-auto, the popover was both clipped AND mispositioned. Replaced the inline fixed-position div with createPortal(popover, document.body) so the popover renders at the document body level, completely escaping the dialog's transform containing block and any overflow-clipped ancestors. z-index raised to z-[100] (above modal's z-50) to ensure it renders on top. Also fixed text-white → text-[var(--background)] for the selected day text, and simplified positioning to always use left=near-trigger.left with right-edge clamping (no more separate left/right branches).
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
… + hook Extracted: - SessionMount.tsx (152 LOC) — xterm terminal mount for one PTY session - AIChatTab.tsx (168 LOC) — pinned AI Chat tab + thread history dropdown - AgentSessionTab.tsx (147 LOC) — pinned Cairn Agent tab + session dropdown - TerminalTab.tsx (75 LOC) — single PTY session tab button - AgentEmptyState.tsx (70 LOC) — no-session empty state with recent list - useAgentSessionActions.ts (96 LOC) — shared create/resume session hook - sessionUtils.ts (10 LOC) — formatDate helper SessionPane.tsx is now a 269-LOC orchestrator that composes tab bar + content switching + top-level effects. type-check:all, lint, 430 tests pass.
flow-view.tsx: 1019→862 LOC — extracted pure functions (computeGroupAssignments, flowNodeToRF, flowEdgeToRF, defaultData, type registries, ADD_NODE_MENU, CONNECTION_LINE_STYLE) into flow-utils.ts (146 LOC). Also fixed hardcoded #6366f1 → var(--accent) and text-white → text-[var(--background)]. note-editor.tsx: 1283→932 LOC — extracted 3 sub-components: - BacklinksPanel.tsx (225 LOC) — BacklinksPanel + NoteTagBar - MDPreviewPanel.tsx (97 LOC) — markdown preview panel - note-editor-utils.ts (31 LOC) — countWords + stripMarkdown helpers type-check:all, lint, 430 tests pass.
Complete changelog covering all 6 phases + PR review findings: - P0: hygiene, CI type-check, tsconfig, esbuild target, native binding - P1: SQL consolidation (~216 LOC + ~400 LOC algorithm dedup) - P2: IPC handlers.ts decomposition (1054→170 LOC, 12 registrars) - P3: component extractions (ModalShell, CairnRefChip, editor-theme move, folder refactors, graphUtils merge, useIpcErrorToasts) - P4: canvas consistency (priority colours, dead props, hooks, SvgTimeAxis, empty states) - P5: 6 god-component splits (card-detail, notes-view, SessionPane, flow-view, note-editor, project-overview) - PR review: data safety (project delete order, transaction wrap, cross-flow validation, phantom edge IDs), resource cleanup (PDF BrowserWindow try/finally), DatePicker createPortal fix, config-cache persistence, convention fixes Also removed cleanup-p0..p5.md and docs/cleanup/README.md (moved outside repo).
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (3)
src/components/agent/useAgentSessionActions.ts (1)
49-49: 💤 Low valueConsider logging swallowed errors in development.
Silent
catch { /* ok */ }blocks mask potential issues during development. While the fire-and-forget pattern is reasonable for resilience, logging at debug level would aid troubleshooting.- try { await window.electron?.piAgent.createSession(summary); } catch { /* ok */ } + try { await window.electron?.piAgent.createSession(summary); } catch (e) { console.debug("createSession failed:", e); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/agent/useAgentSessionActions.ts` at line 49, The catch block for the window.electron.piAgent.createSession call in useAgentSessionActions.ts is silently swallowing errors without any logging. Replace the empty catch block with a debug-level log statement that captures and logs the error details, allowing developers to troubleshoot issues during development while maintaining the fire-and-forget resilience pattern of the operation.src/components/ui/date-picker.tsx (1)
53-68: 💤 Low valueConsider vertical viewport clamping for triggers near the bottom edge.
The horizontal clamping is thorough, but if the trigger is near the viewport bottom, the popover may be clipped. Opening upward when space below is insufficient would improve UX.
function positionAndOpen() { const rect = triggerRef.current?.getBoundingClientRect(); if (rect) { const popoverW = 260; // approximate DayPicker width + const popoverH = 300; // approximate DayPicker height // Default: left edge of popover = trigger's left edge let left = rect.left; // Clamp: if popover would overflow right viewport edge, shift left if (left + popoverW > window.innerWidth - 8) { left = window.innerWidth - popoverW - 8; } // Clamp: never go past left viewport edge if (left < 8) left = 8; - setPopoverPos({ top: rect.bottom + 4, left }); + // Vertical: open below if space, otherwise above + const spaceBelow = window.innerHeight - rect.bottom; + const top = spaceBelow >= popoverH + 8 + ? rect.bottom + 4 + : rect.top - popoverH - 4; + setPopoverPos({ top, left }); } setOpen((o) => !o); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/ui/date-picker.tsx` around lines 53 - 68, The positionAndOpen function currently handles horizontal clamping to keep the popover within the viewport width, but it lacks vertical clamping. Add logic to check if there is sufficient space below the trigger for the popover (approximate height of DayPicker is around 330-350 pixels). If the popover would be clipped at the bottom of the viewport, position it above the trigger instead by setting the top value to rect.top minus the popover height minus a small gap. Also ensure the popover never extends above the viewport top by clamping the top value to a minimum of 8, similar to the horizontal left edge clamping.src/components/notes/notes-view/FolderPickerDialog.tsx (1)
137-150: ⚡ Quick winTrim unused
FolderPickerNodeprops to avoid a misleading API surface.
FolderPickerNodedoesn’t use several declared props, and recursion currently passes dummy no-op values. Tightening this interface will reduce future bugs during edits.Suggested refactor
interface FolderPickerNodeProps { node: FolderNode; currentFolder: string; depth: number; mode: "move" | "create"; addingUnder: string | null; - newFolderName: string; - newInputRef: React.RefObject<HTMLInputElement | null>; onSelect: (folder: string) => void; onOpenAdd: (parentPath: string, e: React.MouseEvent) => void; - onNameChange: (v: string) => void; - onCreate: () => void; - onCancel: () => void; inlineInput: React.ReactNode; } @@ {open && node.children.map((child) => ( <FolderPickerNode key={child.path} node={child} currentFolder={currentFolder} depth={depth + 1} mode={mode} addingUnder={addingUnder} - newFolderName="" - newInputRef={{ current: null }} onSelect={onSelect} onOpenAdd={onOpenAdd} - onNameChange={() => {}} - onCreate={() => {}} - onCancel={() => {}} inlineInput={inlineInput} /> ))}Also applies to: 202-216
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/notes/notes-view/FolderPickerDialog.tsx` around lines 137 - 150, The FolderPickerNodeProps interface contains props that are not actually used by the FolderPickerNode component, and dummy no-op values are currently passed for these unused props during recursion. Identify which props in the FolderPickerNodeProps interface are actually used by the FolderPickerNode component and remove the unused ones from the interface definition. Then update all places where FolderPickerNodeProps is referenced, including the recursive component calls that currently pass dummy values, to remove the unused prop parameters and ensure consistency with the streamlined interface.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/agent/AgentEmptyState.tsx`:
- Around line 54-64: The ArrowRight component uses group-hover:opacity-100 on
line 63, but the parent button element on line 54 is missing the "group" class
in its className. Add "group" to the className of the button element that
contains "w-full flex items-center gap-2 px-2.5 py-1.5..." to establish the
group context, which will enable the group-hover selector to properly show the
arrow when hovering over the button.
In `@src/components/agent/AgentSessionTab.tsx`:
- Around line 67-91: The button component in AgentSessionTab contains a nested
interactive element - the span with role="button" that handles the dropdown
toggle is nested inside the parent button element with onClick={onActivate}.
This creates invalid interactive nesting and blocks keyboard access. Restructure
the layout so the dropdown toggle span and the main tab button are siblings at
the same level rather than nested, allowing both interactive controls to be
properly accessible via keyboard. This structural change should be applied to
both instances mentioned (the main area around lines 67-91 and the related area
around lines 106-139).
In `@src/components/agent/sessionUtils.ts`:
- Around line 4-9: The formatSessionDate function (or equivalent method handling
the date calculation) has an issue with millisecond-based day difference
calculation that can produce incorrect labels near midnight boundaries and
doesn't handle future or invalid timestamps. To fix this, first validate that
the Date object created from the iso string is valid by checking if it's NaN,
then add a check to handle cases where diffDays is negative or the date is
invalid (either return the formatted date as a fallback or an appropriate
message), and consider using proper calendar-day boundary calculations instead
of simple millisecond arithmetic division to ensure consistent behavior across
midnight transitions.
In `@src/components/agent/TerminalTab.tsx`:
- Around line 17-25: The button element with onClick={onActivate} contains an
onAuxClick handler that implements close functionality, creating a nested
interactive element which violates HTML standards and breaks accessibility.
Extract the close control into a separate interactive element (such as a
distinct button) positioned alongside the main tab button rather than nested
inside it, and remove the onAuxClick handler from the primary button. This same
pattern appears in another location in the file (around lines 63-73) and should
be refactored similarly to ensure proper keyboard and screen-reader behavior
throughout the TerminalTab component.
In `@src/components/notes/MDPreviewPanel.tsx`:
- Around line 42-47: The dismiss button in the MDPreviewPanel component that
displays only an X icon is missing accessibility information for screen readers.
Add an aria-label attribute to the button element (the one with
onClick={onDismiss}) with a descriptive label such as "Close preview" or
"Dismiss preview" to make the button's purpose clear to assistive technologies.
In `@src/components/notes/note-editor-utils.ts`:
- Around line 9-10: In the countWords() function, the order of regex
replacements is causing image alt text to be incorrectly counted as words
because the link pattern regex is matching before the image pattern regex. Swap
the order of the two replace calls so that the image syntax replacement (the
regex pattern starting with exclamation mark for ``) is executed
first, before the link syntax replacement (the pattern for `[text](url)`). This
ensures more specific image patterns are processed and removed before attempting
to match the more general link patterns.
In `@src/components/notes/notes-view/ArchivedNoteListItem.tsx`:
- Around line 17-27: Icon-only buttons must have aria-label attributes for
screen reader accessibility. Add an aria-label attribute to the restore button
(the one containing ArchiveRestore icon) with a descriptive label like "Restore
note", and add an aria-label to the dropdown menu trigger button (the one
containing MoreHorizontal icon) with a label like "More options". This ensures
users relying on assistive technologies can understand the purpose of these
buttons.
In `@src/components/notes/notes-view/buildFolderTree.ts`:
- Around line 21-30: The folderMap keys are built from normalized folder
segments (using split and filter), but the lookup on line 29 using
folderMap.get(folder) references the raw unnormalized folder string. This causes
mismatches when the folder path contains trailing slashes or double slashes.
Normalize the folder string before using it as the lookup key in folderMap.get()
by applying the same split("/").filter(Boolean) normalization logic and
rebuilding the path, so the key matches how it was stored during the loop that
populates folderMap.
In `@src/components/notes/notes-view/NoteListItem.tsx`:
- Around line 41-44: Add an aria-label attribute to the button element
containing the MoreHorizontal icon to make it accessible to screen readers and
keyboard navigation. The aria-label should descriptively explain the button's
purpose, such as "Note actions" or "More note options", to provide context for
users relying on assistive technologies.
---
Nitpick comments:
In `@src/components/agent/useAgentSessionActions.ts`:
- Line 49: The catch block for the window.electron.piAgent.createSession call in
useAgentSessionActions.ts is silently swallowing errors without any logging.
Replace the empty catch block with a debug-level log statement that captures and
logs the error details, allowing developers to troubleshoot issues during
development while maintaining the fire-and-forget resilience pattern of the
operation.
In `@src/components/notes/notes-view/FolderPickerDialog.tsx`:
- Around line 137-150: The FolderPickerNodeProps interface contains props that
are not actually used by the FolderPickerNode component, and dummy no-op values
are currently passed for these unused props during recursion. Identify which
props in the FolderPickerNodeProps interface are actually used by the
FolderPickerNode component and remove the unused ones from the interface
definition. Then update all places where FolderPickerNodeProps is referenced,
including the recursive component calls that currently pass dummy values, to
remove the unused prop parameters and ensure consistency with the streamlined
interface.
In `@src/components/ui/date-picker.tsx`:
- Around line 53-68: The positionAndOpen function currently handles horizontal
clamping to keep the popover within the viewport width, but it lacks vertical
clamping. Add logic to check if there is sufficient space below the trigger for
the popover (approximate height of DayPicker is around 330-350 pixels). If the
popover would be clipped at the bottom of the viewport, position it above the
trigger instead by setting the top value to rect.top minus the popover height
minus a small gap. Also ensure the popover never extends above the viewport top
by clamping the top value to a minimum of 8, similar to the horizontal left edge
clamping.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: df5b46f4-4225-4dcb-8b99-c82ed50e8cef
📒 Files selected for processing (24)
changelogs/v2.1.0.mdsrc/components/agent/AIChatTab.tsxsrc/components/agent/AgentEmptyState.tsxsrc/components/agent/AgentSessionTab.tsxsrc/components/agent/SessionMount.tsxsrc/components/agent/SessionPane.tsxsrc/components/agent/TerminalTab.tsxsrc/components/agent/sessionUtils.tssrc/components/agent/useAgentSessionActions.tssrc/components/flow/flow-utils.tssrc/components/flow/flow-view.tsxsrc/components/kanban/card-detail-sidebar.tsxsrc/components/kanban/card-detail.tsxsrc/components/notes/BacklinksPanel.tsxsrc/components/notes/MDPreviewPanel.tsxsrc/components/notes/note-editor-utils.tssrc/components/notes/note-editor.tsxsrc/components/notes/notes-view.tsxsrc/components/notes/notes-view/ArchivedNoteListItem.tsxsrc/components/notes/notes-view/FolderPickerDialog.tsxsrc/components/notes/notes-view/FolderTreeNode.tsxsrc/components/notes/notes-view/NoteListItem.tsxsrc/components/notes/notes-view/buildFolderTree.tssrc/components/ui/date-picker.tsx
…ion, fresh timestamps, a11y fixes
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/agent/TerminalTab.tsx`:
- Around line 42-44: In the TerminalTab.tsx file, the ternary expression
checking session.status is using a color value with a hex fallback that violates
the token-only color rule. Remove the hex color fallback from the first branch
of the ternary operator by changing text-[var(--success,`#22c55e`)] to use only
the CSS variable without the fallback, keeping it as text-[var(--success)]. This
ensures the color styling follows the guideline of using only CSS variable
tokens in the form var(--token) without literal color fallbacks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a418086e-3683-43df-b996-f218f8747705
📒 Files selected for processing (16)
CONTRIBUTING.mdelectron/ipc/flow-handlers.tselectron/lib/url-metadata.tselectron/mcp/tools/dashboards.tssrc/components/agent/AgentEmptyState.tsxsrc/components/agent/AgentSessionTab.tsxsrc/components/agent/TerminalTab.tsxsrc/components/agent/sessionUtils.tssrc/components/agent/useAgentSessionActions.tssrc/components/notes/MDPreviewPanel.tsxsrc/components/notes/note-editor-utils.tssrc/components/notes/notes-view/ArchivedNoteListItem.tsxsrc/components/notes/notes-view/FolderPickerDialog.tsxsrc/components/notes/notes-view/NoteListItem.tsxsrc/components/notes/notes-view/buildFolderTree.tssrc/components/ui/date-picker.tsx
💤 Files with no reviewable changes (1)
- src/components/notes/notes-view/FolderPickerDialog.tsx
✅ Files skipped from review due to trivial changes (1)
- CONTRIBUTING.md
🚧 Files skipped from review as they are similar to previous changes (13)
- src/components/agent/sessionUtils.ts
- src/components/notes/notes-view/buildFolderTree.ts
- src/components/notes/note-editor-utils.ts
- src/components/notes/notes-view/ArchivedNoteListItem.tsx
- src/components/agent/AgentSessionTab.tsx
- electron/mcp/tools/dashboards.ts
- src/components/ui/date-picker.tsx
- src/components/agent/AgentEmptyState.tsx
- electron/lib/url-metadata.ts
- src/components/notes/MDPreviewPanel.tsx
- src/components/notes/notes-view/NoteListItem.tsx
- src/components/agent/useAgentSessionActions.ts
- electron/ipc/flow-handlers.ts
This was referenced Jun 19, 2026
This was referenced Jun 27, 2026
This was referenced Jul 6, 2026
Merged
14 of 17 tasks
15 of 17 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
A full-codebase architectural deep dive (documented in
docs/cleanup/) identified and resolved the biggest sources of complexity, duplication, and dead code across 6 phases (P0–P5). This PR removes 3,700+ lines of duplicated/dead code, fixes 142 silently-accumulating electron type errors, consolidates ~1000 lines of duplicated SQL across the Electron↔MCP boundary, splits the 1054-line IPC god-file into 12 domain registrars, and makes the 7 analytics canvases consistently use their own shared scaffold.Type of change
Screenshots / recording
No UI changes — all refactors are behaviour-preserving. The app looks and works identically.
Checklist
npm run type-checkpassesnpm run lintpassesnpm testpassesnpm run test:e2epasses (run before merging UI changes or cutting a release)var(--accent),var(--text-primary), etc.)text-[Npx]pixel font classes — rem equivalents only (text-[0.714rem],text-xs, etc.)handle()and returnIpcResult<T>schema.ts(no new migrations)mcp-server.tschanges use inlined SQL only — no import fromqueries.tsmcp/tools/codebase.tsalready imported fromqueries.tsand worked. P1 extends that pattern to all 7 tool files. The checklist item should be updated to: "Never construct aDatabaseinstance outsidedb/client.ts(Electron) andmcp-server.ts(MCP runtime)"Notes for reviewer
Phase breakdown (each has its own changelog in
changelogs/cleanup-p*.md):<ModalShell>, sharedCairnRefChip, folder refactorsuseRelativePointer+useNow; Ridgeline→<SvgTimeAxis>Remaining P5 items (card-detail, notes-view, SessionPane, flow-view, note-editor splits) are staged for follow-up PRs per the plan's recommendation to do high-risk splits one at a time.
Verification gates:
npm run type-check:all,npm run lint -- --max-warnings 0,npm run compile,npm test(430 tests across 18 files),npm run smoke-test(4/4) — all green.Summary by CodeRabbit