perf: optimize SessionListItem style recalculation#496
Open
jeffscottward wants to merge 44 commits intoRunMaestro:mainfrom
Open
perf: optimize SessionListItem style recalculation#496jeffscottward wants to merge 44 commits intoRunMaestro:mainfrom
jeffscottward wants to merge 44 commits intoRunMaestro:mainfrom
Conversation
|
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 converts many synchronous file/process APIs to async Promises across core services and tests, makes process spawning asynchronous end-to-end, adds Codex config and session-discovery caches, migrates StatsDB and HistoryManager to async with new DB migrations and indexes, and applies large renderer refactors (memoized styles, component memoization, and UI parsing/highlighting updates). Changes
Sequence Diagram(s)sequenceDiagram
actor App as App Startup
participant Index as main/index.ts
participant WSL as wslDetector.checkWslEnvironment()
participant StatsSingleton as initializeStatsDB()
participant StatsDB as StatsDB.initialize()
participant FSPromises as fs/promises
App->>Index: app.whenReady()
Index->>WSL: await checkWslEnvironment(cwd)
WSL->>FSPromises: await readFile('/proc/version','utf8')
FSPromises-->>WSL: content / throw
WSL-->>Index: boolean
Index->>StatsSingleton: await initializeStatsDB()
StatsSingleton->>StatsDB: await db.initialize()
StatsDB->>FSPromises: await access/mkdir/open as needed
FSPromises-->>StatsDB: responses
StatsDB-->>StatsSingleton: initialized
StatsSingleton-->>Index: done
sequenceDiagram
actor Renderer as Renderer (IPC)
participant HistoryHandler as ipc/history:getAll
participant HistoryMgr as HistoryManager
participant FSPromises as fs/promises
Renderer->>HistoryHandler: invoke 'history:getAll'
HistoryHandler->>HistoryMgr: await getAllEntries()
HistoryMgr->>FSPromises: await readdir/readFile per session (parallel)
FSPromises-->>HistoryMgr: file contents
HistoryMgr-->>HistoryHandler: Promise<HistoryEntry[]>
HistoryHandler-->>Renderer: entries
sequenceDiagram
actor Router as GroupChat Router
participant RouterHandler as route handler
participant ProcMgr as ProcessManager
participant Spawner as ChildProcessSpawner/PtySpawner
participant OSProc as OS process
Router->>RouterHandler: initiate spawn (moderator/participant)
RouterHandler->>ProcMgr: await processManager.spawn(config)
ProcMgr->>Spawner: spawn(config) (async)
Spawner->>OSProc: spawn process / await saveImageToTempFile
OSProc-->>Spawner: spawn result
Spawner-->>ProcMgr: Promise<SpawnResult>
ProcMgr-->>RouterHandler: SpawnResult
RouterHandler->>Router: continue flow
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
src/__tests__/main/stats/stats-db.test.ts (1)
560-582:⚠️ Potential issue | 🟡 MinorTests assert against sync mock but production uses async.
The tests on lines 569 and 581 assert against
mockFsMkdirSync, but after the async migration,initialize()usesfsp.mkdirwhich is mocked bymockFsMkdir. These assertions will pass vacuously (the sync mock is never called) and don't actually verify the intended behavior.🐛 Fix mock assertions
it('should create userData directory if it does not exist', async () => { // Simulate directory not existing mockFsExistsSync.mockReturnValue(false); const { StatsDB } = await import('../../../main/stats'); const db = new StatsDB(); await db.initialize(); - // Verify mkdirSync was called with recursive option - expect(mockFsMkdirSync).toHaveBeenCalledWith(mockUserDataPath, { recursive: true }); + // Verify mkdir (async) was called with recursive option + expect(mockFsMkdir).toHaveBeenCalledWith(mockUserDataPath, { recursive: true }); }); it('should not create directory if it already exists', async () => { // Simulate directory already existing mockFsExistsSync.mockReturnValue(true); const { StatsDB } = await import('../../../main/stats'); const db = new StatsDB(); await db.initialize(); - // Verify mkdirSync was NOT called - expect(mockFsMkdirSync).not.toHaveBeenCalled(); + // Verify mkdir (async) was NOT called + expect(mockFsMkdir).not.toHaveBeenCalled(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/main/stats/stats-db.test.ts` around lines 560 - 582, The tests are asserting against the old synchronous mkdir mock (mockFsMkdirSync) but the production code now uses the async fsp.mkdir mocked as mockFsMkdir; update the two expectations in the StatsDB.initialize tests to assert on mockFsMkdir (not mockFsMkdirSync) being called with mockUserDataPath and { recursive: true } when mockFsExistsSync returns false, and to assert mockFsMkdir was not called when mockFsExistsSync returns true; ensure you reference the StatsDB.initialize flow and keep mockFsExistsSync and mockUserDataPath as the inputs under test.src/main/group-chat/group-chat-router.ts (2)
547-569:⚠️ Potential issue | 🟠 MajorHandle
spawnResult.success === falsebefore proceeding.All four spawn paths treat the operation as successful immediately after await, but none checks
spawnResult.success. That can leave incorrect state transitions (for example, participants marked pending without a real spawned process).💡 Suggested guard (apply to each spawn call site)
const spawnResult = await processManager.spawn({ // ... }); +if (!spawnResult.success) { + throw new Error(`Spawn failed for session ${sessionId}`); +} console.log(`[GroupChat:Debug] Spawn result: ${JSON.stringify(spawnResult)}`);Also applies to: 937-965, 1265-1289, 1465-1485
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/group-chat/group-chat-router.ts` around lines 547 - 569, The code calls processManager.spawn(...) and immediately proceeds without checking spawnResult.success; update each spawn call site (the spawn block that assigns spawnResult, including uses of processManager.spawn, spawnResult, sessionId, and chat.moderatorAgentId) to verify if spawnResult.success is false, and if so log the error (include spawnResult.error/details), revert any state changes made before the spawn (e.g., unmark participants as pending or clear pending process ids), emit an appropriate failure response or throw/return to stop further processing, and ensure no success-path logs or state transitions run when spawnResult.success is false; apply the same guard-and-rollback pattern to the other three spawn blocks referenced in the comment.
1256-1283:⚠️ Potential issue | 🟠 MajorSynthesis spawn path is missing SSH wrapping when
sshRemoteConfigis enabled.The synthesis flow goes straight to
processManager.spawn(...)with local command/cwd, unlike the other group-chat spawn flows that callwrapSpawnWithSsh(...). This causes inconsistent remote execution behavior for synthesis rounds.🔧 Suggested patch pattern for synthesis path
+let spawnCommand = command; +let spawnArgs = finalArgs; +let spawnCwd = os.homedir(); +let spawnPrompt: string | undefined = synthesisPrompt; +let spawnEnvVars = + configResolution.effectiveCustomEnvVars ?? + getCustomEnvVarsCallback?.(chat.moderatorAgentId); + +if (sshStore && chat.moderatorConfig?.sshRemoteConfig) { + const sshWrapped = await wrapSpawnWithSsh( + { + command, + args: finalArgs, + cwd: os.homedir(), + prompt: synthesisPrompt, + customEnvVars: spawnEnvVars, + promptArgs: agent.promptArgs, + noPromptSeparator: agent.noPromptSeparator, + agentBinaryName: agent.binaryName, + }, + chat.moderatorConfig.sshRemoteConfig, + sshStore + ); + spawnCommand = sshWrapped.command; + spawnArgs = sshWrapped.args; + spawnCwd = sshWrapped.cwd; + spawnPrompt = sshWrapped.prompt; + spawnEnvVars = sshWrapped.customEnvVars; +} + const spawnResult = await processManager.spawn({ sessionId, toolType: chat.moderatorAgentId, - cwd: os.homedir(), - command, - args: finalArgs, + cwd: spawnCwd, + command: spawnCommand, + args: spawnArgs, readOnlyMode: true, - prompt: synthesisPrompt, + prompt: spawnPrompt, contextWindow: getContextWindowValue(agent, agentConfigValues), - customEnvVars: - configResolution.effectiveCustomEnvVars ?? - getCustomEnvVarsCallback?.(chat.moderatorAgentId), + customEnvVars: spawnEnvVars, // ... });Based on learnings: "When implementing features that spawn agent processes, support SSH remote execution by checking for sshRemoteConfig, using wrapSpawnWithSsh() from ssh-spawn-wrapper.ts, and passing the SSH store adapter".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/group-chat/group-chat-router.ts` around lines 1256 - 1283, The synthesis spawn path currently calls processManager.spawn(...) directly and ignores chat.moderatorConfig?.sshRemoteConfig; update the logic to mirror other group-chat spawn flows by using wrapSpawnWithSsh(...) from ssh-spawn-wrapper.ts when sshRemoteConfig is present: build the same spawn options you pass into processManager.spawn (including command, finalArgs, cwd, shell/runInShell/sendPromptViaStdin flags, prompt, customEnvVars from configResolution.effectiveCustomEnvVars or getCustomEnvVarsCallback, promptArgs, noPromptSeparator, contextWindow from getContextWindowValue(agent, agentConfigValues), etc.), then call wrapSpawnWithSsh(spawnOptions, chat.moderatorConfig.sshRemoteConfig, <ssh store adapter>) so the wrapped call delegates to processManager.spawn for local runs and to the SSH wrapper for remote runs; ensure getWindowsSpawnConfig and its shell/runInShell/sendPromptViaStdin flags are preserved in the spawnOptions you wrap.src/main/utils/context-groomer.ts (1)
206-337:⚠️ Potential issue | 🔴 CriticalSyntax blocker:
awaitused in non-async Promise executor.Line 321 uses
awaitbut the Promise executor callback (line 206) is not declared asasync. Wrap the spawn logic in an async IIFE with proper error handling.Proposed fix
return new Promise<GroomContextResult>((resolve, reject) => { let responseBuffer = ''; let lastDataTime = Date.now(); let idleCheckInterval: NodeJS.Timeout | null = null; let resolved = false; let chunkCount = 0; let cancelled = false; @@ // Listen for events BEFORE spawning processManager.on('data', onData); processManager.on('exit', onExit); processManager.on('agent-error', onError); - // Spawn the process in batch mode - const spawnResult = await processManager.spawn({ - sessionId: groomerSessionId, - toolType: agentType, - cwd: projectRoot, - command: agent.command, - args: finalArgs, - prompt: prompt, // Triggers batch mode (no PTY) - promptArgs: agent.promptArgs, // For agents using flag-based prompt (e.g., OpenCode -p) - noPromptSeparator: agent.noPromptSeparator, - // Pass SSH config for remote execution support - sessionSshRemoteConfig, - sessionCustomPath, - sessionCustomArgs, - sessionCustomEnvVars, - }); - - if (!spawnResult || spawnResult.pid <= 0) { - cleanup(); - reject(new Error(`Failed to spawn grooming process for ${agentType}`)); - return; - } - - logger.debug('Spawned grooming batch process', LOG_CONTEXT, { - groomerSessionId, - pid: spawnResult.pid, - }); - - // Set up idle check - idleCheckInterval = setInterval(() => { - const idleTime = Date.now() - lastDataTime; - if (idleTime > IDLE_TIMEOUT_MS && responseBuffer.length >= MIN_RESPONSE_LENGTH) { - finishWithResponse('idle timeout with content'); - } - }, 1000); + // Spawn the process in batch mode + void (async () => { + const spawnResult = await processManager.spawn({ + sessionId: groomerSessionId, + toolType: agentType, + cwd: projectRoot, + command: agent.command, + args: finalArgs, + prompt: prompt, // Triggers batch mode (no PTY) + promptArgs: agent.promptArgs, // For agents using flag-based prompt (e.g., OpenCode -p) + noPromptSeparator: agent.noPromptSeparator, + // Pass SSH config for remote execution support + sessionSshRemoteConfig, + sessionCustomPath, + sessionCustomArgs, + sessionCustomEnvVars, + }); + + if (!spawnResult || spawnResult.pid <= 0) { + cleanup(); + reject(new Error(`Failed to spawn grooming process for ${agentType}`)); + return; + } + + logger.debug('Spawned grooming batch process', LOG_CONTEXT, { + groomerSessionId, + pid: spawnResult.pid, + }); + + // Set up idle check + idleCheckInterval = setInterval(() => { + const idleTime = Date.now() - lastDataTime; + if (idleTime > IDLE_TIMEOUT_MS && responseBuffer.length >= MIN_RESPONSE_LENGTH) { + finishWithResponse('idle timeout with content'); + } + }, 1000); + })().catch((error) => { + cleanup(); + if (!resolved) { + resolved = true; + reject(error instanceof Error ? error : new Error(String(error))); + } + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/utils/context-groomer.ts` around lines 206 - 337, The Promise executor is using await (processManager.spawn) but is not async; wrap the spawn logic in an async IIFE (e.g., (async () => { const spawnResult = await processManager.spawn(...); /* existing spawn handling */ })().catch(err => { cleanup(); if (!resolved) { resolved = true; logger.error('Grooming spawn failed', LOG_CONTEXT, { groomerSessionId, error: String(err) }); reject(err); } })); ensure the IIFE uses the same variables/callbacks (onData, onExit, onError, cancelOperation, finishWithResponse, cleanup, activeGroomingSessions) and that any error path performs cleanup, sets resolved/cancelled appropriately, and rejects the outer Promise.src/renderer/components/SessionListItem.tsx (1)
44-50:⚠️ Potential issue | 🟠 MajorProps interface inconsistent with internal usage and call sites.
The
SessionListItemPropsinterface declaresindex: numberandselectedIndex: number(lines 48-50), but:
- Line 109 computes
isSelected = index === selectedIndexinternally- The call site in
AgentSessionsBrowser.tsxpassesisSelecteddirectly instead ofindex/selectedIndexThis creates a mismatch. Based on the AI summary stating the props changed to use
isSelected, the interface should be updated:🔧 Proposed fix to update props interface
export interface SessionListItemProps { /** The Claude session data */ session: ClaudeSession; - /** Zero-based index in the list */ - index: number; - /** Currently selected index for keyboard navigation */ - selectedIndex: number; + /** Whether this item is currently selected */ + isSelected: boolean; /** Whether this session is starred */ isStarred: boolean;And remove line 109 since
isSelectedwould come from props:- const isSelected = index === selectedIndex;Also applies to: 88-109
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/SessionListItem.tsx` around lines 44 - 50, The props interface is inconsistent with usage — update SessionListItemProps to accept isSelected: boolean (remove index and selectedIndex) so it matches how AgentSessionsBrowser.tsx calls SessionListItem, and then remove the internal computed isSelected = index === selectedIndex in the SessionListItem component (the line around the current line 109) so the component uses the passed-in isSelected prop instead.
🧹 Nitpick comments (14)
src/renderer/components/FilePreview.tsx (1)
1616-1667: Consider addingsearchRegexto the dependency array for clarity.The effect checks
!searchRegexat line 1617 but doesn't includesearchRegexin the dependency array. While this works correctly becauseeditModeSearchMatchestransitively depends onsearchRegex, the ESLintexhaustive-depsrule would flag this. Adding it explicitly would improve maintainability and silence the lint warning.Additionally,
editContentin the dependency array is redundant sinceeditModeSearchMatchesalready captures changes to it.♻️ Suggested dependency array refinement
}, [ editModeSearchMatches, currentMatchIndex, isEditableText, markdownEditMode, + searchRegex, searchQuery, - editContent, ]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/FilePreview.tsx` around lines 1616 - 1667, The effect uses searchRegex in its guard but doesn’t include it in the dependency array and also includes redundant editContent; update the useEffect dependencies for the effect that updates editModeSearchMatches/navigation by adding searchRegex and removing editContent so the array reads include editModeSearchMatches, currentMatchIndex, isEditableText, markdownEditMode, searchQuery, searchRegex (refer to useEffect, editModeSearchMatches, currentMatchIndex, searchQuery, searchRegex, editContent, textareaRef in the diff).src/renderer/components/AutoRun.tsx (2)
725-729: Collapse redundant memoization for completed task count.
completedTaskMatchesis only used to read.length, so the extra memo adds indirection without benefit.♻️ Proposed simplification
- const completedTaskMatches = useMemo(() => localContent.match(AUTO_RUN_COMPLETED_TASK_REGEX) || [], [localContent]); - const completedTaskCountFromLocalContent = useMemo( - () => completedTaskMatches.length, - [completedTaskMatches] - ); + const completedTaskCountFromLocalContent = useMemo( + () => localContent.match(AUTO_RUN_COMPLETED_TASK_REGEX)?.length ?? 0, + [localContent] + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/AutoRun.tsx` around lines 725 - 729, completedTaskMatches is memoized but completedTaskCountFromLocalContent only reads its .length, so remove the redundant useMemo: keep the useMemo that computes completedTaskMatches (using localContent.match(AUTO_RUN_COMPLETED_TASK_REGEX) || []) and change completedTaskCountFromLocalContent to a plain derived value (e.g., const completedTaskCountFromLocalContent = completedTaskMatches.length) referencing completedTaskMatches directly; update any imports/usages accordingly (symbols: completedTaskMatches, completedTaskCountFromLocalContent, AUTO_RUN_COMPLETED_TASK_REGEX, localContent).
1082-1102: Avoid recounting matches on every next/prev navigation.Line 1102 includes
currentMatchIndexin the effect dependencies, so navigating results retriggers full match counting even when query/content are unchanged.⚡ Proposed dependency-safe update
useEffect(() => { // Clear any pending count if (searchCountTimeoutRef.current) { clearTimeout(searchCountTimeoutRef.current); } if (searchQueryRegex) { // Debounce the match counting for large documents searchCountTimeoutRef.current = setTimeout(() => { - const matches = localContent.match(searchQueryRegex) || []; - const count = matches ? matches.length : 0; + const count = (localContent.match(searchQueryRegex) || []).length; setTotalMatches(count); - if (count > 0 && currentMatchIndex >= count) { - setCurrentMatchIndex(0); - } + setCurrentMatchIndex((prev) => (count > 0 && prev >= count ? 0 : prev)); }, 150); // Short delay for search responsiveness } else { setTotalMatches(0); setCurrentMatchIndex(0); } return () => { if (searchCountTimeoutRef.current) { clearTimeout(searchCountTimeoutRef.current); } }; - }, [searchQueryRegex, localContent, currentMatchIndex]); + }, [searchQueryRegex, localContent]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/AutoRun.tsx` around lines 1082 - 1102, The effect that counts regex matches is re-run on navigation because currentMatchIndex is included in the dependency array; remove currentMatchIndex from the dependencies of that counting effect and keep only searchQueryRegex and localContent so navigating next/prev doesn't retrigger counting. To preserve the existing behavior that resets the index when the match count becomes 0 or out of range, move that index-reset logic into the same timeout callback (already present) or into a separate effect that depends on totalMatches and currentMatchIndex (use setCurrentMatchIndex with a conditional/fallback), and ensure you still clear searchCountTimeoutRef via the existing cleanup; update references to searchQueryRegex, localContent, searchCountTimeoutRef, setTotalMatches, and setCurrentMatchIndex accordingly.src/main/debug-package/collectors/group-chats.ts (1)
51-80: Consider parallelizing per-file metadata reads.Line 51-80 currently processes chats serially. For larger chat directories, this can become noticeably slow.
⚡ Optional parallelized variant
- for (const file of files) { - if (!file.endsWith('.json') || file.endsWith('.log.json')) { - continue; - } - - const filePath = path.join(groupChatsPath, file); - - try { - const content = await fs.promises.readFile(filePath, 'utf-8'); - const chat = JSON.parse(content); - - // Get corresponding log file for message count - const logPath = path.join(groupChatsPath, `${path.basename(file, '.json')}.log.json`); - const messageCount = await countMessages(logPath); - - const chatInfo: GroupChatInfo = { - id: chat.id || path.basename(file, '.json'), - moderatorAgentId: chat.moderatorAgentId || chat.moderator?.agentId || 'unknown', - participantCount: Array.isArray(chat.participants) ? chat.participants.length : 0, - participants: Array.isArray(chat.participants) - ? chat.participants.map((p: any) => ({ - agentId: p.agentId || 'unknown', - })) - : [], - messageCount, - createdAt: chat.createdAt || 0, - updatedAt: chat.updatedAt || 0, - }; - - groupChats.push(chatInfo); - } catch { - // Skip files that can't be parsed - } - } + const chatFiles = files.filter((file) => file.endsWith('.json') && !file.endsWith('.log.json')); + const results = await Promise.all( + chatFiles.map(async (file): Promise<GroupChatInfo | null> => { + const filePath = path.join(groupChatsPath, file); + try { + const content = await fs.promises.readFile(filePath, 'utf-8'); + const chat = JSON.parse(content); + const logPath = path.join(groupChatsPath, `${path.basename(file, '.json')}.log.json`); + const messageCount = await countMessages(logPath); + return { + id: chat.id || path.basename(file, '.json'), + moderatorAgentId: chat.moderatorAgentId || chat.moderator?.agentId || 'unknown', + participantCount: Array.isArray(chat.participants) ? chat.participants.length : 0, + participants: Array.isArray(chat.participants) + ? chat.participants.map((p: any) => ({ agentId: p.agentId || 'unknown' })) + : [], + messageCount, + createdAt: chat.createdAt || 0, + updatedAt: chat.updatedAt || 0, + }; + } catch { + return null; + } + }) + ); + for (const chatInfo of results) { + if (chatInfo) groupChats.push(chatInfo); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/debug-package/collectors/group-chats.ts` around lines 51 - 80, The loop in group-chats.ts that reads and parses each chat file (iterating over files, building filePath, reading with fs.promises.readFile, parsing JSON, calling countMessages, and pushing GroupChatInfo into groupChats) runs serially and should be parallelized: replace the for-loop with a concurrency-safe parallel approach (e.g., filter files to the .json ones, map them to async worker promises that perform readFile, JSON.parse, call countMessages, build the GroupChatInfo using chat.id, chat.moderatorAgentId/chat.moderator?.agentId, participants, createdAt/updatedAt, etc.), then await Promise.all or Promise.allSettled (or use a p-limit to bound concurrency) and push/collect results into groupChats; also ensure individual file errors are caught so one failure doesn't reject the whole batch (handle per-promise try/catch or use allSettled and log/skips failures).src/__tests__/main/history-manager.test.ts (1)
1057-1118: Add a multi-session-file case forupdateSessionNameByClaudeSessionId.Current tests only validate one session file. Add a case where two session files contain the same
agentSessionIdand assert both files are written.🧪 Suggested test addition
describe('updateSessionNameByClaudeSessionId()', () => { + it('updates matching agentSessionId across multiple session files', async () => { + mockExistsSync.mockReturnValue(true); + mockReaddirSync.mockReturnValue([ + 'sess_a.json' as unknown as fs.Dirent, + 'sess_b.json' as unknown as fs.Dirent, + ]); + + mockReadFileSync.mockImplementation((p: string | fs.PathLike) => { + const s = p.toString(); + if (s.includes('sess_a.json')) { + return createHistoryFileData('sess_a', [ + createMockEntry({ id: 'a1', agentSessionId: 'agent-123', sessionName: 'old' }), + ]); + } + if (s.includes('sess_b.json')) { + return createHistoryFileData('sess_b', [ + createMockEntry({ id: 'b1', agentSessionId: 'agent-123', sessionName: 'old' }), + ]); + } + return '{}'; + }); + + const count = await manager.updateSessionNameByClaudeSessionId('agent-123', 'new-name'); + expect(count).toBe(2); + expect(mockWriteFileSync).toHaveBeenCalledTimes(2); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/main/history-manager.test.ts` around lines 1057 - 1118, Add a new test in history-manager.test.ts for manager.updateSessionNameByClaudeSessionId that exercises multiple session files: mock fs.existsSync to true and fs.readdirSync to return two filenames (e.g., 'sess_a.json' and 'sess_b.json'), make mockReadFileSync return different file contents where both files include entries with the same agentSessionId ('agent-123'), call updateSessionNameByClaudeSessionId('agent-123','new-name'), assert the returned count equals the total updates across both files, and assert mockWriteFileSync was called twice with each written file JSON containing the updated sessionName for entries in both sess_a and sess_b; reference manager.updateSessionNameByClaudeSessionId and verify written payloads by parsing mockWriteFileSync.mock.calls.src/main/history-manager.ts (1)
391-417: Prefer async session listing inside async methods.
getEntriesByProjectPath*andclearByProjectPathare async but still calllistSessionsWithHistorySync(), which keeps blocking filesystem work on the main thread.♻️ Suggested refactor
async getEntriesByProjectPath(projectPath: string): Promise<HistoryEntry[]> { - const sessions = this.listSessionsWithHistorySync(); + const sessions = await this.listSessionsWithHistory(); const entries: HistoryEntry[] = []; ... } async getEntriesByProjectPathPaginated( projectPath: string, options?: PaginationOptions ): Promise<PaginatedResult<HistoryEntry>> { - const sessions = this.listSessionsWithHistorySync(); + const sessions = await this.listSessionsWithHistory(); const entries: HistoryEntry[] = []; ... } async clearByProjectPath(projectPath: string): Promise<void> { - const sessions = this.listSessionsWithHistorySync(); + const sessions = await this.listSessionsWithHistory(); for (const sessionId of sessions) { ... } }Also applies to: 502-504
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/history-manager.ts` around lines 391 - 417, The methods getEntriesByProjectPath, getEntriesByProjectPathPaginated (and clearByProjectPath) are async but call the blocking listSessionsWithHistorySync(); replace that sync call with the non-blocking async session lister (e.g., await listSessionsWithHistory() or the equivalent async method) so filesystem work runs off the main thread, update each method to await the async call, propagate errors as needed, and verify behavior of session iteration using the existing getEntries(sessionId) flow remains unchanged.src/main/stats/stats-db.ts (3)
449-490: Inconsistent sync filesystem usage ingetAvailableBackups().This method still uses synchronous
fs.readdirSyncandfs.statSyncwhile other methods in this class have been migrated to asyncfsp.*equivalents. Consider converting this to async for consistency with the rest of the class, especially since callers may not expect blocking I/O from a method in an otherwise async-first class.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/stats/stats-db.ts` around lines 449 - 490, The getAvailableBackups() method uses blocking fs.readdirSync and fs.statSync while the class uses async fsp APIs; change getAvailableBackups to an async function (e.g., async getAvailableBackups()) and replace readdirSync/statSync with await fsp.readdir(dir) and await fsp.stat(fullPath) (or the existing fsp alias used in this file), update callers if necessary to await the method, preserve the same backup filename regex logic for daily and timestamp matches and keep the try/catch with logger.warn(…) on error.
97-99: Simplify early return in async function.Since
initialize()is now async, the early return on line 98 can simply bereturn;orreturn undefined;rather than explicitly returningPromise.resolve(). Async functions automatically wrap return values in a resolved promise.Suggested simplification
async initialize(): Promise<void> { if (this.initialized) { - return Promise.resolve(); + return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/stats/stats-db.ts` around lines 97 - 99, The initialize() async method uses an explicit Promise.resolve() for an early return; replace the explicit Promise.resolve() with a plain return (or return undefined) to let the async function implicitly return a resolved promise—update the check in initialize() that references this.initialized to simply do "if (this.initialized) return;" so the behavior remains the same but is idiomatic for async functions.
496-534:restoreFromBackup()uses sync filesystem calls.This method uses
fs.existsSync,fs.unlinkSync, andfs.copyFileSyncwhile the rest of the class has migrated to async patterns. The method returnsbooleansynchronously, which limits its integration with async flows. Consider converting to async and returningPromise<boolean>for consistency with other backup-related methods likebackupDatabase().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/stats/stats-db.ts` around lines 496 - 534, The restoreFromBackup function uses synchronous fs calls and returns boolean; change it to async restoreFromBackup(backupPath: string): Promise<boolean> and convert all sync operations to promises (use fs.promises.access/stat instead of existsSync, fs.promises.unlink instead of unlinkSync, fs.promises.copyFile instead of copyFileSync). Ensure you await closing the DB (handle this.db.close() if it returns a promise or wrap in Promise.resolve), set this.db = null and this.initialized = false after close, and wrap the whole flow in try/catch to log errors with logger.error (preserve LOG_CONTEXT) and return false on failure or true on success; also update any callers to await restoreFromBackup.src/main/ipc/handlers/tabNaming.ts (1)
250-259: Consider handling spawn rejection.Using
voiddiscards the Promise, but ifprocessManager.spawn()rejects (e.g., command not found), the rejection will be unhandled. While the outer try-catch at line 261 catches synchronous errors, it won't catch async rejections from the voided Promise.Consider either awaiting the spawn or adding a
.catch()to handle potential rejections:Option: Add catch handler
- void processManager.spawn({ + processManager.spawn({ sessionId, toolType: config.agentType, cwd, command, args: finalArgs, prompt: fullPrompt, customEnvVars, sendPromptViaStdin: shouldSendPromptViaStdin, - }); + }).catch((err) => { + if (!resolved) { + resolved = true; + clearTimeout(timeoutId); + processManager.off('data', onData); + processManager.off('exit', onExit); + logger.error('Tab naming spawn failed', LOG_CONTEXT, { sessionId, error: String(err) }); + resolve(null); + } + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/ipc/handlers/tabNaming.ts` around lines 250 - 259, The voided call to processManager.spawn({ sessionId, toolType: config.agentType, cwd, command, args: finalArgs, prompt: fullPrompt, customEnvVars, sendPromptViaStdin: shouldSendPromptViaStdin }) can reject and cause an unhandled promise rejection; update the call to handle async failures by either awaiting the promise inside the surrounding async function or chaining a .catch(...) handler that logs the error and reports it back (e.g., via processLogger or the IPC error response) so spawn rejections (like command-not-found) are handled instead of discarded.src/__tests__/main/ipc/handlers/git.test.ts (1)
3176-3179: Relax the order assertion to avoid brittle concurrency tests.The current assertion requires a strict
main-then-masterlaunch order, which is more implementation-specific than necessary for validating parallel checks.♻️ Suggested assertion tweak
- expect(callOrder).toEqual(['remote', 'main', 'master']); + expect(callOrder[0]).toBe('remote'); + expect(callOrder).toHaveLength(3); + expect(new Set(callOrder.slice(1))).toEqual(new Set(['main', 'master']));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/main/ipc/handlers/git.test.ts` around lines 3176 - 3179, The test is brittle because expect(callOrder).toEqual(['remote', 'main', 'master']) enforces a specific ordering for concurrent launches; update the assertion around callOrder so it only requires that 'remote' was called first and that both 'main' and 'master' were invoked (in any order) thereafter—for example, assert callOrder[0] === 'remote' and that callOrder contains both 'main' and 'master' (or compare sets) instead of exact array equality; locate the failing assertion using the variable name callOrder and the expect(callOrder).toEqual invocation to make the change.src/main/ipc/handlers/agentSessions.ts (1)
442-468: Consider deduplicating concurrent cache misses in session discovery.When cache is stale, simultaneous calls can each trigger full Claude/Codex scans before the first one repopulates cache. A single in-flight Promise guard avoids redundant heavy I/O.
♻️ Proposed refactor
+let sessionDiscoveryInFlight: Promise<{ + claudeFiles: SessionFileInfo[]; + codexFiles: SessionFileInfo[]; +}> | null = null; + async function discoverSessionFilesWithCache(): Promise<{ claudeFiles: SessionFileInfo[]; codexFiles: SessionFileInfo[]; }> { if (isSessionDiscoveryCacheFresh(sessionDiscoveryCache)) { return { claudeFiles: [...sessionDiscoveryCache!.claudeFiles], codexFiles: [...sessionDiscoveryCache!.codexFiles], }; } - const [claudeFiles, codexFiles] = await Promise.all([ - discoverClaudeSessionFiles(), - discoverCodexSessionFiles(), - ]); - - sessionDiscoveryCache = { - timestampMs: Date.now(), - claudeFiles, - codexFiles, - }; - - return { - claudeFiles: [...claudeFiles], - codexFiles: [...codexFiles], - }; + if (sessionDiscoveryInFlight) { + return sessionDiscoveryInFlight; + } + + sessionDiscoveryInFlight = (async () => { + const [claudeFiles, codexFiles] = await Promise.all([ + discoverClaudeSessionFiles(), + discoverCodexSessionFiles(), + ]); + + sessionDiscoveryCache = { + timestampMs: Date.now(), + claudeFiles, + codexFiles, + }; + + return { + claudeFiles: [...claudeFiles], + codexFiles: [...codexFiles], + }; + })(); + + try { + return await sessionDiscoveryInFlight; + } finally { + sessionDiscoveryInFlight = null; + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/ipc/handlers/agentSessions.ts` around lines 442 - 468, discoverSessionFilesWithCache currently lets concurrent callers trigger duplicate discoverClaudeSessionFiles/discoverCodexSessionFiles work when the cache is stale; add an in-flight promise guard (e.g., a module-scoped sessionDiscoveryPromise) so only the first caller starts Promise.all([discoverClaudeSessionFiles(), discoverCodexSessionFiles()]) and others await that same promise. On cache miss: if sessionDiscoveryPromise exists await it and return the cache snapshot; otherwise set sessionDiscoveryPromise = (the Promise that runs the discovery and populates sessionDiscoveryCache), await it, then clear sessionDiscoveryPromise in finally and return the cloned results. Use the existing symbols discoverSessionFilesWithCache, sessionDiscoveryCache, discoverClaudeSessionFiles, discoverCodexSessionFiles to locate where to add the guard.src/renderer/components/AICommandsPanel.tsx (2)
275-277: Indentation inconsistency.The
onClickhandler on line 275 appears to use spaces instead of tabs, breaking alignment with surrounding code. As per coding guidelines, use tabs for indentation throughout the codebase.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/AICommandsPanel.tsx` around lines 275 - 277, The onClick prop for the add button in AICommandsPanel's JSX (onClick={() => setIsCreating(true)}) uses spaces for indentation causing misalignment; update the indentation to use tabs to match the surrounding JSX formatting, ensuring the line containing onClick and its attributes (including className and style) aligns with sibling props/lines in the AICommandsPanel component and retains the same tab-based indentation pattern as the file.
301-316: Indentation inconsistency in form inputs.Multiple input elements and their closing tags have inconsistent indentation (appears to use spaces instead of tabs). This affects lines 301-316 in the create form and lines 396-410, 416-438 in the edit form.
As per coding guidelines for
src/**/*.{ts,tsx,js,jsx,json}: Use tabs for indentation, not spaces.Also applies to: 396-410
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/AICommandsPanel.tsx` around lines 301 - 316, The JSX inputs in AICommandsPanel (the create and edit forms around the newCommand inputs and their closing tags) use space indentation instead of tabs; update the indentation to tabs for the <input> and surrounding JSX elements (references: newCommand, setNewCommand, commandStyles.fieldBase, and the edit form counterparts) so all lines in the create form and edit form blocks consistently use tabs per project style.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/history-manager.ts`:
- Around line 459-493: The loop in updateSessionNameByClaudeSessionId stops
after updating the first matching file because of the stray "break" after the
write; remove that "break" so the outer loop over parsedSessions continues and
updates all parsed.data.entries matching agentSessionId. Keep the per-file
try/catch and logging (references: parsedSessions, parsed, parsed.filePath,
parsed.data.entries, agentSessionId, sessionName, sessionUpdatedCount,
updatedCount) so each file is written and errors are captured independently.
In `@src/renderer/components/AgentSessionsBrowser.tsx`:
- Line 1491: The call to SessionListItem currently passes isSelected={i ===
selectedIndex} but the SessionListItemProps interface expects index: number and
selectedIndex: number; fix by changing the call site to pass the required props
(e.g. index={i} selectedIndex={selectedIndex}) and remove the isSelected prop,
or alternatively update the SessionListItemProps interface and the
SessionListItem component to accept isSelected: boolean instead of
index/selectedIndex—choose one approach and make corresponding updates to the
SessionListItem component and its usage to keep props consistent (refer to
SessionListItemProps, SessionListItem, selectedIndex, index, isSelected).
In `@src/renderer/components/AutoRun.tsx`:
- Around line 1067-1075: The highlight/search mismatch stems from passing the
untrimmed searchQuery into searchHighlight (via createMarkdownComponents) while
the regex and match counting use trimmedSearchQuery/searchQueryRegex; update all
places that set searchHighlight.query (notably the createMarkdownComponents call
in the component rendering) to pass trimmedSearchQuery instead of searchQuery so
highlightSearchMatches uses the same trimmed input as searchQueryRegex (search
for usages of createMarkdownComponents, searchHighlight, highlightSearchMatches
and replace query: searchQuery with query: trimmedSearchQuery).
In `@src/renderer/components/CsvTableRenderer.tsx`:
- Around line 152-160: The current key generation in CsvTableRenderer (using
offset derived from text.split(searchRegex)) can collide when split yields empty
strings; update the key for the mapped segments to be collision-safe by
combining the running offset with the index (or another stable discriminator) so
each element's key is unique (e.g., use `${offset}-${index}` or similar) when
mapping parts in the function that splits text with searchRegex and computes
offset and isMatch.
- Around line 5-6: The NUMBER_VALUE_REGEX is too permissive (accepts multiple
signs/paren like `(123`, `--10`, `((123))`) causing isNumericValue() to
misclassify values and skew detectColumnAlignments(); tighten NUMBER_VALUE_REGEX
to allow at most one optional leading sign or a single pair of surrounding
parentheses (i.e., either an optional leading '-' or a single surrounding '('
... ')' pair), permit an optional single currency symbol and optional trailing
'%' but disallow repeated '-', '(' or ')' characters and require balanced
parens; update the constant NUMBER_VALUE_REGEX accordingly (so isNumericValue()
uses the stricter pattern) while leaving REGEX_ESCAPE_REGEX unchanged.
In `@src/renderer/components/SessionList.tsx`:
- Around line 1573-1714: The review points out that sessionListStyles.skinny.*
is undefined; update all references that read
sessionListStyles.skinny.skinnyDotFallback,
sessionListStyles.skinny.unreadBadge, and sessionListStyles.skinny.skinnyTooltip
(used in collapsed/skinny mode) to instead read
sessionListStyles.root.skinnyDotFallback, sessionListStyles.root.unreadBadge,
and sessionListStyles.root.skinnyTooltip so they reference the actual style
definitions returned by the useMemo that defines sessionListStyles.
- Around line 894-963: The memo for tooltipStyles can become stale because it
depends on session.contextUsage and the threshold props but only lists
theme.colors in its dependency array; update the useMemo dependencies for
tooltipStyles to include session.contextUsage, contextWarningYellowThreshold,
and contextWarningRedThreshold (in addition to theme.colors) so that
contextUsageColor (from getContextColor) and the width
`${session.contextUsage}%` are recalculated when those values change.
- Around line 121-173: The useMemo for styles includes undefined identifiers
(session.contextUsage, contextWarningYellowThreshold,
contextWarningRedThreshold) in its dependency array which will throw at runtime;
update the dependency array for the useMemo that computes styles (the useMemo
that returns container/textMain/textAccent/... in SessionContextMenu / styles)
to only include the actual used values (e.g., theme.colors or theme) and remove
session.contextUsage, contextWarningYellowThreshold, and
contextWarningRedThreshold from the array.
---
Outside diff comments:
In `@src/__tests__/main/stats/stats-db.test.ts`:
- Around line 560-582: The tests are asserting against the old synchronous mkdir
mock (mockFsMkdirSync) but the production code now uses the async fsp.mkdir
mocked as mockFsMkdir; update the two expectations in the StatsDB.initialize
tests to assert on mockFsMkdir (not mockFsMkdirSync) being called with
mockUserDataPath and { recursive: true } when mockFsExistsSync returns false,
and to assert mockFsMkdir was not called when mockFsExistsSync returns true;
ensure you reference the StatsDB.initialize flow and keep mockFsExistsSync and
mockUserDataPath as the inputs under test.
In `@src/main/group-chat/group-chat-router.ts`:
- Around line 547-569: The code calls processManager.spawn(...) and immediately
proceeds without checking spawnResult.success; update each spawn call site (the
spawn block that assigns spawnResult, including uses of processManager.spawn,
spawnResult, sessionId, and chat.moderatorAgentId) to verify if
spawnResult.success is false, and if so log the error (include
spawnResult.error/details), revert any state changes made before the spawn
(e.g., unmark participants as pending or clear pending process ids), emit an
appropriate failure response or throw/return to stop further processing, and
ensure no success-path logs or state transitions run when spawnResult.success is
false; apply the same guard-and-rollback pattern to the other three spawn blocks
referenced in the comment.
- Around line 1256-1283: The synthesis spawn path currently calls
processManager.spawn(...) directly and ignores
chat.moderatorConfig?.sshRemoteConfig; update the logic to mirror other
group-chat spawn flows by using wrapSpawnWithSsh(...) from ssh-spawn-wrapper.ts
when sshRemoteConfig is present: build the same spawn options you pass into
processManager.spawn (including command, finalArgs, cwd,
shell/runInShell/sendPromptViaStdin flags, prompt, customEnvVars from
configResolution.effectiveCustomEnvVars or getCustomEnvVarsCallback, promptArgs,
noPromptSeparator, contextWindow from getContextWindowValue(agent,
agentConfigValues), etc.), then call wrapSpawnWithSsh(spawnOptions,
chat.moderatorConfig.sshRemoteConfig, <ssh store adapter>) so the wrapped call
delegates to processManager.spawn for local runs and to the SSH wrapper for
remote runs; ensure getWindowsSpawnConfig and its
shell/runInShell/sendPromptViaStdin flags are preserved in the spawnOptions you
wrap.
In `@src/main/utils/context-groomer.ts`:
- Around line 206-337: The Promise executor is using await
(processManager.spawn) but is not async; wrap the spawn logic in an async IIFE
(e.g., (async () => { const spawnResult = await processManager.spawn(...); /*
existing spawn handling */ })().catch(err => { cleanup(); if (!resolved) {
resolved = true; logger.error('Grooming spawn failed', LOG_CONTEXT, {
groomerSessionId, error: String(err) }); reject(err); } })); ensure the IIFE
uses the same variables/callbacks (onData, onExit, onError, cancelOperation,
finishWithResponse, cleanup, activeGroomingSessions) and that any error path
performs cleanup, sets resolved/cancelled appropriately, and rejects the outer
Promise.
In `@src/renderer/components/SessionListItem.tsx`:
- Around line 44-50: The props interface is inconsistent with usage — update
SessionListItemProps to accept isSelected: boolean (remove index and
selectedIndex) so it matches how AgentSessionsBrowser.tsx calls SessionListItem,
and then remove the internal computed isSelected = index === selectedIndex in
the SessionListItem component (the line around the current line 109) so the
component uses the passed-in isSelected prop instead.
---
Nitpick comments:
In `@src/__tests__/main/history-manager.test.ts`:
- Around line 1057-1118: Add a new test in history-manager.test.ts for
manager.updateSessionNameByClaudeSessionId that exercises multiple session
files: mock fs.existsSync to true and fs.readdirSync to return two filenames
(e.g., 'sess_a.json' and 'sess_b.json'), make mockReadFileSync return different
file contents where both files include entries with the same agentSessionId
('agent-123'), call updateSessionNameByClaudeSessionId('agent-123','new-name'),
assert the returned count equals the total updates across both files, and assert
mockWriteFileSync was called twice with each written file JSON containing the
updated sessionName for entries in both sess_a and sess_b; reference
manager.updateSessionNameByClaudeSessionId and verify written payloads by
parsing mockWriteFileSync.mock.calls.
In `@src/__tests__/main/ipc/handlers/git.test.ts`:
- Around line 3176-3179: The test is brittle because
expect(callOrder).toEqual(['remote', 'main', 'master']) enforces a specific
ordering for concurrent launches; update the assertion around callOrder so it
only requires that 'remote' was called first and that both 'main' and 'master'
were invoked (in any order) thereafter—for example, assert callOrder[0] ===
'remote' and that callOrder contains both 'main' and 'master' (or compare sets)
instead of exact array equality; locate the failing assertion using the variable
name callOrder and the expect(callOrder).toEqual invocation to make the change.
In `@src/main/debug-package/collectors/group-chats.ts`:
- Around line 51-80: The loop in group-chats.ts that reads and parses each chat
file (iterating over files, building filePath, reading with
fs.promises.readFile, parsing JSON, calling countMessages, and pushing
GroupChatInfo into groupChats) runs serially and should be parallelized: replace
the for-loop with a concurrency-safe parallel approach (e.g., filter files to
the .json ones, map them to async worker promises that perform readFile,
JSON.parse, call countMessages, build the GroupChatInfo using chat.id,
chat.moderatorAgentId/chat.moderator?.agentId, participants,
createdAt/updatedAt, etc.), then await Promise.all or Promise.allSettled (or use
a p-limit to bound concurrency) and push/collect results into groupChats; also
ensure individual file errors are caught so one failure doesn't reject the whole
batch (handle per-promise try/catch or use allSettled and log/skips failures).
In `@src/main/history-manager.ts`:
- Around line 391-417: The methods getEntriesByProjectPath,
getEntriesByProjectPathPaginated (and clearByProjectPath) are async but call the
blocking listSessionsWithHistorySync(); replace that sync call with the
non-blocking async session lister (e.g., await listSessionsWithHistory() or the
equivalent async method) so filesystem work runs off the main thread, update
each method to await the async call, propagate errors as needed, and verify
behavior of session iteration using the existing getEntries(sessionId) flow
remains unchanged.
In `@src/main/ipc/handlers/agentSessions.ts`:
- Around line 442-468: discoverSessionFilesWithCache currently lets concurrent
callers trigger duplicate discoverClaudeSessionFiles/discoverCodexSessionFiles
work when the cache is stale; add an in-flight promise guard (e.g., a
module-scoped sessionDiscoveryPromise) so only the first caller starts
Promise.all([discoverClaudeSessionFiles(), discoverCodexSessionFiles()]) and
others await that same promise. On cache miss: if sessionDiscoveryPromise exists
await it and return the cache snapshot; otherwise set sessionDiscoveryPromise =
(the Promise that runs the discovery and populates sessionDiscoveryCache), await
it, then clear sessionDiscoveryPromise in finally and return the cloned results.
Use the existing symbols discoverSessionFilesWithCache, sessionDiscoveryCache,
discoverClaudeSessionFiles, discoverCodexSessionFiles to locate where to add the
guard.
In `@src/main/ipc/handlers/tabNaming.ts`:
- Around line 250-259: The voided call to processManager.spawn({ sessionId,
toolType: config.agentType, cwd, command, args: finalArgs, prompt: fullPrompt,
customEnvVars, sendPromptViaStdin: shouldSendPromptViaStdin }) can reject and
cause an unhandled promise rejection; update the call to handle async failures
by either awaiting the promise inside the surrounding async function or chaining
a .catch(...) handler that logs the error and reports it back (e.g., via
processLogger or the IPC error response) so spawn rejections (like
command-not-found) are handled instead of discarded.
In `@src/main/stats/stats-db.ts`:
- Around line 449-490: The getAvailableBackups() method uses blocking
fs.readdirSync and fs.statSync while the class uses async fsp APIs; change
getAvailableBackups to an async function (e.g., async getAvailableBackups()) and
replace readdirSync/statSync with await fsp.readdir(dir) and await
fsp.stat(fullPath) (or the existing fsp alias used in this file), update callers
if necessary to await the method, preserve the same backup filename regex logic
for daily and timestamp matches and keep the try/catch with logger.warn(…) on
error.
- Around line 97-99: The initialize() async method uses an explicit
Promise.resolve() for an early return; replace the explicit Promise.resolve()
with a plain return (or return undefined) to let the async function implicitly
return a resolved promise—update the check in initialize() that references
this.initialized to simply do "if (this.initialized) return;" so the behavior
remains the same but is idiomatic for async functions.
- Around line 496-534: The restoreFromBackup function uses synchronous fs calls
and returns boolean; change it to async restoreFromBackup(backupPath: string):
Promise<boolean> and convert all sync operations to promises (use
fs.promises.access/stat instead of existsSync, fs.promises.unlink instead of
unlinkSync, fs.promises.copyFile instead of copyFileSync). Ensure you await
closing the DB (handle this.db.close() if it returns a promise or wrap in
Promise.resolve), set this.db = null and this.initialized = false after close,
and wrap the whole flow in try/catch to log errors with logger.error (preserve
LOG_CONTEXT) and return false on failure or true on success; also update any
callers to await restoreFromBackup.
In `@src/renderer/components/AICommandsPanel.tsx`:
- Around line 275-277: The onClick prop for the add button in AICommandsPanel's
JSX (onClick={() => setIsCreating(true)}) uses spaces for indentation causing
misalignment; update the indentation to use tabs to match the surrounding JSX
formatting, ensuring the line containing onClick and its attributes (including
className and style) aligns with sibling props/lines in the AICommandsPanel
component and retains the same tab-based indentation pattern as the file.
- Around line 301-316: The JSX inputs in AICommandsPanel (the create and edit
forms around the newCommand inputs and their closing tags) use space indentation
instead of tabs; update the indentation to tabs for the <input> and surrounding
JSX elements (references: newCommand, setNewCommand, commandStyles.fieldBase,
and the edit form counterparts) so all lines in the create form and edit form
blocks consistently use tabs per project style.
In `@src/renderer/components/AutoRun.tsx`:
- Around line 725-729: completedTaskMatches is memoized but
completedTaskCountFromLocalContent only reads its .length, so remove the
redundant useMemo: keep the useMemo that computes completedTaskMatches (using
localContent.match(AUTO_RUN_COMPLETED_TASK_REGEX) || []) and change
completedTaskCountFromLocalContent to a plain derived value (e.g., const
completedTaskCountFromLocalContent = completedTaskMatches.length) referencing
completedTaskMatches directly; update any imports/usages accordingly (symbols:
completedTaskMatches, completedTaskCountFromLocalContent,
AUTO_RUN_COMPLETED_TASK_REGEX, localContent).
- Around line 1082-1102: The effect that counts regex matches is re-run on
navigation because currentMatchIndex is included in the dependency array; remove
currentMatchIndex from the dependencies of that counting effect and keep only
searchQueryRegex and localContent so navigating next/prev doesn't retrigger
counting. To preserve the existing behavior that resets the index when the match
count becomes 0 or out of range, move that index-reset logic into the same
timeout callback (already present) or into a separate effect that depends on
totalMatches and currentMatchIndex (use setCurrentMatchIndex with a
conditional/fallback), and ensure you still clear searchCountTimeoutRef via the
existing cleanup; update references to searchQueryRegex, localContent,
searchCountTimeoutRef, setTotalMatches, and setCurrentMatchIndex accordingly.
In `@src/renderer/components/FilePreview.tsx`:
- Around line 1616-1667: The effect uses searchRegex in its guard but doesn’t
include it in the dependency array and also includes redundant editContent;
update the useEffect dependencies for the effect that updates
editModeSearchMatches/navigation by adding searchRegex and removing editContent
so the array reads include editModeSearchMatches, currentMatchIndex,
isEditableText, markdownEditMode, searchQuery, searchRegex (refer to useEffect,
editModeSearchMatches, currentMatchIndex, searchQuery, searchRegex, editContent,
textareaRef in the diff).
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (56)
src/__tests__/integration/group-chat.integration.test.tssrc/__tests__/main/debug-package/collectors.test.tssrc/__tests__/main/history-manager.test.tssrc/__tests__/main/ipc/handlers/agentSessions.test.tssrc/__tests__/main/ipc/handlers/director-notes.test.tssrc/__tests__/main/ipc/handlers/git.test.tssrc/__tests__/main/ipc/handlers/history.test.tssrc/__tests__/main/parsers/codex-output-parser.test.tssrc/__tests__/main/process-manager/spawners/ChildProcessSpawner.test.tssrc/__tests__/main/stats/aggregations.test.tssrc/__tests__/main/stats/auto-run.test.tssrc/__tests__/main/stats/data-management.test.tssrc/__tests__/main/stats/integration.test.tssrc/__tests__/main/stats/paths.test.tssrc/__tests__/main/stats/query-events.test.tssrc/__tests__/main/stats/stats-db.test.tssrc/__tests__/main/web-server/web-server-factory.test.tssrc/main/debug-package/collectors/group-chats.tssrc/main/debug-package/collectors/storage.tssrc/main/group-chat/group-chat-agent.tssrc/main/group-chat/group-chat-moderator.tssrc/main/group-chat/group-chat-router.tssrc/main/history-manager.tssrc/main/index.tssrc/main/ipc/handlers/agentSessions.tssrc/main/ipc/handlers/director-notes.tssrc/main/ipc/handlers/git.tssrc/main/ipc/handlers/groupChat.tssrc/main/ipc/handlers/history.tssrc/main/ipc/handlers/process.tssrc/main/ipc/handlers/tabNaming.tssrc/main/parsers/codex-output-parser.tssrc/main/parsers/index.tssrc/main/process-manager/ProcessManager.tssrc/main/process-manager/spawners/ChildProcessSpawner.tssrc/main/process-manager/spawners/PtySpawner.tssrc/main/process-manager/utils/imageUtils.tssrc/main/stats/aggregations.tssrc/main/stats/singleton.tssrc/main/stats/stats-db.tssrc/main/utils/context-groomer.tssrc/main/utils/wslDetector.tssrc/main/web-server/WebServer.tssrc/main/web-server/managers/CallbackRegistry.tssrc/main/web-server/routes/apiRoutes.tssrc/main/web-server/types.tssrc/main/web-server/web-server-factory.tssrc/renderer/components/AICommandsPanel.tsxsrc/renderer/components/AgentSessionsBrowser.tsxsrc/renderer/components/AutoRun.tsxsrc/renderer/components/CsvTableRenderer.tsxsrc/renderer/components/ExecutionQueueIndicator.tsxsrc/renderer/components/FilePreview.tsxsrc/renderer/components/SessionList.tsxsrc/renderer/components/SessionListItem.tsxsrc/renderer/components/Toast.tsx
Comment on lines
+459
to
493
| for (const parsed of parsedSessions) { | ||
| if (!parsed) { | ||
| continue; | ||
| } | ||
|
|
||
| try { | ||
| const data: HistoryFileData = JSON.parse(fs.readFileSync(filePath, 'utf-8')); | ||
| let modified = false; | ||
|
|
||
| for (const entry of data.entries) { | ||
| if (entry.agentSessionId === agentSessionId && entry.sessionName !== sessionName) { | ||
| entry.sessionName = sessionName; | ||
| modified = true; | ||
| updatedCount++; | ||
| } | ||
| let sessionUpdatedCount = 0; | ||
| for (const entry of parsed.data.entries) { | ||
| if (entry.agentSessionId === agentSessionId && entry.sessionName !== sessionName) { | ||
| entry.sessionName = sessionName; | ||
| sessionUpdatedCount++; | ||
| updatedCount++; | ||
| } | ||
| } | ||
|
|
||
| if (modified) { | ||
| fs.writeFileSync(filePath, JSON.stringify(data, null, 2), 'utf-8'); | ||
| if (sessionUpdatedCount > 0) { | ||
| try { | ||
| await fs.promises.writeFile( | ||
| parsed.filePath, | ||
| JSON.stringify(parsed.data, null, 2), | ||
| 'utf-8' | ||
| ); | ||
| logger.debug( | ||
| `Updated ${updatedCount} entries for agentSessionId ${agentSessionId} in session ${sessionId}`, | ||
| `Updated ${sessionUpdatedCount} entries for agentSessionId ${agentSessionId} in session ${parsed.sessionId}`, | ||
| LOG_CONTEXT | ||
| ); | ||
| } catch (error) { | ||
| logger.warn( | ||
| `Failed to update sessionName in session ${parsed.sessionId}: ${error}`, | ||
| LOG_CONTEXT | ||
| ); | ||
| captureException(error, { operation: 'history:updateSessionNameWrite', sessionId: parsed.sessionId }); | ||
| } | ||
| } catch (error) { | ||
| logger.warn(`Failed to update sessionName in session ${sessionId}: ${error}`, LOG_CONTEXT); | ||
| captureException(error, { operation: 'history:updateSessionName', sessionId }); | ||
|
|
||
| break; | ||
| } |
There was a problem hiding this comment.
updateSessionNameByClaudeSessionId stops after the first updated file.
Line 492 breaks the outer loop after one matching session file, so later files with the same agentSessionId are not updated.
💡 Proposed fix
if (sessionUpdatedCount > 0) {
try {
await fs.promises.writeFile(
parsed.filePath,
JSON.stringify(parsed.data, null, 2),
'utf-8'
);
logger.debug(
`Updated ${sessionUpdatedCount} entries for agentSessionId ${agentSessionId} in session ${parsed.sessionId}`,
LOG_CONTEXT
);
} catch (error) {
logger.warn(
`Failed to update sessionName in session ${parsed.sessionId}: ${error}`,
LOG_CONTEXT
);
captureException(error, { operation: 'history:updateSessionNameWrite', sessionId: parsed.sessionId });
}
-
- break;
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for (const parsed of parsedSessions) { | |
| if (!parsed) { | |
| continue; | |
| } | |
| try { | |
| const data: HistoryFileData = JSON.parse(fs.readFileSync(filePath, 'utf-8')); | |
| let modified = false; | |
| for (const entry of data.entries) { | |
| if (entry.agentSessionId === agentSessionId && entry.sessionName !== sessionName) { | |
| entry.sessionName = sessionName; | |
| modified = true; | |
| updatedCount++; | |
| } | |
| let sessionUpdatedCount = 0; | |
| for (const entry of parsed.data.entries) { | |
| if (entry.agentSessionId === agentSessionId && entry.sessionName !== sessionName) { | |
| entry.sessionName = sessionName; | |
| sessionUpdatedCount++; | |
| updatedCount++; | |
| } | |
| } | |
| if (modified) { | |
| fs.writeFileSync(filePath, JSON.stringify(data, null, 2), 'utf-8'); | |
| if (sessionUpdatedCount > 0) { | |
| try { | |
| await fs.promises.writeFile( | |
| parsed.filePath, | |
| JSON.stringify(parsed.data, null, 2), | |
| 'utf-8' | |
| ); | |
| logger.debug( | |
| `Updated ${updatedCount} entries for agentSessionId ${agentSessionId} in session ${sessionId}`, | |
| `Updated ${sessionUpdatedCount} entries for agentSessionId ${agentSessionId} in session ${parsed.sessionId}`, | |
| LOG_CONTEXT | |
| ); | |
| } catch (error) { | |
| logger.warn( | |
| `Failed to update sessionName in session ${parsed.sessionId}: ${error}`, | |
| LOG_CONTEXT | |
| ); | |
| captureException(error, { operation: 'history:updateSessionNameWrite', sessionId: parsed.sessionId }); | |
| } | |
| } catch (error) { | |
| logger.warn(`Failed to update sessionName in session ${sessionId}: ${error}`, LOG_CONTEXT); | |
| captureException(error, { operation: 'history:updateSessionName', sessionId }); | |
| break; | |
| } | |
| for (const parsed of parsedSessions) { | |
| if (!parsed) { | |
| continue; | |
| } | |
| let sessionUpdatedCount = 0; | |
| for (const entry of parsed.data.entries) { | |
| if (entry.agentSessionId === agentSessionId && entry.sessionName !== sessionName) { | |
| entry.sessionName = sessionName; | |
| sessionUpdatedCount++; | |
| updatedCount++; | |
| } | |
| } | |
| if (sessionUpdatedCount > 0) { | |
| try { | |
| await fs.promises.writeFile( | |
| parsed.filePath, | |
| JSON.stringify(parsed.data, null, 2), | |
| 'utf-8' | |
| ); | |
| logger.debug( | |
| `Updated ${sessionUpdatedCount} entries for agentSessionId ${agentSessionId} in session ${parsed.sessionId}`, | |
| LOG_CONTEXT | |
| ); | |
| } catch (error) { | |
| logger.warn( | |
| `Failed to update sessionName in session ${parsed.sessionId}: ${error}`, | |
| LOG_CONTEXT | |
| ); | |
| captureException(error, { operation: 'history:updateSessionNameWrite', sessionId: parsed.sessionId }); | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/history-manager.ts` around lines 459 - 493, The loop in
updateSessionNameByClaudeSessionId stops after updating the first matching file
because of the stray "break" after the write; remove that "break" so the outer
loop over parsedSessions continues and updates all parsed.data.entries matching
agentSessionId. Keep the per-file try/catch and logging (references:
parsedSessions, parsed, parsed.filePath, parsed.data.entries, agentSessionId,
sessionName, sessionUpdatedCount, updatedCount) so each file is written and
errors are captured independently.
Greptile SummaryThis PR consolidates inline style objects into Two bugs were introduced during the refactor:
Confidence Score: 2/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[SessionListItem render] --> B{styles useMemo\ncache hit?}
B -- yes --> C[Reuse cached style objects]
B -- no --> D[Recompute styles]
D --> E[theme.colors / isSelected /\nisStarred / session.sessionName]
E --> C
F[SessionList render] --> G[collapsedPillStyles useMemo\ntheme.colors]
F --> H[worktreeListStyles useMemo\ntheme.colors]
F --> I[sessionListStyles useMemo\ntheme.colors]
F --> J[sessionListRenderStyles useMemo\ntheme + tooltipPosition.y\n+ leftSidebarWidthState]
K[AgentSessionsBrowser] -->|isSelected=...| L[SessionListItem\n⚠️ missing index / selectedIndex]
M[discoverSessionFilesWithCache] --> N{sessionDiscoveryCache\nfresh TTL=30s?}
N -- yes --> O[Return shallow copy\nof cached lists]
N -- no --> P[discoverClaudeSessionFiles\n+ discoverCodexSessionFiles\nin parallel]
P --> Q[Store in sessionDiscoveryCache\nwith timestamp]
Q --> O
Last reviewed commit: 5deb587 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| </div> | ||
| <div className="text-xs" style={{ color: theme.colors.textDim }}> | ||
| <div className="text-xs" style={styles.textDim}> | ||
| View application logs |
There was a problem hiding this comment.
Missing useMemo dependencies — context bar will freeze
tooltipStyles.contextUsage computes its width and backgroundColor using session.contextUsage, contextWarningYellowThreshold, and contextWarningRedThreshold, but none of these are listed in the useMemo dependency array (only theme.colors is). This means the context progress bar will be permanently stuck at the value it had on first render; neither the width nor the color will update when context usage changes.
Suggested change
| View application logs | |
| }, [theme.colors, session.contextUsage, contextWarningYellowThreshold, contextWarningRedThreshold]); |
Additional Comments (3)
In JavaScript object literals, a spread applied after an explicit property overrides that property. Here, The
Dozens of test This same pattern also appears throughout the renamed tests in this file — e.g., |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (10)
src/renderer/components/CsvTableRenderer.tsx (1)
300-337:⚠️ Potential issue | 🟡 MinorAdd keyboard accessibility to sortable column headers.
The headers have
onClickhandlers for sorting but lacktabIndexand keyboard event handlers, making them inaccessible to keyboard users.Proposed fix
{headerRow.map((header, i) => ( <th key={i} onClick={() => handleHeaderClick(i)} + onKeyDown={(e) => { + if (e.key === 'Enter' || e.key === ' ') { + e.preventDefault(); + handleHeaderClick(i); + } + }} + tabIndex={0} + role="button" style={{As per coding guidelines: "Add tabIndex attribute and focus event handlers when implementing components that need keyboard focus."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/CsvTableRenderer.tsx` around lines 300 - 337, The header <th> elements rendered in the headerRow mapping are clickable via handleHeaderClick but lack keyboard accessibility; add tabIndex={0} and a keyboard event handler that calls handleHeaderClick(i) when Enter or Space is pressed (e.g., an onKeyDown that checks for key === 'Enter' || key === ' ' / keyCode 13/32), and also add role="button" (and optionally onFocus/onBlur for focus styling) so sortable headers are reachable and operable by keyboard users; update references around headerRow, handleHeaderClick, and sort to ensure behavior and visual focus state match the mouse click interaction.src/renderer/components/SessionList.tsx (1)
174-225:⚠️ Potential issue | 🔴 CriticalVariables
left,top,readyused before declaration.The
menuStylesuseMemo (lines 174-208) referencesleft,top, andreadyin both the computed object and the dependency array, but these are not declared until line 225 viauseContextMenuPosition. This causes a temporal dead zone (TDZ) error at runtime.🐛 Proposed fix — move the hook call before the useMemo
// Use ref to avoid re-registering listener when onDismiss changes const onDismissRef = useRef(onDismiss); onDismissRef.current = onDismiss; + // Measure menu and adjust position to stay within viewport + const { left, top, ready } = useContextMenuPosition(menuRef, x, y); + const styles = useMemo(() => { return { // ... styles definition }; - }, [theme.colors, session.contextUsage, contextWarningYellowThreshold, contextWarningRedThreshold]); + }, [theme.colors]); const menuStyles = useMemo(() => { // ... menuStyles definition using left, top, ready }, [ left, top, ready, // ... other deps ]); // Close on click outside useClickOutside(menuRef, onDismiss); // Close on Escape - stable listener that never re-registers useEffect(() => { // ... }, []); - // Measure menu and adjust position to stay within viewport - const { left, top, ready } = useContextMenuPosition(menuRef, x, y);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/SessionList.tsx` around lines 174 - 225, menuStyles' useMemo references left, top and ready before they are declared which causes a TDZ; move the call to useContextMenuPosition(menuRef, x, y) so that const { left, top, ready } is declared before the useMemo that computes menuStyles, then update the useMemo dependency array to keep left, top and ready (and existing submenuPosition & style refs) so React hooks/readers see the values in scope; ensure useClickOutside(menuRef, onDismiss) and the Escape useEffect remain in place and that no other references to left/top/ready precede the moved hook.src/__tests__/main/stats/stats-db.test.ts (8)
761-766:⚠️ Potential issue | 🔴 CriticalMissing
awaitfor asyncgetAvailableBackups()method.Same issue as above - the async method needs to be awaited.
🐛 Proposed fix
- const backups = db.getAvailableBackups(); + const backups = await db.getAvailableBackups(); expect(backups).toHaveLength(3);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/main/stats/stats-db.test.ts` around lines 761 - 766, The test calls the async method getAvailableBackups() without awaiting it; update the test to await db.getAvailableBackups() (e.g., assign const backups = await db.getAvailableBackups()) so the promise resolves before assertions. Locate the call to getAvailableBackups() in the test block and add the await keyword and ensure the enclosing test function is async if not already.
826-828:⚠️ Potential issue | 🔴 CriticalMissing
awaitfor asyncrestoreFromBackup()method.🐛 Proposed fix
- db.restoreFromBackup('/path/to/backup'); + await db.restoreFromBackup('/path/to/backup'); expect(mockDb.close).toHaveBeenCalled();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/main/stats/stats-db.test.ts` around lines 826 - 828, The test calls the async method restoreFromBackup() without awaiting it, causing the expectation on mockDb.close to run early; update the test so restoreFromBackup() is awaited (or return its promise) before asserting expect(mockDb.close).toHaveBeenCalled(), ensuring the test function is marked async if needed and referencing the restoreFromBackup() call and the mockDb.close assertion in the test.
836-842:⚠️ Potential issue | 🔴 CriticalMissing
awaitfor asyncrestoreFromBackup()method.🐛 Proposed fix
- db.restoreFromBackup('/path/to/backup.db'); + await db.restoreFromBackup('/path/to/backup.db'); - expect(mockFsCopyFileSync).toHaveBeenCalledWith( + expect(mockFsCopyFile).toHaveBeenCalledWith(Note: The assertion should also likely check
mockFsCopyFile(async) instead ofmockFsCopyFileSyncsincerestoreFromBackupnow usesfsp.copyFile.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/main/stats/stats-db.test.ts` around lines 836 - 842, The test calls the async method restoreFromBackup() without awaiting it and still asserts against the sync mock; change the test to await db.restoreFromBackup('/path/to/backup.db') and update the expectation to check the async mock (mockFsCopyFile) was called with '/path/to/backup.db' and a destination containing 'stats.db' (restoreFromBackup uses fsp.copyFile, so assert mockFsCopyFile rather than mockFsCopyFileSync).
778-781:⚠️ Potential issue | 🔴 CriticalMissing
awaitfor asyncgetAvailableBackups()method.🐛 Proposed fix
- const backups = db.getAvailableBackups(); + const backups = await db.getAvailableBackups(); expect(backups).toHaveLength(1);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/main/stats/stats-db.test.ts` around lines 778 - 781, The test is calling the async method getAvailableBackups() without awaiting it; change the test to await db.getAvailableBackups() (e.g., const backups = await db.getAvailableBackups()) and ensure the test function is declared async so the await is valid; update any surrounding test callback to async and keep the rest of the assertions unchanged.
849-853:⚠️ Potential issue | 🔴 CriticalMissing
awaitfor asyncrestoreFromBackup()method.🐛 Proposed fix
- db.restoreFromBackup('/path/to/backup.db'); + await db.restoreFromBackup('/path/to/backup.db'); // Should attempt to unlink WAL and SHM files - expect(mockFsUnlinkSync).toHaveBeenCalled(); + expect(mockFsUnlink).toHaveBeenCalled();Note: Should also check
mockFsUnlink(async) instead ofmockFsUnlinkSync.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/main/stats/stats-db.test.ts` around lines 849 - 853, The test calls the async method restoreFromBackup() without awaiting it and then asserts unlink behavior; change the test to await db.restoreFromBackup(...) to ensure the promise resolves before assertions and replace the sync assertion mockFsUnlinkSync with the async mock assertion mockFsUnlink (or mockFsUnlink.mockResolvedValue...) so the test verifies the asynchronous unlink path; update references to restoreFromBackup and mockFsUnlink/mockFsUnlinkSync in the test accordingly.
952-954:⚠️ Potential issue | 🔴 CriticalMissing
awaitfor asyncbackupDatabase()method.
backupDatabase()now returnsPromise<BackupResult>but the test doesn't await it.🐛 Proposed fix
mockDb.pragma.mockClear(); - db.backupDatabase(); + await db.backupDatabase(); expect(mockDb.pragma).toHaveBeenCalledWith('wal_checkpoint(TRUNCATE)');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/main/stats/stats-db.test.ts` around lines 952 - 954, The test calls db.backupDatabase() but doesn't await the async method; update the test to await db.backupDatabase() (and optionally capture the Promise result if needed) before asserting expect(mockDb.pragma).toHaveBeenCalledWith('wal_checkpoint(TRUNCATE)'); ensure the call to backupDatabase() in the test for stats-db.test.ts is awaited so the async work completes prior to the expectation.
978-980:⚠️ Potential issue | 🔴 CriticalMissing
awaitfor asyncbackupDatabase()method.🐛 Proposed fix
- db.backupDatabase(); + await db.backupDatabase(); expect(callOrder).toEqual(['checkpoint', 'copy']);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/main/stats/stats-db.test.ts` around lines 978 - 980, The test calls db.backupDatabase() without awaiting it, so make the test await the async backupDatabase() call (change db.backupDatabase(); to await db.backupDatabase();) and ensure the surrounding test function is declared async (or returns the Promise) so the expect(callOrder).toEqual(['checkpoint','copy']) runs after completion; update references to backupDatabase and the test function signature accordingly.
799-803:⚠️ Potential issue | 🔴 CriticalMissing
awaitfor asyncgetAvailableBackups()method.🐛 Proposed fix
- const backups = db.getAvailableBackups(); + const backups = await db.getAvailableBackups(); expect(backups[0].date).toBe('2026-02-01');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/main/stats/stats-db.test.ts` around lines 799 - 803, The test calls the async method getAvailableBackups() without awaiting it; update the test to await db.getAvailableBackups() and ensure the surrounding test function is declared async so backups resolves before assertions (reference the getAvailableBackups() call and the backups variable in this test block).
♻️ Duplicate comments (5)
src/renderer/components/AutoRun.tsx (1)
1514-1520:⚠️ Potential issue | 🟡 MinorPass
trimmedSearchQueryinstead ofsearchQuerytosearchHighlight.The match counting at line 1073 uses
trimmedSearchQueryto build the regex, but the highlighting at line 1516 passes the untrimmedsearchQuery. This mismatch means searching for" foo "(with spaces) will count matches for"foo"but attempt to highlight the literal string with spaces, causing inconsistent behavior.🔧 Suggested fix
searchHighlight: { - query: searchQuery, + query: trimmedSearchQuery, currentMatchIndex, onMatchRendered: handleMatchRendered, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/AutoRun.tsx` around lines 1514 - 1520, The searchHighlight object is using the untrimmed variable searchQuery while match counting uses trimmedSearchQuery, causing mismatched highlights for queries with surrounding whitespace; update the searchHighlight call (the object passed where containerRef: previewRef, searchHighlight: {...}) to use trimmedSearchQuery for query (i.e., pass trimmedSearchQuery instead of searchQuery) and leave currentMatchIndex and onMatchRendered (handleMatchRendered) unchanged so counting and highlighting use the same trimmed input.src/renderer/components/SessionList.tsx (4)
172-172:⚠️ Potential issue | 🔴 CriticalRemove undefined identifiers from
useMemodependency array.The dependency array references
session.contextUsage,contextWarningYellowThreshold, andcontextWarningRedThreshold, but these identifiers are either unused in the computed styles (session.contextUsage) or not defined inSessionContextMenu's scope (the threshold variables). This causes a runtimeReferenceError.🐛 Proposed fix
- }, [theme.colors, session.contextUsage, contextWarningYellowThreshold, contextWarningRedThreshold]); + }, [theme.colors]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/SessionList.tsx` at line 172, The useMemo dependency array in SessionList.tsx (within the SessionContextMenu / useMemo call) includes undefined or unused identifiers (session.contextUsage, contextWarningYellowThreshold, contextWarningRedThreshold) causing a ReferenceError; update the dependency array to only include the actual values referenced inside the memoized callback (e.g., theme.colors and any properties of session that the callback reads), removing session.contextUsage and the two threshold names that are not defined in this scope so the dependencies match the used variables.
3220-3220:⚠️ Potential issue | 🔴 CriticalInvalid path
sessionListStyles.skinny.unreadBadge— should be.root..Same issue as above:
sessionListStyleshas noskinnyproperty. This breaks the unread notification badge in collapsed sidebar mode.🐛 Proposed fix
- style={sessionListStyles.skinny.unreadBadge} + style={sessionListStyles.root.unreadBadge}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/SessionList.tsx` at line 3220, The unread badge is referencing a non-existent style path sessionListStyles.skinny.unreadBadge; update the reference to the correct style location sessionListStyles.root.unreadBadge (or move the unreadBadge into the .root object) so the collapsed sidebar uses the valid style key; change any other uses of sessionListStyles.skinny to sessionListStyles.root and ensure the unreadBadge entry exists on sessionListStyles.root in the SessionList component.
1798-1811:⚠️ Potential issue | 🔴 CriticalInvalid style object path:
sessionListStyles.skinnydoes not exist.Lines 1798-1805 and 1807-1811 reference
sessionListStyles.skinny.skinnyDotFallbackandsessionListStyles.skinny.skinnyTooltip, butsessionListStyles(defined at lines 1599-1757) only has arootproperty. Theskinnypath is undefined, causing runtime errors.🐛 Proposed fix
skinny: { dot: { activeByState: skinnyDotActiveByState, inactiveByState: skinnyDotInactiveByState, fallback: { active: { - ...sessionListStyles.skinny.skinnyDotFallback, + ...sessionListStyles.root.skinnyDotFallback, opacity: 1, }, inactive: { - ...sessionListStyles.skinny.skinnyDotFallback, + ...sessionListStyles.root.skinnyDotFallback, opacity: 0.25, }, }, }, tooltip: { - ...sessionListStyles.skinny.skinnyTooltip, + ...sessionListStyles.root.skinnyTooltip, left: '80px', ...(tooltipPosition ? { top: `${tooltipPosition.y}px` } : {}), }, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/SessionList.tsx` around lines 1798 - 1811, sessionListStyles.skinny is undefined causing runtime errors; either add a skinny object to the sessionListStyles definition with skinnyDotFallback and skinnyTooltip keys, or update the references to point to the existing path (e.g. replace sessionListStyles.skinny.skinnyDotFallback and sessionListStyles.skinny.skinnyTooltip with sessionListStyles.root.skinnyDotFallback and sessionListStyles.root.skinnyTooltip). Adjust the code around tooltipPosition (still use tooltipPosition ? { top: `${tooltipPosition.y}px` } : {}) and ensure the names skinnyDotFallback and skinnyTooltip match exactly what you add or reference.
920-988:⚠️ Potential issue | 🟠 Major
tooltipStylesmemo has incomplete dependencies — causes stale context styles.The memoized
tooltipStylescomputescontextUsageColor(lines 921-926) usingsession.contextUsageand the threshold props, and directly usessession.contextUsagefor the width style (line 972). However, the dependency array only includes[theme.colors].When
session.contextUsage,contextWarningYellowThreshold, orcontextWarningRedThresholdchange, the memo won't recalculate, leaving stale context usage colors and bar widths.🐛 Proposed fix
- }, [theme.colors]); + }, [ + theme.colors, + session.contextUsage, + contextWarningYellowThreshold, + contextWarningRedThreshold, + ]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/SessionList.tsx` around lines 920 - 988, The useMemo for tooltipStyles is missing dependencies so contextUsageColor and the width (`session.contextUsage`) can become stale; update the dependency array for the tooltipStyles memo (where tooltipStyles is defined and getContextColor is called) to include session.contextUsage, contextWarningYellowThreshold, and contextWarningRedThreshold in addition to theme.colors so the styles (including the contextUsage width and color) recompute when those values change.
🧹 Nitpick comments (2)
src/renderer/components/FilePreview.tsx (1)
869-880: Remove redundant regex copies.These three memoized values (
searchRegexForCodeSearch,searchRegexForMarkdownSearch,searchRegexForEdit) are all identical copies ofsearchRegex. ThelastIndexstate concern doesn't apply here becausematchAll(used bygetMatches) always starts from index 0 and returns all matches without mutating the regex's state.You can use
searchRegexdirectly in all three contexts and remove these intermediate memos.♻️ Suggested simplification
const contentSearchMatches = useMemo(() => { return getMatches(file?.content ?? '', searchRegex); }, [searchRegex, file?.content]); const contentSearchMatchCount = contentSearchMatches.length; -const searchRegexForCodeSearch = useMemo(() => { - if (!searchRegex) return null; - return new RegExp(searchRegex.source, searchRegex.flags); -}, [searchRegex]); -const searchRegexForMarkdownSearch = useMemo(() => { - if (!searchRegex) return null; - return new RegExp(searchRegex.source, searchRegex.flags); -}, [searchRegex]); -const searchRegexForEdit = useMemo(() => { - if (!searchRegex) return null; - return new RegExp(searchRegex.source, searchRegex.flags); -}, [searchRegex]);Then update usages to reference
searchRegexdirectly (e.g., in the effects andeditModeSearchMatches).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/FilePreview.tsx` around lines 869 - 880, Remove the three redundant memoized regex copies (searchRegexForCodeSearch, searchRegexForMarkdownSearch, searchRegexForEdit) and replace their usages with the original searchRegex; update any references in effects, getMatches calls, and editModeSearchMatches logic to use searchRegex directly since matchAll does not mutate lastIndex, and delete the useMemo blocks for those three symbols to simplify the component.src/main/stats/stats-db.ts (1)
95-98: Minor: RedundantPromise.resolve()in async function.In an async function, a bare
return;implicitly returns a resolved promise. The explicitPromise.resolve()is unnecessary.🔧 Suggested simplification
async initialize(): Promise<void> { if (this.initialized) { - return Promise.resolve(); + return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/stats/stats-db.ts` around lines 95 - 98, The initialize method (async initialize()) contains a redundant Promise.resolve(); replace the explicit Promise.resolve() with a simple bare return (or remove the return entirely) so the async function relies on its implicit resolved Promise; update the initialize() implementation where the early-return checks this.initialized to use return; instead of return Promise.resolve();.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/__tests__/main/stats/stats-db.test.ts`:
- Around line 817-818: The test is calling the async method restoreFromBackup on
db without awaiting it, so the assertion checks a Promise instead of its
resolved boolean; update the test to await
db.restoreFromBackup('/path/to/nonexistent/backup') (or return the Promise) and
ensure the test function is declared async so the expect(result).toBe(false)
asserts the resolved value of restoreFromBackup.
- Around line 741-742: The test is calling the async method
getAvailableBackups() without awaiting it, so change the test to handle the
Promise by marking the test callback async (or returning the Promise) and
awaiting db.getAvailableBackups(); then assert against the resolved value
(expect(await db.getAvailableBackups()).toEqual([]) or store into a variable via
const backups = await db.getAvailableBackups() before asserting). Ensure
getAvailableBackups() is referenced exactly by name in the updated test.
In `@src/renderer/components/SessionList.tsx`:
- Around line 535-559: The useMemo for styles in HamburgerMenuContent currently
lists undefined dependencies (session.contextUsage,
contextWarningYellowThreshold, contextWarningRedThreshold); update the
dependency array to only include the actual values in scope—at minimum
theme.colors—so change the dependencies for the styles useMemo in
HamburgerMenuContent (the styles constant defined via useMemo) to [theme.colors]
(or any other props actually used) and remove the erroneous
session/contextThreshold identifiers.
---
Outside diff comments:
In `@src/__tests__/main/stats/stats-db.test.ts`:
- Around line 761-766: The test calls the async method getAvailableBackups()
without awaiting it; update the test to await db.getAvailableBackups() (e.g.,
assign const backups = await db.getAvailableBackups()) so the promise resolves
before assertions. Locate the call to getAvailableBackups() in the test block
and add the await keyword and ensure the enclosing test function is async if not
already.
- Around line 826-828: The test calls the async method restoreFromBackup()
without awaiting it, causing the expectation on mockDb.close to run early;
update the test so restoreFromBackup() is awaited (or return its promise) before
asserting expect(mockDb.close).toHaveBeenCalled(), ensuring the test function is
marked async if needed and referencing the restoreFromBackup() call and the
mockDb.close assertion in the test.
- Around line 836-842: The test calls the async method restoreFromBackup()
without awaiting it and still asserts against the sync mock; change the test to
await db.restoreFromBackup('/path/to/backup.db') and update the expectation to
check the async mock (mockFsCopyFile) was called with '/path/to/backup.db' and a
destination containing 'stats.db' (restoreFromBackup uses fsp.copyFile, so
assert mockFsCopyFile rather than mockFsCopyFileSync).
- Around line 778-781: The test is calling the async method
getAvailableBackups() without awaiting it; change the test to await
db.getAvailableBackups() (e.g., const backups = await db.getAvailableBackups())
and ensure the test function is declared async so the await is valid; update any
surrounding test callback to async and keep the rest of the assertions
unchanged.
- Around line 849-853: The test calls the async method restoreFromBackup()
without awaiting it and then asserts unlink behavior; change the test to await
db.restoreFromBackup(...) to ensure the promise resolves before assertions and
replace the sync assertion mockFsUnlinkSync with the async mock assertion
mockFsUnlink (or mockFsUnlink.mockResolvedValue...) so the test verifies the
asynchronous unlink path; update references to restoreFromBackup and
mockFsUnlink/mockFsUnlinkSync in the test accordingly.
- Around line 952-954: The test calls db.backupDatabase() but doesn't await the
async method; update the test to await db.backupDatabase() (and optionally
capture the Promise result if needed) before asserting
expect(mockDb.pragma).toHaveBeenCalledWith('wal_checkpoint(TRUNCATE)'); ensure
the call to backupDatabase() in the test for stats-db.test.ts is awaited so the
async work completes prior to the expectation.
- Around line 978-980: The test calls db.backupDatabase() without awaiting it,
so make the test await the async backupDatabase() call (change
db.backupDatabase(); to await db.backupDatabase();) and ensure the surrounding
test function is declared async (or returns the Promise) so the
expect(callOrder).toEqual(['checkpoint','copy']) runs after completion; update
references to backupDatabase and the test function signature accordingly.
- Around line 799-803: The test calls the async method getAvailableBackups()
without awaiting it; update the test to await db.getAvailableBackups() and
ensure the surrounding test function is declared async so backups resolves
before assertions (reference the getAvailableBackups() call and the backups
variable in this test block).
In `@src/renderer/components/CsvTableRenderer.tsx`:
- Around line 300-337: The header <th> elements rendered in the headerRow
mapping are clickable via handleHeaderClick but lack keyboard accessibility; add
tabIndex={0} and a keyboard event handler that calls handleHeaderClick(i) when
Enter or Space is pressed (e.g., an onKeyDown that checks for key === 'Enter' ||
key === ' ' / keyCode 13/32), and also add role="button" (and optionally
onFocus/onBlur for focus styling) so sortable headers are reachable and operable
by keyboard users; update references around headerRow, handleHeaderClick, and
sort to ensure behavior and visual focus state match the mouse click
interaction.
In `@src/renderer/components/SessionList.tsx`:
- Around line 174-225: menuStyles' useMemo references left, top and ready before
they are declared which causes a TDZ; move the call to
useContextMenuPosition(menuRef, x, y) so that const { left, top, ready } is
declared before the useMemo that computes menuStyles, then update the useMemo
dependency array to keep left, top and ready (and existing submenuPosition &
style refs) so React hooks/readers see the values in scope; ensure
useClickOutside(menuRef, onDismiss) and the Escape useEffect remain in place and
that no other references to left/top/ready precede the moved hook.
---
Duplicate comments:
In `@src/renderer/components/AutoRun.tsx`:
- Around line 1514-1520: The searchHighlight object is using the untrimmed
variable searchQuery while match counting uses trimmedSearchQuery, causing
mismatched highlights for queries with surrounding whitespace; update the
searchHighlight call (the object passed where containerRef: previewRef,
searchHighlight: {...}) to use trimmedSearchQuery for query (i.e., pass
trimmedSearchQuery instead of searchQuery) and leave currentMatchIndex and
onMatchRendered (handleMatchRendered) unchanged so counting and highlighting use
the same trimmed input.
In `@src/renderer/components/SessionList.tsx`:
- Line 172: The useMemo dependency array in SessionList.tsx (within the
SessionContextMenu / useMemo call) includes undefined or unused identifiers
(session.contextUsage, contextWarningYellowThreshold,
contextWarningRedThreshold) causing a ReferenceError; update the dependency
array to only include the actual values referenced inside the memoized callback
(e.g., theme.colors and any properties of session that the callback reads),
removing session.contextUsage and the two threshold names that are not defined
in this scope so the dependencies match the used variables.
- Line 3220: The unread badge is referencing a non-existent style path
sessionListStyles.skinny.unreadBadge; update the reference to the correct style
location sessionListStyles.root.unreadBadge (or move the unreadBadge into the
.root object) so the collapsed sidebar uses the valid style key; change any
other uses of sessionListStyles.skinny to sessionListStyles.root and ensure the
unreadBadge entry exists on sessionListStyles.root in the SessionList component.
- Around line 1798-1811: sessionListStyles.skinny is undefined causing runtime
errors; either add a skinny object to the sessionListStyles definition with
skinnyDotFallback and skinnyTooltip keys, or update the references to point to
the existing path (e.g. replace sessionListStyles.skinny.skinnyDotFallback and
sessionListStyles.skinny.skinnyTooltip with
sessionListStyles.root.skinnyDotFallback and
sessionListStyles.root.skinnyTooltip). Adjust the code around tooltipPosition
(still use tooltipPosition ? { top: `${tooltipPosition.y}px` } : {}) and ensure
the names skinnyDotFallback and skinnyTooltip match exactly what you add or
reference.
- Around line 920-988: The useMemo for tooltipStyles is missing dependencies so
contextUsageColor and the width (`session.contextUsage`) can become stale;
update the dependency array for the tooltipStyles memo (where tooltipStyles is
defined and getContextColor is called) to include session.contextUsage,
contextWarningYellowThreshold, and contextWarningRedThreshold in addition to
theme.colors so the styles (including the contextUsage width and color)
recompute when those values change.
---
Nitpick comments:
In `@src/main/stats/stats-db.ts`:
- Around line 95-98: The initialize method (async initialize()) contains a
redundant Promise.resolve(); replace the explicit Promise.resolve() with a
simple bare return (or remove the return entirely) so the async function relies
on its implicit resolved Promise; update the initialize() implementation where
the early-return checks this.initialized to use return; instead of return
Promise.resolve();.
In `@src/renderer/components/FilePreview.tsx`:
- Around line 869-880: Remove the three redundant memoized regex copies
(searchRegexForCodeSearch, searchRegexForMarkdownSearch, searchRegexForEdit) and
replace their usages with the original searchRegex; update any references in
effects, getMatches calls, and editModeSearchMatches logic to use searchRegex
directly since matchAll does not mutate lastIndex, and delete the useMemo blocks
for those three symbols to simplify the component.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/__tests__/main/ipc/handlers/git.test.tssrc/__tests__/main/stats/stats-db.test.tssrc/main/ipc/handlers/git.tssrc/main/stats/migrations.tssrc/main/stats/schema.tssrc/main/stats/stats-db.tssrc/renderer/components/AutoRun.tsxsrc/renderer/components/CsvTableRenderer.tsxsrc/renderer/components/FilePreview.tsxsrc/renderer/components/SessionList.tsxsrc/renderer/components/SessionListItem.tsxsrc/shared/stats-types.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/renderer/components/SessionListItem.tsx
- src/tests/main/ipc/handlers/git.test.ts
Comment on lines
741
to
742
| const backups = db.getAvailableBackups(); | ||
| expect(backups).toEqual([]); |
There was a problem hiding this comment.
Missing await for async getAvailableBackups() method.
getAvailableBackups() now returns Promise<Array<...>> but the test doesn't await it. This will cause the assertion to compare against a Promise object rather than the resolved array.
🐛 Proposed fix
- const backups = db.getAvailableBackups();
- expect(backups).toEqual([]);
+ const backups = await db.getAvailableBackups();
+ expect(backups).toEqual([]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const backups = db.getAvailableBackups(); | |
| expect(backups).toEqual([]); | |
| const backups = await db.getAvailableBackups(); | |
| expect(backups).toEqual([]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/__tests__/main/stats/stats-db.test.ts` around lines 741 - 742, The test
is calling the async method getAvailableBackups() without awaiting it, so change
the test to handle the Promise by marking the test callback async (or returning
the Promise) and awaiting db.getAvailableBackups(); then assert against the
resolved value (expect(await db.getAvailableBackups()).toEqual([]) or store into
a variable via const backups = await db.getAvailableBackups() before asserting).
Ensure getAvailableBackups() is referenced exactly by name in the updated test.
Comment on lines
817
to
818
| const result = db.restoreFromBackup('/path/to/nonexistent/backup'); | ||
| expect(result).toBe(false); |
There was a problem hiding this comment.
Missing await for async restoreFromBackup() method.
restoreFromBackup() now returns Promise<boolean> but the test doesn't await it. The assertion will always pass because a Promise object is truthy/falsy regardless of the resolved value.
🐛 Proposed fix
- const result = db.restoreFromBackup('/path/to/nonexistent/backup');
- expect(result).toBe(false);
+ const result = await db.restoreFromBackup('/path/to/nonexistent/backup');
+ expect(result).toBe(false);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const result = db.restoreFromBackup('/path/to/nonexistent/backup'); | |
| expect(result).toBe(false); | |
| const result = await db.restoreFromBackup('/path/to/nonexistent/backup'); | |
| expect(result).toBe(false); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/__tests__/main/stats/stats-db.test.ts` around lines 817 - 818, The test
is calling the async method restoreFromBackup on db without awaiting it, so the
assertion checks a Promise instead of its resolved boolean; update the test to
await db.restoreFromBackup('/path/to/nonexistent/backup') (or return the
Promise) and ensure the test function is declared async so the
expect(result).toBe(false) asserts the resolved value of restoreFromBackup.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/renderer/components/CsvTableRenderer.tsx (1)
89-91: Avoid per-call regex allocation inisNumericValueLine 89 recreates a large regex on every call. This function is called repeatedly during alignment detection; hoisting to a module-level constant avoids repeated allocations and keeps the pattern single-sourced.
Proposed refactor
+const STRICT_NUMBER_VALUE_REGEX = + /^(?:-?\$?(?:\d+|\d{1,3}(?:,\d{3})+)(?:\.\d+)?%?|\(\$?(?:\d+|\d{1,3}(?:,\d{3})+)(?:\.\d+)?\)%?)$/; + function isNumericValue(value: string): boolean { const trimmed = value.trim(); if (trimmed === '') return false; - return /^(?:-?\$?(?:\d+|\d{1,3}(?:,\d{3})+)(?:\.\d+)?%?|\(\$?(?:\d+|\d{1,3}(?:,\d{3})+)(?:\.\d+)?\)%?)$/.test( - trimmed - ); + return STRICT_NUMBER_VALUE_REGEX.test(trimmed); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/CsvTableRenderer.tsx` around lines 89 - 91, The isNumericValue function currently constructs its large regex on every call; hoist the regex to a module-level constant (e.g., NUMERIC_VALUE_REGEX) and replace the inline /.../.test(trimmed) with NUMERIC_VALUE_REGEX.test(trimmed) inside isNumericValue so the pattern is allocated once and reused across calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/web-server/managers/CallbackRegistry.ts`:
- Around line 219-229: The three setters setStarTabCallback,
setReorderTabCallback, and setToggleBookmarkCallback are never called so
starTab(), reorderTab(), and toggleBookmark() always return false; fix by wiring
these callbacks into the WebServer initialization alongside the other setters
(call CallbackRegistry.setStarTabCallback, .setReorderTabCallback,
.setToggleBookmarkCallback with the same handler functions used for other tab
ops), add corresponding API route handlers in apiRoutes.ts that invoke
callbackRegistry.starTab/reorderTab/toggleBookmark (mirroring existing route
patterns for tabs), and register IPC handlers (if the actions can be initiated
from the main process) to forward requests to the same callback methods so the
feature is functional end-to-end.
In `@src/renderer/components/CsvTableRenderer.tsx`:
- Around line 134-135: compareValues currently strips parentheses via
NUMERIC_CLEANUP_REGEX so accounting negatives like "(123)" are parsed as
positive 123; modify compareValues (the logic using aVal, bVal and
NUMERIC_CLEANUP_REGEX) to detect parenthesis-wrapped negatives before cleanup:
if a value matches a parenthesis-wrapped numeric pattern (starts with '(' and
ends with ')') remove the parentheses, set a negative flag and apply that sign
after parseFloat (same for bVal) so "(123)" becomes -123; keep using
NUMERIC_CLEANUP_REGEX for removing other non-numeric characters but ensure sign
handling occurs before/independently of that cleanup and preserves negative
zero/NaN behavior consistent with isNumericValue.
---
Nitpick comments:
In `@src/renderer/components/CsvTableRenderer.tsx`:
- Around line 89-91: The isNumericValue function currently constructs its large
regex on every call; hoist the regex to a module-level constant (e.g.,
NUMERIC_VALUE_REGEX) and replace the inline /.../.test(trimmed) with
NUMERIC_VALUE_REGEX.test(trimmed) inside isNumericValue so the pattern is
allocated once and reused across calls.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/web-server/managers/CallbackRegistry.tssrc/renderer/components/CsvTableRenderer.tsx
Comment on lines
+134
to
+135
| const aNum = parseFloat(aVal.replace(NUMERIC_CLEANUP_REGEX, '')); | ||
| const bNum = parseFloat(bVal.replace(NUMERIC_CLEANUP_REGEX, '')); |
There was a problem hiding this comment.
compareValues mis-sorts accounting negatives wrapped in parentheses
Line 134 and Line 135 remove parentheses before parsing, so (123) is treated as 123 instead of -123. Since isNumericValue accepts parenthesized values, numeric sorting becomes incorrect for finance-style CSV data.
Proposed fix
+function parseNumericForSort(raw: string): number | null {
+ const trimmed = raw.trim();
+ const isAccountingNegative = /^\(.*\)$/.test(trimmed);
+ const normalized = trimmed.replace(NUMERIC_CLEANUP_REGEX, '');
+ const parsed = parseFloat(normalized);
+ if (Number.isNaN(parsed)) return null;
+ return isAccountingNegative ? -parsed : parsed;
+}
+
function compareValues(a: string, b: string, direction: SortDirection): number {
const aVal = a.trim();
const bVal = b.trim();
@@
- const aNum = parseFloat(aVal.replace(NUMERIC_CLEANUP_REGEX, ''));
- const bNum = parseFloat(bVal.replace(NUMERIC_CLEANUP_REGEX, ''));
+ const aNum = parseNumericForSort(aVal);
+ const bNum = parseNumericForSort(bVal);
- if (!isNaN(aNum) && !isNaN(bNum)) {
+ if (aNum !== null && bNum !== null) {
return direction === 'asc' ? aNum - bNum : bNum - aNum;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/components/CsvTableRenderer.tsx` around lines 134 - 135,
compareValues currently strips parentheses via NUMERIC_CLEANUP_REGEX so
accounting negatives like "(123)" are parsed as positive 123; modify
compareValues (the logic using aVal, bVal and NUMERIC_CLEANUP_REGEX) to detect
parenthesis-wrapped negatives before cleanup: if a value matches a
parenthesis-wrapped numeric pattern (starts with '(' and ends with ')') remove
the parentheses, set a negative flag and apply that sign after parseFloat (same
for bVal) so "(123)" becomes -123; keep using NUMERIC_CLEANUP_REGEX for removing
other non-numeric characters but ensure sign handling occurs
before/independently of that cleanup and preserves negative zero/NaN behavior
consistent with isNumericValue.
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/__tests__/main/history-manager.test.ts (1)
145-149:⚠️ Potential issue | 🟡 MinorAll test descriptions are identical and non-descriptive.
Every test in this file uses
'should run async path'as the description. This makes it impossible to understand what each test verifies from test output or failure reports. Each test should have a unique, descriptive name explaining the specific behavior being tested.For example:
'should return true when migration marker exists'instead of'should run async path''should return entries from existing session file'instead of'should run async path'Also applies to: 288-298, 455-504, 511-566, 568-619, 626-693, 700-767, 774-812, 819-834, 841-855, 862-910, 917-945, 952-995, 1002-1020, 1027-1055, 1062-1137, 1144-1181, 1188-1208, 1215-1329, 1336-1345, 1352-1365
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/main/history-manager.test.ts` around lines 145 - 149, Replace the duplicate test descriptions 'should run async path' in the history-manager.test.ts test suite with unique, descriptive names that state the expected behavior for each individual it(...) block (e.g., "should return true when migration marker exists", "should return entries from existing session file", "should use legacy file path", etc.); search for all occurrences of it('should run async path', ...) in this file and update the string arguments to clearly describe what manager.getHistoryDir(), manager.getLegacyFilePath(), migration checks, session reads, and other assertions in each test are verifying so failures are immediately understandable.src/renderer/components/AICommandsPanel.tsx (1)
59-63:⚠️ Potential issue | 🟡 MinorPotential stale closure in
onChangecallback.The callback captures
newCommandfrom the current render. If the user modifies other fields (command/description) and then the autocomplete triggers this callback, those changes could be overwritten. Use functional update form to be safe:} = useTemplateAutocomplete({ textareaRef: newCommandTextareaRef, value: newCommand.prompt, - onChange: (value) => setNewCommand({ ...newCommand, prompt: value }), + onChange: (value) => setNewCommand((prev) => ({ ...prev, prompt: value })), });The same pattern applies to the edit autocomplete hook at line 75.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/AICommandsPanel.tsx` around lines 59 - 63, The onChange passed into useTemplateAutocomplete captures the current newCommand object causing stale-closure overwrites of other fields; change the updater to use the functional form of state setters (e.g., call setNewCommand(prev => ({ ...prev, prompt: value }))) so updates merge with the latest state, and apply the same fix for the edit autocomplete callback (use setEditCommand(prev => ({ ...prev, prompt: value }))) while keeping the same textarea refs and hook usage.
♻️ Duplicate comments (9)
src/main/history-manager.ts (1)
505-506:⚠️ Potential issue | 🟠 Major
updateSessionNameByClaudeSessionIdstops after the first updated file.The
breakstatement on Line 506 exits the loop after updating the first matching session file, so subsequent files with entries matching the sameagentSessionIdare not updated.🐛 Proposed fix
if (sessionUpdatedCount > 0) { try { await fs.promises.writeFile( parsed.filePath, JSON.stringify(parsed.data, null, 2), 'utf-8' ); logger.debug( `Updated ${sessionUpdatedCount} entries for agentSessionId ${agentSessionId} in session ${parsed.sessionId}`, LOG_CONTEXT ); } catch (error) { logger.warn( `Failed to update sessionName in session ${parsed.sessionId}: ${error}`, LOG_CONTEXT ); captureException(error, { operation: 'history:updateSessionNameWrite', sessionId: parsed.sessionId, }); } - - break; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/history-manager.ts` around lines 505 - 506, The break in updateSessionNameByClaudeSessionId causes the loop to stop after updating the first matching file; remove the break so the loop continues and updates all files whose session entries match the provided agentSessionId, ensuring each matching file is processed (keep existing file-read, modify, and write logic in that function).src/__tests__/main/stats/stats-db.test.ts (3)
741-742:⚠️ Potential issue | 🟠 MajorMissing
awaitfor asyncgetAvailableBackups()method.Based on the relevant code snippets showing
getAvailableBackups()returnsPromise<Array<...>>, this test doesn't await the async call. The assertion compares against a Promise object rather than the resolved array.🐛 Proposed fix
- const backups = db.getAvailableBackups(); + const backups = await db.getAvailableBackups(); expect(backups).toEqual([]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/main/stats/stats-db.test.ts` around lines 741 - 742, The test is awaiting the Promise incorrectly: change the call to use await on db.getAvailableBackups() and ensure the test function is declared async so the awaited result is used (e.g., const backups = await db.getAvailableBackups()), then assert expect(backups).toEqual([]); reference getAvailableBackups() and the backups variable when updating the test.
817-818:⚠️ Potential issue | 🟠 MajorMissing
awaitfor asyncrestoreFromBackup()method.Based on the AI summary indicating async migration patterns,
restoreFromBackup()now returnsPromise<boolean>. The assertion will always pass because a Promise object is truthy regardless of the resolved value.🐛 Proposed fix
- const result = db.restoreFromBackup('/path/to/nonexistent/backup'); + const result = await db.restoreFromBackup('/path/to/nonexistent/backup'); expect(result).toBe(false);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/main/stats/stats-db.test.ts` around lines 817 - 818, The test calls the async method restoreFromBackup but doesn't await it, causing the assertion to check a Promise instead of its boolean result; update the test to be async (e.g., add async to the enclosing test function) and await db.restoreFromBackup('/path/to/nonexistent/backup'), assigning the resolved value to result before asserting expect(result).toBe(false) so the actual Promise resolution is tested.
799-800:⚠️ Potential issue | 🟠 MajorMissing
awaitfor asyncgetAvailableBackups()method.🐛 Proposed fix
- const backups = db.getAvailableBackups(); + const backups = await db.getAvailableBackups(); expect(backups[0].date).toBe('2026-02-01');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/main/stats/stats-db.test.ts` around lines 799 - 800, The test calls the async method getAvailableBackups() without awaiting it, causing backups to be a Promise rather than the resolved array; update the test to await db.getAvailableBackups() (e.g., const backups = await db.getAvailableBackups()) so backups[0].date can be asserted correctly and ensure the surrounding test function is async if not already.src/renderer/components/AutoRun.tsx (1)
1512-1526:⚠️ Potential issue | 🟠 MajorUse the same normalized query for highlighting and match counting.
Line 1525 still passes raw
searchQueryintosearchHighlight, while counting/navigation usestrimmedSearchQuery. Inputs like" foo "can still desync highlighted hits vs match count/navigation.🔧 Proposed fix
const searchHighlightedComponents = useMemo(() => { // Only create search-highlighted components when actually needed - if (!searchOpen || !searchQuery.trim() || totalMatches === 0) { + if (!searchOpen || !trimmedSearchQuery || totalMatches === 0) { return null; } @@ searchHighlight: { - query: searchQuery, + query: trimmedSearchQuery, currentMatchIndex, onMatchRendered: handleMatchRendered, }, }); @@ searchOpen, - searchQuery, + trimmedSearchQuery, totalMatches, currentMatchIndex, handleMatchRendered, ]);Also applies to: 1545-1553
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/AutoRun.tsx` around lines 1512 - 1526, The highlight query being passed to createMarkdownComponents is the raw searchQuery, which can desync highlighting from match counting/navigation that use trimmedSearchQuery; update the call sites (e.g., the createMarkdownComponents invocation in AutoRun.tsx and the same block around lines 1545-1553) to pass the normalized query (use trimmedSearchQuery or a single normalized variable) into the searchHighlight.query field and any other highlight-related props so highlighting, totalMatches, and currentMatchIndex all use the same normalized string.src/renderer/components/SessionList.tsx (4)
1754-1765:⚠️ Potential issue | 🔴 CriticalUse the correct style path (
root) for skinny-mode styles.
sessionListStyles.skinny.*is not defined; skinny styles are nested undersessionListStyles.root.*. This breaks skinny-mode fallback/tooltip/badge styling.🐛 Proposed fix
- ...sessionListStyles.skinny.skinnyDotFallback, + ...sessionListStyles.root.skinnyDotFallback, @@ - ...sessionListStyles.skinny.skinnyDotFallback, + ...sessionListStyles.root.skinnyDotFallback, @@ - ...sessionListStyles.skinny.skinnyTooltip, + ...sessionListStyles.root.skinnyTooltip, @@ - sessionListStyles.skinny.skinnyDotFallback, - sessionListStyles.skinny.skinnyTooltip, + sessionListStyles.root.skinnyDotFallback, + sessionListStyles.root.skinnyTooltip, @@ - style={sessionListStyles.skinny.unreadBadge} + style={sessionListStyles.root.unreadBadge}Also applies to: 1784-1785, 3200-3200
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/SessionList.tsx` around lines 1754 - 1765, The skinny-mode styles are referenced from the wrong path: replace uses of sessionListStyles.skinny.* with sessionListStyles.root.skinny.* (e.g., update the active/inactive/skinnyDotFallback, tooltip, badge references) so the code reads sessionListStyles.root.skinny.skinnyDotFallback, sessionListStyles.root.skinny.skinnyTooltip, etc.; apply the same fix to the other occurrences noted (the blocks around the other referenced ranges) to ensure skinny-mode fallback/tooltip/badge styles resolve correctly.
888-957:⚠️ Potential issue | 🟠 Major
tooltipStylesmemo is missing dependencies and can go stale.At Lines 889-893 and 940, styles derive from
session.contextUsageand thresholds, but Line 956 only depends ontheme.colors.🐛 Proposed fix
- }, [theme.colors]); + }, [ + theme.colors, + session.contextUsage, + contextWarningYellowThreshold, + contextWarningRedThreshold, + ]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/SessionList.tsx` around lines 888 - 957, The useMemo for tooltipStyles currently only lists [theme.colors] so derived values can go stale; update the dependency array for tooltipStyles to include session.contextUsage, contextWarningYellowThreshold, contextWarningRedThreshold and (optionally) theme or theme.colors so getContextColor and any style entries that reference session/context thresholds are recalculated when those values change; refer to the tooltipStyles constant, getContextColor call, and the session.contextUsage/contextWarningYellowThreshold/contextWarningRedThreshold symbols when making this change.
172-177:⚠️ Potential issue | 🔴 CriticalRemove out-of-scope dependencies from
SessionContextMenustyles memo.At Lines 174-176,
contextWarningYellowThresholdandcontextWarningRedThresholdare not in this component scope, which will break evaluation.session.contextUsageis also not used by this memo.🐛 Proposed fix
- }, [ - theme.colors, - session.contextUsage, - contextWarningYellowThreshold, - contextWarningRedThreshold, - ]); + }, [theme.colors]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/SessionList.tsx` around lines 172 - 177, The useMemo for SessionContextMenu styles includes out-of-scope/unused dependencies (contextWarningYellowThreshold, contextWarningRedThreshold, and session.contextUsage) which will break evaluation; update the dependency array in the SessionContextMenu styles memo to remove those three and retain only the actual in-scope dependencies (e.g., theme.colors) so the memo reflects only values used inside the memoized function.
552-557:⚠️ Potential issue | 🔴 CriticalFix undefined dependencies in
HamburgerMenuContentstyles memo.At Lines 554-556, these identifiers are not in scope in
HamburgerMenuContent, causing runtime/compile-time breakage.🐛 Proposed fix
- }, [ - theme.colors, - session.contextUsage, - contextWarningYellowThreshold, - contextWarningRedThreshold, - ]); + }, [theme.colors]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/SessionList.tsx` around lines 552 - 557, The memo for HamburgerMenuContent styles is using identifiers that aren't in scope (theme.colors, session.contextUsage, contextWarningYellowThreshold, contextWarningRedThreshold); update the dependency list or capture the correct variables in scope so React can track changes: either move the useMemo inside the component where theme and session are defined or change the dependency array to reference the in-scope symbols (e.g., theme, session, contextWarningYellowThreshold, contextWarningRedThreshold or session?.contextUsage) used inside the memo; ensure the memo references only variables available to HamburgerMenuContent and include those exact symbol names in the dependency array so the memo is stable and compiles.
🧹 Nitpick comments (9)
src/main/utils/wslDetector.ts (1)
29-31: Avoid duplicate lowercase allocations in WSL detection.You call
toLowerCase()twice on the same string. Normalize once and reuse.♻️ Suggested micro-refactor
- const version = await fs.readFile('/proc/version', 'utf8'); - wslDetectionCache = - version.toLowerCase().includes('microsoft') || version.toLowerCase().includes('wsl'); + const version = (await fs.readFile('/proc/version', 'utf8')).toLowerCase(); + wslDetectionCache = version.includes('microsoft') || version.includes('wsl');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/utils/wslDetector.ts` around lines 29 - 31, The code calls version.toLowerCase() twice when computing wslDetectionCache; normalize once and reuse to avoid duplicate allocations by assigning const lower = version.toLowerCase() (or similar) and then set wslDetectionCache = lower.includes('microsoft') || lower.includes('wsl'), referencing the existing version variable and wslDetectionCache identifier in wslDetector logic.src/main/debug-package/collectors/group-chats.ts (1)
51-84: Consider parallel file processing for improved performance.The current sequential
awaitinside thefor...ofloop processes files one at a time. For debug collection with potentially many group chat files, parallel processing could improve throughput.That said, sequential processing is safer and perfectly acceptable for a debug collector that isn't on a hot path.
♻️ Optional: Parallel processing alternative
- for (const file of files) { - if (!file.endsWith('.json') || file.endsWith('.log.json')) { - continue; - } - - const filePath = path.join(groupChatsPath, file); - - try { - const content = await fs.promises.readFile(filePath, 'utf-8'); - const chat = JSON.parse(content); - - // Get corresponding log file for message count - const logPath = path.join(groupChatsPath, `${path.basename(file, '.json')}.log.json`); - const messageCount = await countMessages(logPath); - - const chatInfo: GroupChatInfo = { - id: chat.id || path.basename(file, '.json'), - moderatorAgentId: chat.moderatorAgentId || chat.moderator?.agentId || 'unknown', - participantCount: Array.isArray(chat.participants) ? chat.participants.length : 0, - participants: Array.isArray(chat.participants) - ? chat.participants.map((p: any) => ({ - agentId: p.agentId || 'unknown', - })) - : [], - messageCount, - createdAt: chat.createdAt || 0, - updatedAt: chat.updatedAt || 0, - }; - - groupChats.push(chatInfo); - } catch { - // Skip files that can't be parsed - } - } - - return groupChats; + const jsonFiles = files.filter( + (file) => file.endsWith('.json') && !file.endsWith('.log.json') + ); + + const results = await Promise.all( + jsonFiles.map(async (file) => { + const filePath = path.join(groupChatsPath, file); + try { + const content = await fs.promises.readFile(filePath, 'utf-8'); + const chat = JSON.parse(content); + + const logPath = path.join(groupChatsPath, `${path.basename(file, '.json')}.log.json`); + const messageCount = await countMessages(logPath); + + return { + id: chat.id || path.basename(file, '.json'), + moderatorAgentId: chat.moderatorAgentId || chat.moderator?.agentId || 'unknown', + participantCount: Array.isArray(chat.participants) ? chat.participants.length : 0, + participants: Array.isArray(chat.participants) + ? chat.participants.map((p: any) => ({ + agentId: p.agentId || 'unknown', + })) + : [], + messageCount, + createdAt: chat.createdAt || 0, + updatedAt: chat.updatedAt || 0, + } as GroupChatInfo; + } catch { + return null; + } + }) + ); + + return results.filter((chat): chat is GroupChatInfo => chat !== null);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/debug-package/collectors/group-chats.ts` around lines 51 - 84, The loop over files in collect/group-chats is sequential and should be converted to parallel processing: map the files array to an array of async tasks that skip non-JSON or *.log.json names (same filter logic), then inside each task read and parse the file (using fs.promises.readFile), call countMessages(logPath), build the GroupChatInfo (use same fields/id fallback/moderator/participants mapping), and return it; use Promise.all to await all tasks and push only successfully parsed results into groupChats (or filter out failed tasks), ensuring errors for individual files are caught inside each task so one bad file doesn’t reject the whole Promise.all. Reference symbols: files, groupChatsPath, countMessages, GroupChatInfo, groupChats.src/main/history-manager.ts (2)
516-524:clearByProjectPathcould parallelize reads.Similar to
getEntriesByProjectPath, this method awaitsgetEntriessequentially in a loop. Consider parallelizing reads while keeping clears sequential to avoid potential race conditions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/history-manager.ts` around lines 516 - 524, In clearByProjectPath, avoid sequentially awaiting getEntries for each session: call this.listSessionsWithHistorySync(), then start all reads concurrently by mapping sessionIds to getEntries(sessionId) and awaiting Promise.all to get an array of entries; after that, iterate the results in order and call this.clearSession(sessionId) synchronously only for those where entries[0]?.projectPath === projectPath to keep clears sequential and prevent races. Use the existing method names (clearByProjectPath, listSessionsWithHistorySync, getEntries, clearSession) to locate and implement the change.
399-432: Consider parallelizing session reads.Both
getEntriesByProjectPathandgetEntriesByProjectPathPaginatediterate sequentially over sessions withawaitinside the loop. Since there are no dependencies between iterations, these could usePromise.alllikegetAllEntriesdoes for better performance.♻️ Proposed refactor for getEntriesByProjectPath
async getEntriesByProjectPath(projectPath: string): Promise<HistoryEntry[]> { const sessions = this.listSessionsWithHistorySync(); - const entries: HistoryEntry[] = []; - - for (const sessionId of sessions) { - const sessionEntries = await this.getEntries(sessionId); - if (sessionEntries.length > 0 && sessionEntries[0].projectPath === projectPath) { - entries.push(...sessionEntries); - } - } + const allSessionEntries = await Promise.all( + sessions.map((sessionId) => this.getEntries(sessionId)) + ); + const entries = allSessionEntries + .filter((sessionEntries) => sessionEntries.length > 0 && sessionEntries[0].projectPath === projectPath) + .flat(); return sortEntriesByTimestamp(entries); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/history-manager.ts` around lines 399 - 432, Both getEntriesByProjectPath and getEntriesByProjectPathPaginated perform session reads sequentially; change them to fetch per-session entries in parallel by mapping sessions from listSessionsWithHistorySync() to an array of getEntries(sessionId) promises, await Promise.all to get the sessionEntries arrays, then flatten and filter those where the first entry's projectPath equals the given projectPath before calling sortEntriesByTimestamp (and paginateEntries in the paginated variant); keep references to getEntriesByProjectPath, getEntriesByProjectPathPaginated, listSessionsWithHistorySync, getEntries, sortEntriesByTimestamp, and paginateEntries so the logic is applied in both functions.src/renderer/components/AICommandsPanel.tsx (1)
225-240: Consider addingaria-expandedfor accessibility.The collapsible section button would benefit from an
aria-expandedattribute to communicate state to screen readers:<button onClick={() => setVariablesExpanded(!variablesExpanded)} className="w-full px-3 py-2 flex items-center justify-between hover:bg-white/5 transition-colors" + aria-expanded={variablesExpanded} >The same applies to the command list toggle buttons at line 464.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/AICommandsPanel.tsx` around lines 225 - 240, The Template Variables toggle button (the button that calls setVariablesExpanded and reads variablesExpanded) is missing an accessibility state: add aria-expanded={variablesExpanded} to that button so screen readers know whether the collapsible is open; likewise locate the command list toggle buttons (the ones that toggle the commandsExpanded state via setCommandsExpanded or similar) and add aria-expanded={commandsExpanded} (or the appropriate boolean state variable) to each corresponding button so the expanded/collapsed state is exposed to assistive technologies.src/__tests__/main/ipc/handlers/agentSessions.test.ts (3)
91-113: PartialStatsobjects may cause runtime errors if unmocked properties are accessed.The mock returns objects with only
isDirectory,size, andmtimeMs, but casts them asfs.Stats. If the handler accesses otherStatsproperties (e.g.,mtime,birthtime,mode), tests may fail with cryptic errors or pass incorrectly withundefinedvalues.Suggested improvement
Consider extending the mock objects with commonly used properties or using a more complete mock:
const createMockStats = (overrides: Partial<fs.Stats>): fs.Stats => ({ isDirectory: () => false, isFile: () => true, isBlockDevice: () => false, isCharacterDevice: () => false, isSymbolicLink: () => false, isFIFO: () => false, isSocket: () => false, size: 0, mtimeMs: 0, mtime: new Date(0), ...overrides, } as fs.Stats);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/main/ipc/handlers/agentSessions.test.ts` around lines 91 - 113, The test's fs.stat mock returns partial objects cast to fs.Stats which can break when other Stat properties are accessed; create a helper like createMockStats(overrides: Partial<fs.Stats>) that returns a complete fs.Stats-shaped object (implementing isDirectory, isFile, all isX methods, size, mtimeMs, mtime as Date, birthtime, mode, etc.) and use it inside the vi.mocked(fs.stat).mockImplementation to return createMockStats({ isDirectory: () => true, size: 0, mtimeMs: 1 }) for projectDir, createMockStats({ isDirectory: () => false, size: 123, mtimeMs: 1700000 }) for sessionFilePath, and a default createMockStats(...) for the fallback so tests won't encounter undefined Stat properties.
54-58: Consider importingGLOBAL_STATS_CACHE_VERSIONfrom the actual source.The hardcoded version
3may drift from the actual implementation value. If the real constant changes, tests could pass while using a stale version.Suggested approach
If the mock architecture allows, consider re-exporting the actual constant:
vi.mock('../../../../main/utils/statsCache', () => ({ loadGlobalStatsCache: vi.fn(), saveGlobalStatsCache: vi.fn(), - GLOBAL_STATS_CACHE_VERSION: 3, + GLOBAL_STATS_CACHE_VERSION: vi.importActual('../../../../main/utils/statsCache').then(m => m.GLOBAL_STATS_CACHE_VERSION), }));Alternatively, add a comment noting that this value must be kept in sync with the source.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/main/ipc/handlers/agentSessions.test.ts` around lines 54 - 58, Replace the hardcoded mock value for GLOBAL_STATS_CACHE_VERSION with the real exported constant so the test cannot drift: update the mock setup that currently defines GLOBAL_STATS_CACHE_VERSION: 3 to import/re-export the constant from the actual module that defines it (use the real symbol GLOBAL_STATS_CACHE_VERSION) or, if import in the mock is not possible, add a clear comment next to the mocked GLOBAL_STATS_CACHE_VERSION and a TODO to keep it in sync; keep the mocked functions loadGlobalStatsCache and saveGlobalStatsCache as-is but ensure the version comes from the source to avoid stale test values.
570-578: Exact call count assertions may be brittle.Asserting specific counts like
expect(firstPassAccessCalls).toBe(2)couples the test to implementation details. If the handler adds an extrafs.accesscall for validation or logging, this test breaks despite correct caching behavior.Consider asserting that call counts remain stable between passes (which you already do in lines 575-578) without asserting the exact initial counts, or add a comment explaining why these specific counts matter.
Alternative approach
- expect(firstPassAccessCalls).toBe(2); - expect(firstPassReaddirCalls).toBe(3); - expect(firstPassStatCalls).toBe(3); - expect(firstPassReadFileCalls).toBe(1); + // First pass should make filesystem calls for discovery + expect(firstPassAccessCalls).toBeGreaterThan(0); + expect(firstPassReaddirCalls).toBeGreaterThan(0); + expect(firstPassStatCalls).toBeGreaterThan(0); - expect(secondPassAccessCalls).toBe(firstPassAccessCalls); + // Second pass within TTL should reuse cache - no additional FS calls + expect(secondPassAccessCalls).toBe(firstPassAccessCalls);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/main/ipc/handlers/agentSessions.test.ts` around lines 570 - 578, Remove the brittle exact-count assertions and instead assert relative/stability properties: drop the lines asserting exact numbers for firstPassAccessCalls, firstPassReaddirCalls, firstPassStatCalls, and firstPassReadFileCalls (the four expect(...).toBe(n) lines) and either assert they are > 0 if you need to ensure activity (e.g., expect(firstPassAccessCalls).toBeGreaterThan(0)) or rely solely on the existing equality checks between firstPass* and secondPass* variables (firstPassAccessCalls === secondPassAccessCalls, etc.); alternatively add a short comment near the stability assertions explaining why exact counts are intentionally not asserted. Ensure you update the test around the variables firstPassAccessCalls, firstPassReaddirCalls, firstPassStatCalls, firstPassReadFileCalls and the matching secondPass* assertions accordingly.src/renderer/components/FilePreview.tsx (1)
827-833: Skip markdown task scans for non-markdown files.Lines [827]-[833] run two full regex passes for any file content. Guarding with
isMarkdownavoids unnecessary work on non-markdown files.Suggested refactor
const markdownTaskMatches = useMemo(() => { - if (!file?.content) return null; + if (!isMarkdown || !file?.content) return null; return { open: file.content.match(MARKDOWN_TASK_OPEN_REGEX), closed: file.content.match(MARKDOWN_TASK_CLOSED_REGEX), }; -}, [file?.content]); +}, [isMarkdown, file?.content]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/FilePreview.tsx` around lines 827 - 833, The markdown task regexes run for every file regardless of type—modify the useMemo that computes markdownTaskMatches to first check isMarkdown and return null when false so you skip the heavy regex work; update the dependency array for useMemo to include isMarkdown (e.g., useMemo(() => { if (!isMarkdown || !file?.content) return null; ... }, [file?.content, isMarkdown])) and keep the existing references to MARKDOWN_TASK_OPEN_REGEX and MARKDOWN_TASK_CLOSED_REGEX and the markdownTaskMatches symbol so the memo only executes the .match calls for markdown files.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/releases.md`:
- Around line 400-409: The release notes use the non-standard bullet character
“•”, which many Markdown renderers don’t recognize; update the bulleted lists in
the shown releases.md sections by replacing every “•” list marker with the
standard hyphen "-" so the items (e.g., "Enhanced mobile web interface...",
"Added ThinkingStatusPill...", "Implemented task count badges...", etc.) render
as proper Markdown lists; also make the same replacement in the second block
referenced (the other similar list) to ensure portable rendering.
- Line 41: The release series header contains a formatting typo: change the text
in the header line "# Smaller Changes in 014.x" to use the correct semantic
version format "# Smaller Changes in 0.14.x" so references to the 0.14 release
series are accurate.
- Line 114: Update the sentence in the release notes that reads “You will
received (configurable) warnings…” to correct the grammar: change “received” to
“receive” so it reads “You will receive (configurable) warnings at 60% and 80%
context consumption with a hint to compact.” Make this single-word correction in
the releases.md entry mentioning context management options.
- Line 200: The release notes contain inconsistent wording: find the sentence
containing "Enhanced update checker to filter prerelease tags like -rc, -beta
🚀" and change "prerelease" to the normalized form "pre-release" to match
earlier sections; ensure any other occurrences in the same document follow the
same hyphenated "pre-release" spelling for consistency (update the sentence text
and any matching occurrences).
- Line 35: Fix the typo in the release note sentence that currently reads
"Launch from file preview or from the FIle tree panel" by replacing the
incorrect "FIle" with "File" so the sentence reads "Launch from file preview or
from the File tree panel"; update the text in the docs/releases.md release note
where that phrase appears.
- Line 135: Update the capitalization of "Github" to the official "GitHub" in
the release note sentence that starts "Github Worktree support was added." —
locate the string "Github Worktree support was added." in docs/releases.md (the
release note paragraph) and change "Github" -> "GitHub" so the text reads
"GitHub Worktree support was added."
- Line 24: Replace the phrase "cross context memory" with the hyphenated
"cross-context memory" in the release note line that reads 'Agents are now
inherently aware of your activity history as seen in the history panel 📜 (this
is built-in cross context memory!)' so the compound modifier is correctly
hyphenated.
In `@src/__tests__/main/stats/stats-db.test.ts`:
- Around line 778-779: The test calls the async method getAvailableBackups()
without awaiting it; update the test so the test function is async (or returns
the promise) and await db.getAvailableBackups() before asserting—e.g., change
the test callback to async and use const backups = await
db.getAvailableBackups() so expect(backups).toHaveLength(1) runs on the resolved
value.
- Around line 761-762: The test calls the async method getAvailableBackups()
without awaiting it; change the invocation in the test to await
db.getAvailableBackups() and ensure the enclosing test function is declared
async (or otherwise handles the returned Promise) so that
expect(backups).toHaveLength(3) runs on the resolved result; update the test
where getAvailableBackups() is used (and any previous similar calls) to use
await and make the test function async.
In `@src/main/debug-package/collectors/storage.ts`:
- Around line 35-52: The getDirectorySize function currently uses
fs.promises.stat (symbols: getDirectorySize, stats, fileStats) which follows
symlinks and can cause recursive loops; change to use fs.promises.lstat to
detect symlinks and avoid recursing into them, and implement a visitedPaths (or
visitedInodes) Set passed through recursive calls so that before recursing you
call fs.promises.realpath (or use the inode from lstat) and skip recursion if
the real path (or inode) is already in visitedPaths; update both the initial
stats and per-file checks to use lstat for symlink detection, add tracking
(visitedPaths) to getDirectorySize signature, and ensure symlinks are either
skipped or safely resolved only when not already visited.
In `@src/main/ipc/handlers/git.ts`:
- Around line 1089-1095: The parallel execGit calls (the Promise.all calling
execGit(['rev-parse', ...]) and the earlier is-inside-work-tree execGit call)
are only passing subdirPath as the localCwd so SSH mode ignores the intended
remote working directory; update those execGit invocations to pass
effectiveRemoteCwd (the computed remote cwd used for SSH) as the remoteCwd
argument (alongside subdirPath as localCwd and sshRemote) so metadata and
rev-parse results resolve from the correct remote directory; specifically modify
the calls that reference execGit([...], subdirPath, sshRemote) and the
is-inside-work-tree execGit to use execGit([...], subdirPath, sshRemote,
effectiveRemoteCwd) (or the existing execGit signature that accepts remoteCwd).
In `@src/renderer/components/FilePreview.tsx`:
- Around line 1527-1534: In FilePreview.tsx inside the effect that scans
markdown fallback matches (the loop iterating nodeMatches using matchCount and
targetIndex), remove the early return when the target is found and instead break
out of the loop so the shared post-branch reset logic always runs; ensure you
still perform the scrollIntoView on the found node's parentElement (if present)
before breaking and then let the code after the loop update or clear
matchElementsRef.current so stale elements are not left between views.
- Line 71: The CODE_LANGUAGE_REGEX currently uses /\language-(\w+)/ which misses
non-word language IDs like c++, objective-c, or f#; update the regex (symbol:
CODE_LANGUAGE_REGEX in FilePreview.tsx) to capture any valid language token (for
example /language-([^\s`]+)/ or /language-([A-Za-z0-9.+#-]+)/) and update the
usage sites (the parsing logic around line where code fences are
handled—referenced near lines 989-990) so those language IDs are correctly
extracted and passed to the syntax highlighter.
In `@src/renderer/components/SessionList.tsx`:
- Around line 179-213: The menuStyles useMemo references left, top, ready and
submenuPosition before they are declared, causing a TDZ crash; move the
useContextMenuPosition(...) call (the hook that returns left, top, ready,
submenuPosition) so it appears above the const menuStyles = useMemo(...) and
then keep those variables in the useMemo dependency array
(submenuPosition.horizontal/vertical, left, top, ready) so menuStyles uses
already-declared values from useContextMenuPosition.
In `@src/renderer/components/SessionListItem.tsx`:
- Around line 191-195: The interactive row in SessionListItem is a plain div
(ref={isSelected ? (selectedItemRef as React.RefObject<HTMLDivElement>) : null},
onClick={() => onSessionClick(session)}, style={styles.container}) and needs
keyboard focus support: add tabIndex={0} to the container, wire up
onFocus/onBlur to forward focus state if needed (use selectedItemRef or
component state), and add an onKeyDown handler that calls
onSessionClick(session) when Enter or Space is pressed to match onClick
behavior; ensure accessibility roles/aria-selected are set if this represents a
selectable item.
---
Outside diff comments:
In `@src/__tests__/main/history-manager.test.ts`:
- Around line 145-149: Replace the duplicate test descriptions 'should run async
path' in the history-manager.test.ts test suite with unique, descriptive names
that state the expected behavior for each individual it(...) block (e.g.,
"should return true when migration marker exists", "should return entries from
existing session file", "should use legacy file path", etc.); search for all
occurrences of it('should run async path', ...) in this file and update the
string arguments to clearly describe what manager.getHistoryDir(),
manager.getLegacyFilePath(), migration checks, session reads, and other
assertions in each test are verifying so failures are immediately
understandable.
In `@src/renderer/components/AICommandsPanel.tsx`:
- Around line 59-63: The onChange passed into useTemplateAutocomplete captures
the current newCommand object causing stale-closure overwrites of other fields;
change the updater to use the functional form of state setters (e.g., call
setNewCommand(prev => ({ ...prev, prompt: value }))) so updates merge with the
latest state, and apply the same fix for the edit autocomplete callback (use
setEditCommand(prev => ({ ...prev, prompt: value }))) while keeping the same
textarea refs and hook usage.
---
Duplicate comments:
In `@src/__tests__/main/stats/stats-db.test.ts`:
- Around line 741-742: The test is awaiting the Promise incorrectly: change the
call to use await on db.getAvailableBackups() and ensure the test function is
declared async so the awaited result is used (e.g., const backups = await
db.getAvailableBackups()), then assert expect(backups).toEqual([]); reference
getAvailableBackups() and the backups variable when updating the test.
- Around line 817-818: The test calls the async method restoreFromBackup but
doesn't await it, causing the assertion to check a Promise instead of its
boolean result; update the test to be async (e.g., add async to the enclosing
test function) and await db.restoreFromBackup('/path/to/nonexistent/backup'),
assigning the resolved value to result before asserting
expect(result).toBe(false) so the actual Promise resolution is tested.
- Around line 799-800: The test calls the async method getAvailableBackups()
without awaiting it, causing backups to be a Promise rather than the resolved
array; update the test to await db.getAvailableBackups() (e.g., const backups =
await db.getAvailableBackups()) so backups[0].date can be asserted correctly and
ensure the surrounding test function is async if not already.
In `@src/main/history-manager.ts`:
- Around line 505-506: The break in updateSessionNameByClaudeSessionId causes
the loop to stop after updating the first matching file; remove the break so the
loop continues and updates all files whose session entries match the provided
agentSessionId, ensuring each matching file is processed (keep existing
file-read, modify, and write logic in that function).
In `@src/renderer/components/AutoRun.tsx`:
- Around line 1512-1526: The highlight query being passed to
createMarkdownComponents is the raw searchQuery, which can desync highlighting
from match counting/navigation that use trimmedSearchQuery; update the call
sites (e.g., the createMarkdownComponents invocation in AutoRun.tsx and the same
block around lines 1545-1553) to pass the normalized query (use
trimmedSearchQuery or a single normalized variable) into the
searchHighlight.query field and any other highlight-related props so
highlighting, totalMatches, and currentMatchIndex all use the same normalized
string.
In `@src/renderer/components/SessionList.tsx`:
- Around line 1754-1765: The skinny-mode styles are referenced from the wrong
path: replace uses of sessionListStyles.skinny.* with
sessionListStyles.root.skinny.* (e.g., update the
active/inactive/skinnyDotFallback, tooltip, badge references) so the code reads
sessionListStyles.root.skinny.skinnyDotFallback,
sessionListStyles.root.skinny.skinnyTooltip, etc.; apply the same fix to the
other occurrences noted (the blocks around the other referenced ranges) to
ensure skinny-mode fallback/tooltip/badge styles resolve correctly.
- Around line 888-957: The useMemo for tooltipStyles currently only lists
[theme.colors] so derived values can go stale; update the dependency array for
tooltipStyles to include session.contextUsage, contextWarningYellowThreshold,
contextWarningRedThreshold and (optionally) theme or theme.colors so
getContextColor and any style entries that reference session/context thresholds
are recalculated when those values change; refer to the tooltipStyles constant,
getContextColor call, and the
session.contextUsage/contextWarningYellowThreshold/contextWarningRedThreshold
symbols when making this change.
- Around line 172-177: The useMemo for SessionContextMenu styles includes
out-of-scope/unused dependencies (contextWarningYellowThreshold,
contextWarningRedThreshold, and session.contextUsage) which will break
evaluation; update the dependency array in the SessionContextMenu styles memo to
remove those three and retain only the actual in-scope dependencies (e.g.,
theme.colors) so the memo reflects only values used inside the memoized
function.
- Around line 552-557: The memo for HamburgerMenuContent styles is using
identifiers that aren't in scope (theme.colors, session.contextUsage,
contextWarningYellowThreshold, contextWarningRedThreshold); update the
dependency list or capture the correct variables in scope so React can track
changes: either move the useMemo inside the component where theme and session
are defined or change the dependency array to reference the in-scope symbols
(e.g., theme, session, contextWarningYellowThreshold, contextWarningRedThreshold
or session?.contextUsage) used inside the memo; ensure the memo references only
variables available to HamburgerMenuContent and include those exact symbol names
in the dependency array so the memo is stable and compiles.
---
Nitpick comments:
In `@src/__tests__/main/ipc/handlers/agentSessions.test.ts`:
- Around line 91-113: The test's fs.stat mock returns partial objects cast to
fs.Stats which can break when other Stat properties are accessed; create a
helper like createMockStats(overrides: Partial<fs.Stats>) that returns a
complete fs.Stats-shaped object (implementing isDirectory, isFile, all isX
methods, size, mtimeMs, mtime as Date, birthtime, mode, etc.) and use it inside
the vi.mocked(fs.stat).mockImplementation to return createMockStats({
isDirectory: () => true, size: 0, mtimeMs: 1 }) for projectDir,
createMockStats({ isDirectory: () => false, size: 123, mtimeMs: 1700000 }) for
sessionFilePath, and a default createMockStats(...) for the fallback so tests
won't encounter undefined Stat properties.
- Around line 54-58: Replace the hardcoded mock value for
GLOBAL_STATS_CACHE_VERSION with the real exported constant so the test cannot
drift: update the mock setup that currently defines GLOBAL_STATS_CACHE_VERSION:
3 to import/re-export the constant from the actual module that defines it (use
the real symbol GLOBAL_STATS_CACHE_VERSION) or, if import in the mock is not
possible, add a clear comment next to the mocked GLOBAL_STATS_CACHE_VERSION and
a TODO to keep it in sync; keep the mocked functions loadGlobalStatsCache and
saveGlobalStatsCache as-is but ensure the version comes from the source to avoid
stale test values.
- Around line 570-578: Remove the brittle exact-count assertions and instead
assert relative/stability properties: drop the lines asserting exact numbers for
firstPassAccessCalls, firstPassReaddirCalls, firstPassStatCalls, and
firstPassReadFileCalls (the four expect(...).toBe(n) lines) and either assert
they are > 0 if you need to ensure activity (e.g.,
expect(firstPassAccessCalls).toBeGreaterThan(0)) or rely solely on the existing
equality checks between firstPass* and secondPass* variables
(firstPassAccessCalls === secondPassAccessCalls, etc.); alternatively add a
short comment near the stability assertions explaining why exact counts are
intentionally not asserted. Ensure you update the test around the variables
firstPassAccessCalls, firstPassReaddirCalls, firstPassStatCalls,
firstPassReadFileCalls and the matching secondPass* assertions accordingly.
In `@src/main/debug-package/collectors/group-chats.ts`:
- Around line 51-84: The loop over files in collect/group-chats is sequential
and should be converted to parallel processing: map the files array to an array
of async tasks that skip non-JSON or *.log.json names (same filter logic), then
inside each task read and parse the file (using fs.promises.readFile), call
countMessages(logPath), build the GroupChatInfo (use same fields/id
fallback/moderator/participants mapping), and return it; use Promise.all to
await all tasks and push only successfully parsed results into groupChats (or
filter out failed tasks), ensuring errors for individual files are caught inside
each task so one bad file doesn’t reject the whole Promise.all. Reference
symbols: files, groupChatsPath, countMessages, GroupChatInfo, groupChats.
In `@src/main/history-manager.ts`:
- Around line 516-524: In clearByProjectPath, avoid sequentially awaiting
getEntries for each session: call this.listSessionsWithHistorySync(), then start
all reads concurrently by mapping sessionIds to getEntries(sessionId) and
awaiting Promise.all to get an array of entries; after that, iterate the results
in order and call this.clearSession(sessionId) synchronously only for those
where entries[0]?.projectPath === projectPath to keep clears sequential and
prevent races. Use the existing method names (clearByProjectPath,
listSessionsWithHistorySync, getEntries, clearSession) to locate and implement
the change.
- Around line 399-432: Both getEntriesByProjectPath and
getEntriesByProjectPathPaginated perform session reads sequentially; change them
to fetch per-session entries in parallel by mapping sessions from
listSessionsWithHistorySync() to an array of getEntries(sessionId) promises,
await Promise.all to get the sessionEntries arrays, then flatten and filter
those where the first entry's projectPath equals the given projectPath before
calling sortEntriesByTimestamp (and paginateEntries in the paginated variant);
keep references to getEntriesByProjectPath, getEntriesByProjectPathPaginated,
listSessionsWithHistorySync, getEntries, sortEntriesByTimestamp, and
paginateEntries so the logic is applied in both functions.
In `@src/main/utils/wslDetector.ts`:
- Around line 29-31: The code calls version.toLowerCase() twice when computing
wslDetectionCache; normalize once and reuse to avoid duplicate allocations by
assigning const lower = version.toLowerCase() (or similar) and then set
wslDetectionCache = lower.includes('microsoft') || lower.includes('wsl'),
referencing the existing version variable and wslDetectionCache identifier in
wslDetector logic.
In `@src/renderer/components/AICommandsPanel.tsx`:
- Around line 225-240: The Template Variables toggle button (the button that
calls setVariablesExpanded and reads variablesExpanded) is missing an
accessibility state: add aria-expanded={variablesExpanded} to that button so
screen readers know whether the collapsible is open; likewise locate the command
list toggle buttons (the ones that toggle the commandsExpanded state via
setCommandsExpanded or similar) and add aria-expanded={commandsExpanded} (or the
appropriate boolean state variable) to each corresponding button so the
expanded/collapsed state is exposed to assistive technologies.
In `@src/renderer/components/FilePreview.tsx`:
- Around line 827-833: The markdown task regexes run for every file regardless
of type—modify the useMemo that computes markdownTaskMatches to first check
isMarkdown and return null when false so you skip the heavy regex work; update
the dependency array for useMemo to include isMarkdown (e.g., useMemo(() => { if
(!isMarkdown || !file?.content) return null; ... }, [file?.content,
isMarkdown])) and keep the existing references to MARKDOWN_TASK_OPEN_REGEX and
MARKDOWN_TASK_CLOSED_REGEX and the markdownTaskMatches symbol so the memo only
executes the .match calls for markdown files.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
docs/releases.mdsrc/__tests__/main/history-manager.test.tssrc/__tests__/main/ipc/handlers/agentSessions.test.tssrc/__tests__/main/ipc/handlers/git.test.tssrc/__tests__/main/stats/aggregations.test.tssrc/__tests__/main/stats/auto-run.test.tssrc/__tests__/main/stats/data-management.test.tssrc/__tests__/main/stats/integration.test.tssrc/__tests__/main/stats/paths.test.tssrc/__tests__/main/stats/query-events.test.tssrc/__tests__/main/stats/stats-db.test.tssrc/__tests__/main/web-server/web-server-factory.test.tssrc/main/debug-package/collectors/group-chats.tssrc/main/debug-package/collectors/storage.tssrc/main/history-manager.tssrc/main/ipc/handlers/git.tssrc/main/utils/wslDetector.tssrc/renderer/components/AICommandsPanel.tsxsrc/renderer/components/AgentSessionsBrowser.tsxsrc/renderer/components/AutoRun.tsxsrc/renderer/components/FilePreview.tsxsrc/renderer/components/SessionList.tsxsrc/renderer/components/SessionListItem.tsxsrc/renderer/hooks/batch/useBatchProcessor.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- src/tests/main/web-server/web-server-factory.test.ts
- src/tests/main/ipc/handlers/git.test.ts
- src/tests/main/stats/query-events.test.ts
- src/tests/main/stats/auto-run.test.ts
- src/renderer/components/AgentSessionsBrowser.tsx
|
|
||
| - Desktop app performance improvements (more to come on this, we want Maestro blazing fast) 🐌 | ||
| - Added local manifest feature for custom playbooks 📖 | ||
| - Agents are now inherently aware of your activity history as seen in the history panel 📜 (this is built-in cross context memory!) |
There was a problem hiding this comment.
Fix hyphenation in compound modifier.
Line 24 should use “cross-context memory” for correct wording.
Suggested edit
-- Agents are now inherently aware of your activity history as seen in the history panel 📜 (this is built-in cross context memory!)
+- Agents are now inherently aware of your activity history as seen in the history panel 📜 (this is built-in cross-context memory!)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - Agents are now inherently aware of your activity history as seen in the history panel 📜 (this is built-in cross context memory!) | |
| - Agents are now inherently aware of your activity history as seen in the history panel 📜 (this is built-in cross-context memory!) |
🧰 Tools
🪛 LanguageTool
[grammar] ~24-~24: Use a hyphen to join words.
Context: ...history panel 📜 (this is built-in cross context memory!) - Added markdown render...
(QB_NEW_EN_HYPHEN)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/releases.md` at line 24, Replace the phrase "cross context memory" with
the hyphenated "cross-context memory" in the release note line that reads
'Agents are now inherently aware of your activity history as seen in the history
panel 📜 (this is built-in cross context memory!)' so the compound modifier is
correctly hyphenated.
|
|
||
| The major contributions to 0.14.x remain: | ||
|
|
||
| 🗄️ Document Graphs. Launch from file preview or from the FIle tree panel. Explore relationships between Markdown documents that contain links between documents and to URLs. |
There was a problem hiding this comment.
Correct typo in “File tree panel”.
Line 35 has FIle (capital I).
Suggested edit
-🗄️ Document Graphs. Launch from file preview or from the FIle tree panel.
+🗄️ Document Graphs. Launch from file preview or from the File tree panel.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| 🗄️ Document Graphs. Launch from file preview or from the FIle tree panel. Explore relationships between Markdown documents that contain links between documents and to URLs. | |
| 🗄️ Document Graphs. Launch from file preview or from the File tree panel. Explore relationships between Markdown documents that contain links between documents and to URLs. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/releases.md` at line 35, Fix the typo in the release note sentence that
currently reads "Launch from file preview or from the FIle tree panel" by
replacing the incorrect "FIle" with "File" so the sentence reads "Launch from
file preview or from the File tree panel"; update the text in the
docs/releases.md release note where that phrase appears.
|
|
||
| ## Context Management Tools | ||
|
|
||
| 📖 Added context management options from tab right-click menu. You can now compress, merge, and transfer contexts between agents. You will received (configurable) warnings at 60% and 80% context consumption with a hint to compact. |
There was a problem hiding this comment.
Grammar issue in sentence tense.
Line 114 should be “You will receive …” (not “will received”).
Suggested edit
-📖 Added context management options from tab right-click menu. You can now compress, merge, and transfer contexts between agents. You will received (configurable) warnings at 60% and 80% context consumption with a hint to compact.
+📖 Added context management options from tab right-click menu. You can now compress, merge, and transfer contexts between agents. You will receive (configurable) warnings at 60% and 80% context consumption with a hint to compact.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| 📖 Added context management options from tab right-click menu. You can now compress, merge, and transfer contexts between agents. You will received (configurable) warnings at 60% and 80% context consumption with a hint to compact. | |
| 📖 Added context management options from tab right-click menu. You can now compress, merge, and transfer contexts between agents. You will receive (configurable) warnings at 60% and 80% context consumption with a hint to compact. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/releases.md` at line 114, Update the sentence in the release notes that
reads “You will received (configurable) warnings…” to correct the grammar:
change “received” to “receive” so it reads “You will receive (configurable)
warnings at 60% and 80% context consumption with a hint to compact.” Make this
single-word correction in the releases.md entry mentioning context management
options.
| - @ file mentions now include documents from your Auto Run folder (which may not live in your agent working directory) 🗄️ | ||
| - The wizard is now capable of detecting and continuing on past started projects 🧙 | ||
| 🌳 Github Worktree support was added. Any agent bound to a Git repository has the option to enable worktrees, each of which show up as a sub-agent with their own write-lock and Auto Run capability. Now you can truly develop in parallel on the same project and issue PRs when you're ready, all from within Maestro. Huge improvement, major thanks to @petersilberman. |
There was a problem hiding this comment.
Use official capitalization for GitHub.
Line 135 uses Github; use GitHub.
Suggested edit
-🌳 Github Worktree support was added.
+🌳 GitHub Worktree support was added.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| 🌳 Github Worktree support was added. Any agent bound to a Git repository has the option to enable worktrees, each of which show up as a sub-agent with their own write-lock and Auto Run capability. Now you can truly develop in parallel on the same project and issue PRs when you're ready, all from within Maestro. Huge improvement, major thanks to @petersilberman. | |
| 🌳 GitHub Worktree support was added. Any agent bound to a Git repository has the option to enable worktrees, each of which show up as a sub-agent with their own write-lock and Auto Run capability. Now you can truly develop in parallel on the same project and issue PRs when you're ready, all from within Maestro. Huge improvement, major thanks to `@petersilberman`. |
🧰 Tools
🪛 LanguageTool
[uncategorized] ~135-~135: The official name of this software platform is spelled with a capital “H”.
Context: ...11.0** | Released December 22, 2025 🌳 Github Worktree support was added. Any agent b...
(GITHUB)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/releases.md` at line 135, Update the capitalization of "Github" to the
official "GitHub" in the release note sentence that starts "Github Worktree
support was added." — locate the string "Github Worktree support was added." in
docs/releases.md (the release note paragraph) and change "Github" -> "GitHub" so
the text reads "GitHub Worktree support was added."
src/main/ipc/handlers/git.ts
Outdated
Comment on lines
+1089
to
+1095
| const [toplevelResult, gitDirResult, gitCommonDirResult, branchResult] = | ||
| await Promise.all([ | ||
| execGit(['rev-parse', '--show-toplevel'], subdirPath, sshRemote), | ||
| execGit(['rev-parse', '--git-dir'], subdirPath, sshRemote), | ||
| execGit(['rev-parse', '--git-common-dir'], subdirPath, sshRemote), | ||
| execGit(['rev-parse', '--abbrev-ref', 'HEAD'], subdirPath, sshRemote), | ||
| ]); |
There was a problem hiding this comment.
Pass remoteCwd for SSH execGit calls in this parallel block.
In SSH mode, these calls currently pass subdirPath only as localCwd. execGit remote execution uses remoteCwd, so metadata can be resolved from the wrong remote directory.
Proposed fix
+ const effectiveRemoteCwd = sshRemote ? subdirPath : undefined;
const [toplevelResult, gitDirResult, gitCommonDirResult, branchResult] =
await Promise.all([
- execGit(['rev-parse', '--show-toplevel'], subdirPath, sshRemote),
- execGit(['rev-parse', '--git-dir'], subdirPath, sshRemote),
- execGit(['rev-parse', '--git-common-dir'], subdirPath, sshRemote),
- execGit(['rev-parse', '--abbrev-ref', 'HEAD'], subdirPath, sshRemote),
+ execGit(
+ ['rev-parse', '--show-toplevel'],
+ subdirPath,
+ sshRemote,
+ effectiveRemoteCwd
+ ),
+ execGit(['rev-parse', '--git-dir'], subdirPath, sshRemote, effectiveRemoteCwd),
+ execGit(
+ ['rev-parse', '--git-common-dir'],
+ subdirPath,
+ sshRemote,
+ effectiveRemoteCwd
+ ),
+ execGit(
+ ['rev-parse', '--abbrev-ref', 'HEAD'],
+ subdirPath,
+ sshRemote,
+ effectiveRemoteCwd
+ ),
]);Also apply the same effectiveRemoteCwd to the is-inside-work-tree call at Line 1077 for consistency in this handler.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const [toplevelResult, gitDirResult, gitCommonDirResult, branchResult] = | |
| await Promise.all([ | |
| execGit(['rev-parse', '--show-toplevel'], subdirPath, sshRemote), | |
| execGit(['rev-parse', '--git-dir'], subdirPath, sshRemote), | |
| execGit(['rev-parse', '--git-common-dir'], subdirPath, sshRemote), | |
| execGit(['rev-parse', '--abbrev-ref', 'HEAD'], subdirPath, sshRemote), | |
| ]); | |
| const effectiveRemoteCwd = sshRemote ? subdirPath : undefined; | |
| const [toplevelResult, gitDirResult, gitCommonDirResult, branchResult] = | |
| await Promise.all([ | |
| execGit( | |
| ['rev-parse', '--show-toplevel'], | |
| subdirPath, | |
| sshRemote, | |
| effectiveRemoteCwd | |
| ), | |
| execGit(['rev-parse', '--git-dir'], subdirPath, sshRemote, effectiveRemoteCwd), | |
| execGit( | |
| ['rev-parse', '--git-common-dir'], | |
| subdirPath, | |
| sshRemote, | |
| effectiveRemoteCwd | |
| ), | |
| execGit( | |
| ['rev-parse', '--abbrev-ref', 'HEAD'], | |
| subdirPath, | |
| sshRemote, | |
| effectiveRemoteCwd | |
| ), | |
| ]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/ipc/handlers/git.ts` around lines 1089 - 1095, The parallel execGit
calls (the Promise.all calling execGit(['rev-parse', ...]) and the earlier
is-inside-work-tree execGit call) are only passing subdirPath as the localCwd so
SSH mode ignores the intended remote working directory; update those execGit
invocations to pass effectiveRemoteCwd (the computed remote cwd used for SSH) as
the remoteCwd argument (alongside subdirPath as localCwd and sshRemote) so
metadata and rev-parse results resolve from the correct remote directory;
specifically modify the calls that reference execGit([...], subdirPath,
sshRemote) and the is-inside-work-tree execGit to use execGit([...], subdirPath,
sshRemote, effectiveRemoteCwd) (or the existing execGit signature that accepts
remoteCwd).
| const HEADING_REGEX = /^(#{1,6})\s+(.+)$/; | ||
| const HIGHLIGHT_TEXT_REGEX = /==([^=]+)==/g; | ||
| const FILE_PROTOCOL_REGEX = /^file:\/\//; | ||
| const CODE_LANGUAGE_REGEX = /language-(\w+)/; |
There was a problem hiding this comment.
Expand code-language regex to support non-word language IDs.
Line [71] captures only \w+, so code fences like c++, objective-c, or f# won’t be parsed correctly at Line [989] and may fall back to text.
Suggested fix
-const CODE_LANGUAGE_REGEX = /language-(\w+)/;
+const CODE_LANGUAGE_REGEX = /(?:^|\s)language-([A-Za-z0-9#+-]+)(?:\s|$)/;Also applies to: 989-990
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/components/FilePreview.tsx` at line 71, The CODE_LANGUAGE_REGEX
currently uses /\language-(\w+)/ which misses non-word language IDs like c++,
objective-c, or f#; update the regex (symbol: CODE_LANGUAGE_REGEX in
FilePreview.tsx) to capture any valid language token (for example
/language-([^\s`]+)/ or /language-([A-Za-z0-9.+#-]+)/) and update the usage
sites (the parsing logic around line where code fences are handled—referenced
near lines 989-990) so those language IDs are correctly extracted and passed to
the syntax highlighter.
Comment on lines
+1527
to
1534
| for (const _ of nodeMatches) { | ||
| if (matchCount === targetIndex) { | ||
| const parentElement = (textNode as Text).parentElement; | ||
| if (parentElement) { | ||
| parentElement.scrollIntoView({ behavior: 'smooth', block: 'center' }); | ||
| } | ||
| matchCount++; | ||
| return; | ||
| } |
There was a problem hiding this comment.
Avoid early effect return inside markdown fallback match scan.
At Line [1533], returning from inside the loop exits the effect before shared post-branch reset logic (Line [1541]). In fallback mode, this can leave stale matchElementsRef.current from prior views.
Suggested fix
- for (const _ of nodeMatches) {
- if (matchCount === targetIndex) {
- const parentElement = (textNode as Text).parentElement;
- if (parentElement) {
- parentElement.scrollIntoView({ behavior: 'smooth', block: 'center' });
- }
- return;
- }
- matchCount++;
- }
+ let foundTarget = false;
+ for (const _ of nodeMatches) {
+ if (matchCount === targetIndex) {
+ const parentElement = (textNode as Text).parentElement;
+ if (parentElement) {
+ parentElement.scrollIntoView({ behavior: 'smooth', block: 'center' });
+ }
+ foundTarget = true;
+ break;
+ }
+ matchCount++;
+ }
+ if (foundTarget) break;Also applies to: 1541-1541
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/components/FilePreview.tsx` around lines 1527 - 1534, In
FilePreview.tsx inside the effect that scans markdown fallback matches (the loop
iterating nodeMatches using matchCount and targetIndex), remove the early return
when the target is found and instead break out of the loop so the shared
post-branch reset logic always runs; ensure you still perform the scrollIntoView
on the found node's parentElement (if present) before breaking and then let the
code after the loop update or clear matchElementsRef.current so stale elements
are not left between views.
Comment on lines
191
to
+195
| <div | ||
| ref={isSelected ? (selectedItemRef as React.RefObject<HTMLDivElement>) : null} | ||
| onClick={() => onSessionClick(session)} | ||
| className="w-full text-left px-6 py-4 flex items-start gap-4 hover:bg-white/5 transition-colors border-b group cursor-pointer" | ||
| style={{ | ||
| backgroundColor: isSelected ? theme.colors.accent + '15' : 'transparent', | ||
| borderColor: theme.colors.border + '50', | ||
| }} | ||
| style={styles.container} |
There was a problem hiding this comment.
Interactive row is still not keyboard-focus compliant
Line 193 makes the row clickable, but the container at Line 191 is a plain div without tabIndex and focus handling. This blocks keyboard-only navigation/interaction parity for the row itself.
As per coding guidelines src/renderer/components/**/*.{ts,tsx}: Add tabIndex attribute and focus event handlers when implementing components that need keyboard focus.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/components/SessionListItem.tsx` around lines 191 - 195, The
interactive row in SessionListItem is a plain div (ref={isSelected ?
(selectedItemRef as React.RefObject<HTMLDivElement>) : null}, onClick={() =>
onSessionClick(session)}, style={styles.container}) and needs keyboard focus
support: add tabIndex={0} to the container, wire up onFocus/onBlur to forward
focus state if needed (use selectedItemRef or component state), and add an
onKeyDown handler that calls onSessionClick(session) when Enter or Space is
pressed to match onClick behavior; ensure accessibility roles/aria-selected are
set if this represents a selectable item.
There was a problem hiding this comment.
♻️ Duplicate comments (4)
src/renderer/components/SessionList.tsx (2)
1744-1755:⚠️ Potential issue | 🔴 CriticalInvalid style path (
sessionListStyles.skinny) causes runtime failure.Lines 1744, 1748, 1754, 1774, 1775, and 3190 reference
sessionListStyles.skinny.*, but those keys are defined undersessionListStyles.root.*(Line 1617, 1620, 1624). This path is undefined.🐛 Required fix
- ...sessionListStyles.skinny.skinnyDotFallback, + ...sessionListStyles.root.skinnyDotFallback, @@ - ...sessionListStyles.skinny.skinnyDotFallback, + ...sessionListStyles.root.skinnyDotFallback, @@ - ...sessionListStyles.skinny.skinnyTooltip, + ...sessionListStyles.root.skinnyTooltip, @@ - sessionListStyles.skinny.skinnyDotFallback, - sessionListStyles.skinny.skinnyTooltip, + sessionListStyles.root.skinnyDotFallback, + sessionListStyles.root.skinnyTooltip, @@ - style={sessionListStyles.skinny.unreadBadge} + style={sessionListStyles.root.unreadBadge}Also applies to: 1774-1775, 3190-3190
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/SessionList.tsx` around lines 1744 - 1755, The runtime failure is caused by using the wrong style path: replace references to sessionListStyles.skinny (e.g., skinnyDotFallback, skinnyTooltip) with the correct path under sessionListStyles.root.skinny (so use sessionListStyles.root.skinny.skinnyDotFallback and sessionListStyles.root.skinny.skinnyTooltip, etc.) wherever sessionListStyles.skinny.* is referenced (notably in the tooltip and dot style objects); update all occurrences (including the ones around tooltip/left and opacity settings) to use sessionListStyles.root.skinny.* so the keys resolve correctly.
878-947:⚠️ Potential issue | 🟠 Major
tooltipStylesmemo can render stale context usage styles.At Line 946, dependencies only include
theme.colors, but the memo also derives styles fromsession.contextUsage,contextWarningYellowThreshold, andcontextWarningRedThreshold(Line 879–883, 930). This can keep width/color stale.🐛 Proposed fix
- }, [theme.colors]); + }, [ + theme.colors, + session.contextUsage, + contextWarningYellowThreshold, + contextWarningRedThreshold, + ]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/SessionList.tsx` around lines 878 - 947, The useMemo for tooltipStyles can return stale styles because its dependency array only includes theme.colors while the computed object also uses session.contextUsage, contextWarningYellowThreshold and contextWarningRedThreshold; update the useMemo dependencies to include session.contextUsage, contextWarningYellowThreshold, and contextWarningRedThreshold (along with theme.colors) so tooltipStyles recalculates when those values change (identify the useMemo that defines tooltipStyles and the call to getContextColor(session.contextUsage, theme, contextWarningYellowThreshold, contextWarningRedThreshold) to locate the exact spot).src/__tests__/main/stats/stats-db.test.ts (2)
823-825:⚠️ Potential issue | 🟠 MajorAwait
restoreFromBackup()to avoid Promise/race assertions.Current assertions may run before async restore work completes.
🐛 Proposed fix
- const result = db.restoreFromBackup('/path/to/nonexistent/backup'); + const result = await db.restoreFromBackup('/path/to/nonexistent/backup'); expect(result).toBe(false); - db.restoreFromBackup('/path/to/backup'); + await db.restoreFromBackup('/path/to/backup'); expect(mockDb.close).toHaveBeenCalled(); - db.restoreFromBackup('/path/to/backup.db'); + await db.restoreFromBackup('/path/to/backup.db'); expect(mockFsCopyFileSync).toHaveBeenCalledWith( '/path/to/backup.db', expect.stringContaining('stats.db') ); - db.restoreFromBackup('/path/to/backup.db'); + await db.restoreFromBackup('/path/to/backup.db'); expect(mockFsUnlinkSync).toHaveBeenCalled();#!/bin/bash # Verify unawaited restoreFromBackup calls in this test file. rg -nP '^\s*(const\s+\w+\s*=\s*)?db\.restoreFromBackup\(' src/__tests__/main/stats/stats-db.test.ts # Expected after fix: no outputAlso applies to: 832-835, 842-847, 855-859
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/main/stats/stats-db.test.ts` around lines 823 - 825, The test is calling db.restoreFromBackup(...) without awaiting the returned Promise which can cause assertions to run before the async work completes; update each occurrence of db.restoreFromBackup (including the instances around the referenced ranges) to await the call (or return the Promise from the test) so the test runner waits for completion, e.g., replace unawaited calls to db.restoreFromBackup in the test cases with awaited calls or return the Promise from the test function to ensure assertions run after restoreFromBackup resolves or rejects.
747-748:⚠️ Potential issue | 🟠 MajorAwait
getAvailableBackups()before assertions.These tests are asserting on the returned Promise instead of resolved backup arrays.
🐛 Proposed fix
- const backups = db.getAvailableBackups(); + const backups = await db.getAvailableBackups(); expect(backups).toEqual([]); - const backups = db.getAvailableBackups(); + const backups = await db.getAvailableBackups(); expect(backups).toHaveLength(3); - const backups = db.getAvailableBackups(); + const backups = await db.getAvailableBackups(); expect(backups).toHaveLength(1); - const backups = db.getAvailableBackups(); + const backups = await db.getAvailableBackups(); expect(backups[0].date).toBe('2026-02-01');#!/bin/bash # Verify unawaited getAvailableBackups calls in this test file. rg -nP '^\s*const\s+\w+\s*=\s*db\.getAvailableBackups\(\)' src/__tests__/main/stats/stats-db.test.ts # Expected after fix: no outputAlso applies to: 767-768, 784-785, 805-809
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/main/stats/stats-db.test.ts` around lines 747 - 748, The tests call db.getAvailableBackups() but do not await the returned Promise; update each occurrence (e.g., the variable assignments in the test that use getAvailableBackups) to await the call (replace assignments like const backups = db.getAvailableBackups() with awaiting the Promise) before performing expect assertions, and apply the same change for the other occurrences referenced (around the similar assertions at the other locations) so assertions operate on resolved arrays rather than Promises.
🧹 Nitpick comments (3)
prompt.XXXXXX.txt (2)
17-17: Condense duplicated failure text to unique actionable entries.The payload repeats identical error blocks several times. Keeping one instance per unique failure (with count) improves readability and review efficiency.
✂️ Example simplification
-"notes":"src/main/stats/stats-db.ts:13:failure Error: ... (repeated many times) ..." +"notes":"[x5] src/main/stats/stats-db.ts:13: Error: [vitest] No \"default\" export is defined on the \"fs\" mock. ...",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@prompt.XXXXXX.txt` at line 17, The failures array in the PR payload contains repeated identical error blocks; deduplicate them when constructing the payload by collapsing entries with the same unique key (e.g., combine source + id + name + htmlUrl + notes) into a single entry and add a count field for how many times it occurred. Locate the code that builds or serializes the failures array (look for the variable/parameter named failures or the function that collects check-run results) and replace the push/pop logic with a reducer or Map that groups by the unique key, emits one representative object per group, and sets count; ensure tests that consume failures still read the new count property or handle missing duplicates.
1-17: Avoid committing ephemeral CI-agent prompt snapshots to the repo.This file is tightly coupled to one CI run (
head SHA, check-run IDs, timestamps) and will age out immediately, creating maintenance noise unrelated to shipped behavior.🧹 Proposed minimal change
-You are an autonomous coding agent fixing a GitHub pull request for Maestro. -... -{"repo":"RunMaestro/Maestro","number":496, ...} +<!-- Remove this generated triage artifact from the tracked source tree. --> +<!-- Keep CI guidance in workflow logs/artifacts or internal runbook docs instead. -->🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@prompt.XXXXXX.txt` around lines 1 - 17, Delete the ephemeral CI snapshot file prompt.XXXXXX.txt from the commit (remove it from the repo history/HEAD) and add a rule to ignore such files (for example add "prompt.*.txt" or a more specific pattern) to .gitignore so future CI snapshots aren’t committed; ensure you remove any staged/tracked copy (git rm or equivalent) and commit the .gitignore update and removal together, and verify no code references the file name.src/__tests__/main/stats/stats-db.test.ts (1)
731-732: UsemockResolvedValueformockFsReaddirto keep async contract explicit.
mockFsReaddirrepresentsfs.promises.readdir(confirmed at line 102); usingmockReturnValuereturns raw arrays instead of promises. Whileawaitworks on raw arrays,mockResolvedValuemaintains explicit async fidelity in tests.Also applies to: 741-742, 757-761, 778-779, 795-799
♻️ Proposed refactor
- mockFsReaddir.mockReturnValue([]); + mockFsReaddir.mockResolvedValue([]); - mockFsReaddir.mockReturnValue([]); + mockFsReaddir.mockResolvedValue([]); - mockFsReaddir.mockReturnValue([ + mockFsReaddir.mockResolvedValue([ 'stats.db.daily.2026-02-01', 'stats.db.daily.2026-02-02', 'stats.db.daily.2026-02-03', ]); - mockFsReaddir.mockReturnValue([`stats.db.backup.${timestamp}`]); + mockFsReaddir.mockResolvedValue([`stats.db.backup.${timestamp}`]); - mockFsReaddir.mockReturnValue([ + mockFsReaddir.mockResolvedValue([ 'stats.db.daily.2026-01-15', 'stats.db.daily.2026-02-01', 'stats.db.daily.2026-01-20', ]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/main/stats/stats-db.test.ts` around lines 731 - 732, The test mocks for fs.promises.readdir use mockReturnValue, which returns raw arrays and breaks the async contract; update every use of mockFsReaddir in this test file (the mock for fs.promises.readdir) to use mockResolvedValue(...) instead of mockReturnValue(...) so the mock returns a resolved Promise (do this for the occurrences currently at mockFsReaddir.mockReturnValue(...) and the other spots noted in the comment); keep the same values passed in but switch to mockResolvedValue to make the async behavior explicit.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/__tests__/main/stats/stats-db.test.ts`:
- Around line 823-825: The test is calling db.restoreFromBackup(...) without
awaiting the returned Promise which can cause assertions to run before the async
work completes; update each occurrence of db.restoreFromBackup (including the
instances around the referenced ranges) to await the call (or return the Promise
from the test) so the test runner waits for completion, e.g., replace unawaited
calls to db.restoreFromBackup in the test cases with awaited calls or return the
Promise from the test function to ensure assertions run after restoreFromBackup
resolves or rejects.
- Around line 747-748: The tests call db.getAvailableBackups() but do not await
the returned Promise; update each occurrence (e.g., the variable assignments in
the test that use getAvailableBackups) to await the call (replace assignments
like const backups = db.getAvailableBackups() with awaiting the Promise) before
performing expect assertions, and apply the same change for the other
occurrences referenced (around the similar assertions at the other locations) so
assertions operate on resolved arrays rather than Promises.
In `@src/renderer/components/SessionList.tsx`:
- Around line 1744-1755: The runtime failure is caused by using the wrong style
path: replace references to sessionListStyles.skinny (e.g., skinnyDotFallback,
skinnyTooltip) with the correct path under sessionListStyles.root.skinny (so use
sessionListStyles.root.skinny.skinnyDotFallback and
sessionListStyles.root.skinny.skinnyTooltip, etc.) wherever
sessionListStyles.skinny.* is referenced (notably in the tooltip and dot style
objects); update all occurrences (including the ones around tooltip/left and
opacity settings) to use sessionListStyles.root.skinny.* so the keys resolve
correctly.
- Around line 878-947: The useMemo for tooltipStyles can return stale styles
because its dependency array only includes theme.colors while the computed
object also uses session.contextUsage, contextWarningYellowThreshold and
contextWarningRedThreshold; update the useMemo dependencies to include
session.contextUsage, contextWarningYellowThreshold, and
contextWarningRedThreshold (along with theme.colors) so tooltipStyles
recalculates when those values change (identify the useMemo that defines
tooltipStyles and the call to getContextColor(session.contextUsage, theme,
contextWarningYellowThreshold, contextWarningRedThreshold) to locate the exact
spot).
---
Nitpick comments:
In `@prompt.XXXXXX.txt`:
- Line 17: The failures array in the PR payload contains repeated identical
error blocks; deduplicate them when constructing the payload by collapsing
entries with the same unique key (e.g., combine source + id + name + htmlUrl +
notes) into a single entry and add a count field for how many times it occurred.
Locate the code that builds or serializes the failures array (look for the
variable/parameter named failures or the function that collects check-run
results) and replace the push/pop logic with a reducer or Map that groups by the
unique key, emits one representative object per group, and sets count; ensure
tests that consume failures still read the new count property or handle missing
duplicates.
- Around line 1-17: Delete the ephemeral CI snapshot file prompt.XXXXXX.txt from
the commit (remove it from the repo history/HEAD) and add a rule to ignore such
files (for example add "prompt.*.txt" or a more specific pattern) to .gitignore
so future CI snapshots aren’t committed; ensure you remove any staged/tracked
copy (git rm or equivalent) and commit the .gitignore update and removal
together, and verify no code references the file name.
In `@src/__tests__/main/stats/stats-db.test.ts`:
- Around line 731-732: The test mocks for fs.promises.readdir use
mockReturnValue, which returns raw arrays and breaks the async contract; update
every use of mockFsReaddir in this test file (the mock for fs.promises.readdir)
to use mockResolvedValue(...) instead of mockReturnValue(...) so the mock
returns a resolved Promise (do this for the occurrences currently at
mockFsReaddir.mockReturnValue(...) and the other spots noted in the comment);
keep the same values passed in but switch to mockResolvedValue to make the async
behavior explicit.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
payload.XXXXXX.jsonprompt.XXXXXX.txtsrc/__tests__/main/stats/stats-db.test.tssrc/renderer/components/SessionList.tsx
✅ Files skipped from review due to trivial changes (1)
- payload.XXXXXX.json
…mm85yqkw # Conflicts: # src/__tests__/main/parsers/index.test.ts
| session={session} | ||
| index={i} | ||
| selectedIndex={selectedIndex} | ||
| isSelected={i === selectedIndex} |
There was a problem hiding this comment.
TypeScript prop interface mismatch will break compilation
isSelected is not a property in SessionListItemProps, and the two required props index: number and selectedIndex: number are no longer being passed. This will produce TypeScript errors:
Property 'index' is missing in type '{ isSelected: boolean; ... }'
Object literal may only specify known properties, and 'isSelected' does not exist in type 'SessionListItemProps'
The SessionListItemProps interface in SessionListItem.tsx was not updated alongside this change — it still declares index and selectedIndex as required fields and has no isSelected field. Either revert this call site to pass index={i} and selectedIndex={selectedIndex}, or (the better path) also update SessionListItemProps to replace those two props with isSelected: boolean.
Suggested change
| isSelected={i === selectedIndex} | |
| index={i} | |
| selectedIndex={selectedIndex} |
Comment on lines
+1527
to
+1534
| drawerWrapperWithLeftBorder: { | ||
| borderLeft: `1px solid ${theme.colors.accent}30`, | ||
| borderBottom: `1px solid ${theme.colors.accent}30`, | ||
| }, | ||
| drawerWrapperNoLeftBorder: { | ||
| borderLeft: 'none', | ||
| borderBottom: `1px solid ${theme.colors.accent}30`, | ||
| }, |
There was a problem hiding this comment.
Missing backgroundColor in drawer wrapper styles — visual regression
drawerWrapper (line 1523) is the only style object that includes backgroundColor: \${theme.colors.accent}10`, but it is never referenced anywhere in the render code. The two variants that are actually used — drawerWrapperWithLeftBorderanddrawerWrapperNoLeftBorder— both omitbackgroundColor`. As a result the worktree-children drawer loses its accent-tinted background and becomes fully transparent, whereas the original inline style always applied it.
The fix is to spread the shared base into each variant:
drawerWrapperWithLeftBorder: {
...drawerWrapper,
borderLeft: `1px solid ${theme.colors.accent}30`,
},
drawerWrapperNoLeftBorder: {
...drawerWrapper,
borderLeft: 'none',
},
Or simply inline backgroundColor into both objects directly.
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.
Summary
Notes
Auto Run checklist
Summary by CodeRabbit
New Features
Improvements
Bug Fixes