perf: optimize SessionListItem style recalculation#496
Open
jeffscottward wants to merge 23 commits intoRunMaestro:mainfrom
Open
perf: optimize SessionListItem style recalculation#496jeffscottward wants to merge 23 commits intoRunMaestro:mainfrom
jeffscottward wants to merge 23 commits intoRunMaestro:mainfrom
Conversation
📝 WalkthroughWalkthroughThis PR converts many synchronous file- and process-related APIs to async/Promise-based flows, updates call sites to await results, adds session discovery and Codex config caching, changes StatsDB/HistoryManager to async, makes process spawning asynchronous, and applies extensive renderer component refactors (memoization and centralized styling). Tests were updated accordingly. 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')
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: ipcInvoke('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 spawned child process
Router->>RouterHandler: initiate spawn (moderator/participant)
RouterHandler->>ProcMgr: await processManager.spawn(config)
ProcMgr->>Spawner: spawn(config) (async)
Spawner->>OSProc: spawn process / prepare images (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
Suggested reviewers
🚥 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://cdn.statically.io/img/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.
| session={session} | ||
| index={i} | ||
| selectedIndex={selectedIndex} | ||
| isSelected={i === selectedIndex} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd SessionListItem.tsx src/renderer/componentsRepository: RunMaestro/Maestro
Length of output: 105
🏁 Script executed:
cd src/renderer/components && cat -n AgentSessionsBrowser.tsx | sed -n '1485,1500p'Repository: RunMaestro/Maestro
Length of output: 852
🏁 Script executed:
cd src/renderer/components && cat -n SessionListItem.tsx | sed -n '40,60p'Repository: RunMaestro/Maestro
Length of output: 894
🏁 Script executed:
cd src/renderer/components && cat -n SessionListItem.tsx | sed -n '44,80p'Repository: RunMaestro/Maestro
Length of output: 1900
🏁 Script executed:
cd src/renderer/components && cat -n AgentSessionsBrowser.tsx | sed -n '1488,1510p'Repository: RunMaestro/Maestro
Length of output: 1157
🏁 Script executed:
cd src/renderer/components && cat -n SessionListItem.tsx | sed -n '80,90p'Repository: RunMaestro/Maestro
Length of output: 406
Props mismatch with SessionListItemProps interface causes TypeScript compilation error.
The SessionListItemProps interface requires index: number and selectedIndex: number props, but this call site passes isSelected: boolean instead and omits the required index and selectedIndex props. This will cause a TypeScript error.
Either update the interface to replace index and selectedIndex with isSelected, or pass the required props from the call site.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/components/AgentSessionsBrowser.tsx` at 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).
Comment on lines
+152
to
+160
| const parts = text.split(searchRegex); | ||
| if (parts.length === 1) return text; | ||
| // Use running character offset as key to guarantee uniqueness across | ||
| // identical substrings appearing at different positions. | ||
| let offset = 0; | ||
| return parts.map((part) => { | ||
| return parts.map((part, index) => { | ||
| const key = offset; | ||
| offset += part.length; | ||
| return regex.test(part) ? ( | ||
| const isMatch = index % 2 === 1; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/renderer/components/CsvTableRenderer.tsx | sed -n '140,170p'Repository: RunMaestro/Maestro
Length of output: 1041
🏁 Script executed:
python3 << 'EOF'
import re
# Test cases to see if split can produce empty segments
test_cases = [
("hello world", r"\s+", "space separated"),
("a,b,c", r",", "comma separated"),
("a,,b", r",", "consecutive delimiters"),
("hello", r"l", "substring match"),
("test", r"e|s", "alternation"),
("aaa", r"a", "all chars match"),
("aaa", r"a*", "zero-width match"),
]
print("Testing split() behavior with various regex patterns:\n")
for text, pattern, desc in test_cases:
parts = re.split(pattern, text)
print(f"Text: '{text}' | Pattern: {pattern} | {desc}")
print(f" Parts: {parts}")
print(f" Empty parts present: {'' in parts}")
# Simulate the offset key calculation
offset = 0
offsets = []
for part in parts:
offsets.append(offset)
offset += len(part)
print(f" Offset sequence: {offsets}")
print(f" Duplicate offsets: {len(offsets) != len(set(offsets))}")
print()
EOFRepository: RunMaestro/Maestro
Length of output: 1206
🏁 Script executed:
rg "highlightMatches" src/renderer/components/CsvTableRenderer.tsx -A 2 -B 2Repository: RunMaestro/Maestro
Length of output: 369
🏁 Script executed:
rg "searchMatchRegex" src/renderer/components/CsvTableRenderer.tsx -B 3 -A 3Repository: RunMaestro/Maestro
Length of output: 627
🏁 Script executed:
rg "REGEX_ESCAPE_REGEX" src/renderer/components/CsvTableRenderer.tsx -B 2 -A 2Repository: RunMaestro/Maestro
Length of output: 406
Use a collision-safe key for highlighted segments.
The key={offset} at line 158 can duplicate when split() yields empty segments (e.g., when the match occurs at the string start, end, or when consecutive matches appear). This causes React key collisions and potential rendering issues.
Proposed fix
- const key = offset;
+ const key = `${offset}-${index}`;🤖 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 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.
Comment on lines
+121
to
+173
| const styles = useMemo(() => { | ||
| return { | ||
| container: { | ||
| backgroundColor: theme.colors.bgSidebar, | ||
| borderColor: theme.colors.border, | ||
| }, | ||
| textMain: { | ||
| color: theme.colors.textMain, | ||
| }, | ||
| textAccent: { | ||
| color: theme.colors.accent, | ||
| }, | ||
| textError: { | ||
| color: theme.colors.error, | ||
| }, | ||
| divider: { | ||
| borderColor: theme.colors.border, | ||
| }, | ||
| submenu: { | ||
| backgroundColor: theme.colors.bgSidebar, | ||
| borderColor: theme.colors.border, | ||
| }, | ||
| submenuBelowLeft: { | ||
| backgroundColor: theme.colors.bgSidebar, | ||
| borderColor: theme.colors.border, | ||
| top: 0, | ||
| left: '100%', | ||
| marginLeft: 4, | ||
| }, | ||
| submenuBelowRight: { | ||
| backgroundColor: theme.colors.bgSidebar, | ||
| borderColor: theme.colors.border, | ||
| top: 0, | ||
| right: '100%', | ||
| marginRight: 4, | ||
| }, | ||
| submenuAboveLeft: { | ||
| backgroundColor: theme.colors.bgSidebar, | ||
| borderColor: theme.colors.border, | ||
| bottom: 0, | ||
| left: '100%', | ||
| marginLeft: 4, | ||
| }, | ||
| submenuAboveRight: { | ||
| backgroundColor: theme.colors.bgSidebar, | ||
| borderColor: theme.colors.border, | ||
| bottom: 0, | ||
| right: '100%', | ||
| marginRight: 4, | ||
| }, | ||
| }; | ||
| }, [theme.colors, session.contextUsage, contextWarningYellowThreshold, contextWarningRedThreshold]); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/renderer/components/SessionList.tsx | head -200Repository: RunMaestro/Maestro
Length of output: 6971
🏁 Script executed:
grep -n "contextWarningYellowThreshold\|contextWarningRedThreshold" src/renderer/components/SessionList.tsxRepository: RunMaestro/Maestro
Length of output: 961
🏁 Script executed:
sed -n '88,107p' src/renderer/components/SessionList.tsxRepository: RunMaestro/Maestro
Length of output: 365
🏁 Script executed:
sed -n '121,172p' src/renderer/components/SessionList.tsx | grep -i "contextWarning\|contextUsage"Repository: RunMaestro/Maestro
Length of output: 163
🏁 Script executed:
sed -n '121,172p' src/renderer/components/SessionList.tsxRepository: RunMaestro/Maestro
Length of output: 1296
Remove undefined identifiers from useMemo dependency array.
Line 172 references contextWarningYellowThreshold, contextWarningRedThreshold, and session.contextUsage in the dependency array, but these identifiers do not exist in the SessionContextMenu function scope and are not used in the styles object. This will cause a runtime error.
Proposed fix
- }, [theme.colors, session.contextUsage, contextWarningYellowThreshold, contextWarningRedThreshold]);
+ }, [theme.colors]);📝 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 styles = useMemo(() => { | |
| return { | |
| container: { | |
| backgroundColor: theme.colors.bgSidebar, | |
| borderColor: theme.colors.border, | |
| }, | |
| textMain: { | |
| color: theme.colors.textMain, | |
| }, | |
| textAccent: { | |
| color: theme.colors.accent, | |
| }, | |
| textError: { | |
| color: theme.colors.error, | |
| }, | |
| divider: { | |
| borderColor: theme.colors.border, | |
| }, | |
| submenu: { | |
| backgroundColor: theme.colors.bgSidebar, | |
| borderColor: theme.colors.border, | |
| }, | |
| submenuBelowLeft: { | |
| backgroundColor: theme.colors.bgSidebar, | |
| borderColor: theme.colors.border, | |
| top: 0, | |
| left: '100%', | |
| marginLeft: 4, | |
| }, | |
| submenuBelowRight: { | |
| backgroundColor: theme.colors.bgSidebar, | |
| borderColor: theme.colors.border, | |
| top: 0, | |
| right: '100%', | |
| marginRight: 4, | |
| }, | |
| submenuAboveLeft: { | |
| backgroundColor: theme.colors.bgSidebar, | |
| borderColor: theme.colors.border, | |
| bottom: 0, | |
| left: '100%', | |
| marginLeft: 4, | |
| }, | |
| submenuAboveRight: { | |
| backgroundColor: theme.colors.bgSidebar, | |
| borderColor: theme.colors.border, | |
| bottom: 0, | |
| right: '100%', | |
| marginRight: 4, | |
| }, | |
| }; | |
| }, [theme.colors, session.contextUsage, contextWarningYellowThreshold, contextWarningRedThreshold]); | |
| const styles = useMemo(() => { | |
| return { | |
| container: { | |
| backgroundColor: theme.colors.bgSidebar, | |
| borderColor: theme.colors.border, | |
| }, | |
| textMain: { | |
| color: theme.colors.textMain, | |
| }, | |
| textAccent: { | |
| color: theme.colors.accent, | |
| }, | |
| textError: { | |
| color: theme.colors.error, | |
| }, | |
| divider: { | |
| borderColor: theme.colors.border, | |
| }, | |
| submenu: { | |
| backgroundColor: theme.colors.bgSidebar, | |
| borderColor: theme.colors.border, | |
| }, | |
| submenuBelowLeft: { | |
| backgroundColor: theme.colors.bgSidebar, | |
| borderColor: theme.colors.border, | |
| top: 0, | |
| left: '100%', | |
| marginLeft: 4, | |
| }, | |
| submenuBelowRight: { | |
| backgroundColor: theme.colors.bgSidebar, | |
| borderColor: theme.colors.border, | |
| top: 0, | |
| right: '100%', | |
| marginRight: 4, | |
| }, | |
| submenuAboveLeft: { | |
| backgroundColor: theme.colors.bgSidebar, | |
| borderColor: theme.colors.border, | |
| bottom: 0, | |
| left: '100%', | |
| marginLeft: 4, | |
| }, | |
| submenuAboveRight: { | |
| backgroundColor: theme.colors.bgSidebar, | |
| borderColor: theme.colors.border, | |
| bottom: 0, | |
| right: '100%', | |
| marginRight: 4, | |
| }, | |
| }; | |
| }, [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 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.
Comment on lines
1573
to
1714
| const sessionListStyles = useMemo(() => { | ||
| return { | ||
| root: { | ||
| container: { | ||
| backgroundColor: theme.colors.bgSidebar, | ||
| borderColor: theme.colors.border, | ||
| '--tw-ring-color': theme.colors.accent, | ||
| }, | ||
| headerBorder: { | ||
| borderColor: theme.colors.border, | ||
| }, | ||
| iconAccent: { | ||
| color: theme.colors.accent, | ||
| }, | ||
| textMain: { | ||
| color: theme.colors.textMain, | ||
| }, | ||
| borderDim: { | ||
| color: theme.colors.textDim, | ||
| }, | ||
| borderError: { | ||
| color: theme.colors.error, | ||
| }, | ||
| badgeColor: { | ||
| color: '#FFD700', | ||
| }, | ||
| menuButtonColor: { | ||
| color: theme.colors.textDim, | ||
| }, | ||
| actionDivider: { | ||
| borderColor: theme.colors.border, | ||
| }, | ||
| bottomDivider: { | ||
| borderColor: theme.colors.border, | ||
| }, | ||
| bottomPrimaryAction: { | ||
| backgroundColor: theme.colors.accent, | ||
| color: theme.colors.accentForeground, | ||
| }, | ||
| bottomCollapse: { | ||
| borderColor: theme.colors.border, | ||
| }, | ||
| sidebarFilterInput: { | ||
| borderColor: theme.colors.accent, | ||
| color: theme.colors.textMain, | ||
| }, | ||
| sectionHeaderAccent: { | ||
| color: theme.colors.accent, | ||
| }, | ||
| sectionHeaderDim: { | ||
| color: theme.colors.textDim, | ||
| }, | ||
| sectionHeaderAccentFill: theme.colors.accent, | ||
| sectionBorderAccent: { | ||
| borderColor: theme.colors.accent, | ||
| }, | ||
| sectionBorder: { | ||
| borderColor: theme.colors.border, | ||
| }, | ||
| groupDelete: { | ||
| color: theme.colors.error, | ||
| }, | ||
| newGroupButton: { | ||
| backgroundColor: `${theme.colors.accent}20`, | ||
| color: theme.colors.accent, | ||
| border: `1px solid ${theme.colors.accent}40`, | ||
| }, | ||
| dropZone: { | ||
| borderColor: theme.colors.accent, | ||
| color: theme.colors.textDim, | ||
| backgroundColor: `${theme.colors.accent}10`, | ||
| }, | ||
| unreadBadge: { | ||
| backgroundColor: theme.colors.error, | ||
| }, | ||
| skinnyDotFallback: { | ||
| border: `1.5px solid ${theme.colors.textDim}`, | ||
| backgroundColor: 'transparent', | ||
| }, | ||
| skinnyTooltip: { | ||
| minWidth: '240px', | ||
| backgroundColor: theme.colors.bgSidebar, | ||
| border: `1px solid ${theme.colors.border}`, | ||
| }, | ||
| live: { | ||
| overlay: { | ||
| width: '280px', | ||
| }, | ||
| panel: { | ||
| backgroundColor: theme.colors.bgSidebar, | ||
| border: `1px solid ${theme.colors.border}`, | ||
| }, | ||
| panelSectionBorder: { | ||
| borderColor: theme.colors.border, | ||
| }, | ||
| panelTextDim: { | ||
| color: theme.colors.textDim, | ||
| }, | ||
| installBanner: { | ||
| backgroundColor: theme.colors.bgActivity, | ||
| }, | ||
| customPortInput: { | ||
| backgroundColor: theme.colors.bgActivity, | ||
| borderColor: theme.colors.border, | ||
| color: theme.colors.textMain, | ||
| }, | ||
| customPortCaption: { | ||
| color: theme.colors.textDim, | ||
| opacity: 0.7, | ||
| }, | ||
| urlPillContainer: { | ||
| backgroundColor: theme.colors.bgActivity, | ||
| }, | ||
| tabInactiveText: { | ||
| color: theme.colors.textDim, | ||
| }, | ||
| overlayMenuPanel: { | ||
| backgroundColor: theme.colors.bgSidebar, | ||
| border: `1px solid ${theme.colors.border}`, | ||
| maxHeight: 'calc(100vh - 90px)', | ||
| }, | ||
| copyActionButton: { | ||
| color: theme.colors.textDim, | ||
| }, | ||
| openBrowserLocal: { | ||
| color: '#4ade80', | ||
| borderColor: 'rgba(74, 222, 128, 0.3)', | ||
| }, | ||
| openBrowserRemote: { | ||
| color: '#60a5fa', | ||
| borderColor: 'rgba(96, 165, 250, 0.3)', | ||
| }, | ||
| webIcon: { | ||
| color: theme.colors.textDim, | ||
| }, | ||
| }, | ||
| badge: { | ||
| color: theme.colors.accent, | ||
| }, | ||
| }, | ||
| }; | ||
| }, [theme.colors]); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
wc -l src/renderer/components/SessionList.tsxRepository: RunMaestro/Maestro
Length of output: 106
🏁 Script executed:
# First, let's see the sessionListStyles definition and the lines around 1573-1714
sed -n '1573,1714p' src/renderer/components/SessionList.tsx | head -50Repository: RunMaestro/Maestro
Length of output: 1210
🏁 Script executed:
# Search for all occurrences of "sessionListStyles.skinny" in the file
rg 'sessionListStyles\.skinny' src/renderer/components/SessionList.tsx -n -A 2 -B 2Repository: RunMaestro/Maestro
Length of output: 772
🏁 Script executed:
# Check the structure of sessionListStyles - look for where 'skinny' property is defined
rg 'skinny:' src/renderer/components/SessionList.tsx -n -A 5Repository: RunMaestro/Maestro
Length of output: 44
🏁 Script executed:
# Also check the full sessionListStyles object structure
sed -n '1573,1750p' src/renderer/components/SessionList.tsxRepository: RunMaestro/Maestro
Length of output: 5327
Fix incorrect style object path references in skinny mode.
The code accesses sessionListStyles.skinny.* on lines 3103, 3119, and 3129, but this property does not exist in the sessionListStyles object. All style definitions, including skinnyDotFallback, unreadBadge, and skinnyTooltip, are nested under root. Using the non-existent skinny path results in undefined property access and breaks rendering in collapsed mode.
Change all occurrences to use sessionListStyles.root.* instead:
🐛 Required fix
- ...sessionListStyles.skinny.skinnyDotFallback,
+ ...sessionListStyles.root.skinnyDotFallback,
@@
- style={sessionListStyles.skinny.unreadBadge}
+ style={sessionListStyles.root.unreadBadge}
@@
- ...sessionListStyles.skinny.skinnyTooltip,
+ ...sessionListStyles.root.skinnyTooltip,🤖 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 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.
Greptile SummaryThis PR bundles two independent workstreams: (1) a The
The async Confidence Score: 2/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["session.contextUsage changes"] --> B{"useMemo dep array\ncontains contextUsage?"}
B -- "No (bug)" --> C["tooltipStyles NOT recomputed\n→ bar width frozen at initial value"]
B -- "Yes (fix)" --> D["tooltipStyles recomputed\n→ bar width updates correctly"]
E["s.state changes → getStatusColor"] --> F{"collapsedPillStyles.segment.base\nspread order"}
F -- "After getStatusColor (bug)" --> G["backgroundColor overridden\nwith 'transparent'"]
F -- "Before getStatusColor (fix)" --> H["backgroundColor = status color\n(correct)"]
style C fill:#f66,color:#fff
style G fill:#f66,color:#fff
style D fill:#6a6,color:#fff
style H fill:#6a6,color:#fff
Last reviewed commit: 117a782 |
![greptile-apps[bot]](https://cdn.statically.io/img/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.
Comment on lines
+535
to
+559
| const styles = useMemo(() => { | ||
| return { | ||
| icon: { | ||
| color: theme.colors.accent, | ||
| }, | ||
| textMain: { | ||
| color: theme.colors.textMain, | ||
| }, | ||
| textDim: { | ||
| color: theme.colors.textDim, | ||
| }, | ||
| divider: { | ||
| borderColor: theme.colors.border, | ||
| }, | ||
| shortcutBadge: { | ||
| backgroundColor: theme.colors.bgActivity, | ||
| color: theme.colors.textDim, | ||
| }, | ||
| }; | ||
| }, [ | ||
| theme.colors, | ||
| session.contextUsage, | ||
| contextWarningYellowThreshold, | ||
| contextWarningRedThreshold, | ||
| ]); |
There was a problem hiding this comment.
Undefined identifiers in HamburgerMenuContent styles dependency array.
The styles useMemo references session.contextUsage, contextWarningYellowThreshold, and contextWarningRedThreshold in its dependency array (lines 554-559), but none of these exist in HamburgerMenuContent's scope. The component only receives theme, onNewAgentSession, openWizard, startTour, and setMenuOpen as props. This appears to be copy-paste residue from SessionContextMenu.
🐛 Proposed fix
}, [
- theme.colors,
- session.contextUsage,
- contextWarningYellowThreshold,
- contextWarningRedThreshold,
- ]);
+ theme.colors,
+ ]);📝 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 styles = useMemo(() => { | |
| return { | |
| icon: { | |
| color: theme.colors.accent, | |
| }, | |
| textMain: { | |
| color: theme.colors.textMain, | |
| }, | |
| textDim: { | |
| color: theme.colors.textDim, | |
| }, | |
| divider: { | |
| borderColor: theme.colors.border, | |
| }, | |
| shortcutBadge: { | |
| backgroundColor: theme.colors.bgActivity, | |
| color: theme.colors.textDim, | |
| }, | |
| }; | |
| }, [ | |
| theme.colors, | |
| session.contextUsage, | |
| contextWarningYellowThreshold, | |
| contextWarningRedThreshold, | |
| ]); | |
| const styles = useMemo(() => { | |
| return { | |
| icon: { | |
| color: theme.colors.accent, | |
| }, | |
| textMain: { | |
| color: theme.colors.textMain, | |
| }, | |
| textDim: { | |
| color: theme.colors.textDim, | |
| }, | |
| divider: { | |
| borderColor: theme.colors.border, | |
| }, | |
| shortcutBadge: { | |
| backgroundColor: theme.colors.bgActivity, | |
| color: theme.colors.textDim, | |
| }, | |
| }; | |
| }, [ | |
| 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 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.
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