Adopt async file IO for history and stats#385
Open
openasocket wants to merge 34 commits intoRunMaestro:mainfrom
Open
Adopt async file IO for history and stats#385openasocket wants to merge 34 commits intoRunMaestro:mainfrom
openasocket wants to merge 34 commits intoRunMaestro:mainfrom
Conversation
Collaborator
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughWidespread async migration and performance work: synchronous filesystem and spawn APIs converted to Promise-based async across main and tests; HistoryManager and StatsDB APIs became async; ProcessManager/ChildProcessSpawner and image helpers return Promises; renderer components gained React.memo/useMemo and style memoization; Codex config and agent session discovery caching added. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Collaborator
|
@greptile review |
|
✅ Actions performedReview triggered.
|
Greptile SummaryThis PR implements a comprehensive performance optimization sweep across React rendering, main process I/O, and database queries. All changes are behavior-preserving and follow established patterns from Key improvements:
Test coverage: All changes include comprehensive test updates. The async conversions required updating 15+ test files to use The Confidence Score: 5/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Performance Optimizations] --> B[React Layer]
A --> C[Main Process]
A --> D[Database Layer]
B --> B1[React.memo wraps]
B --> B2[useMemo for computations]
B --> B3[Style extraction]
B1 --> B1a[SessionListItem]
B1 --> B1b[ExecutionQueueIndicator]
B1 --> B1c[14 UsageDashboard charts]
B2 --> B2a[Session list sorting]
B2 --> B2b[Regex pre-compilation]
B2 --> B2c[Date parsing cache]
B3 --> B3a[Inline styles → useMemo]
C --> C1[Async File I/O]
C --> C2[Query Parallelization]
C --> C3[Session Discovery]
C1 --> C1a[history-manager.ts]
C1 --> C1b[stats-db.ts]
C1 --> C1c[imageUtils.ts]
C2 --> C2a[Git handlers - Promise.all]
C3 --> C3a[Batched scanning]
C3 --> C3b[30s cache TTL]
D --> D1[Compound Indexes]
D --> D2[Async Init/Vacuum]
D1 --> D1a[time+agent]
D1 --> D1b[time+project]
D1 --> D1c[time+source]
Last reviewed commit: f1e54db |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (11)
src/renderer/components/UsageDashboard/LongestAutoRunsTable.tsx (1)
128-138:⚠️ Potential issue | 🟠 MajorError handling swallows exceptions instead of bubbling to Sentry.
The
catchblock logs toconsole.errorand then continues, which violates the coding guideline requiring unexpected errors to bubble up to Sentry for tracking. For a data-fetching failure that the user cannot recover from directly, either let it bubble or explicitly report it via Sentry utilities.Proposed fix using Sentry utilities
+import { captureException } from '../../../utils/sentry'; + const fetchData = useCallback(async () => { setLoading(true); try { const autoRunSessions = await window.maestro.stats.getAutoRunSessions(timeRange); setSessions(autoRunSessions); } catch (err) { - console.error('Failed to fetch Auto Run sessions for table:', err); + captureException(err, { extra: { context: 'LongestAutoRunsTable fetchData', timeRange } }); + // Optionally set an error state to show user feedback } finally { setLoading(false); } }, [timeRange]);As per coding guidelines: "Do NOT silently swallow exceptions with try-catch-console.error blocks. Let unhandled exceptions bubble up to Sentry for error tracking."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/UsageDashboard/LongestAutoRunsTable.tsx` around lines 128 - 138, The fetchData function currently swallows errors in its catch by only calling console.error; replace that with error reporting to Sentry or rethrowing so exceptions are not silently ignored. Update the catch in fetchData (wrapping window.maestro.stats.getAutoRunSessions) to call the project's Sentry utility (e.g., Sentry.captureException or the app's captureError helper) with the caught err and a helpful message, then rethrow or allow the error to bubble instead of solely logging; ensure setLoading(false) still runs in finally and preserve existing setSessions behavior.src/renderer/components/SessionList.tsx (1)
1799-1813:⚠️ Potential issue | 🟡 MinorInconsistent indentation in button element.
The changed lines (1799-1800, 1804-1805) have different indentation than the unchanged lines within the same element (1801-1803, 1806). The
onClickhandler body andtitleattribute are more deeply indented than other attributes.As per coding guidelines,
src/**/*.{ts,tsx,js,jsx}: Use tabs for indentation, not spaces. Always match existing file indentation when editing.Suggested fix to align indentation
{/* Drawer handle at bottom - click to collapse */} - <button - onClick={(e) => { - e.stopPropagation(); - onToggleWorktreeExpanded(session.id); - }} - className="w-full flex items-center justify-center gap-1.5 py-0.5 text-[9px] font-medium hover:opacity-80 transition-opacity cursor-pointer" - style={sessionListStyles.worktreeCollapseButton} - title="Click to collapse worktrees" - > + <button + onClick={(e) => { + e.stopPropagation(); + onToggleWorktreeExpanded(session.id); + }} + className="w-full flex items-center justify-center gap-1.5 py-0.5 text-[9px] font-medium hover:opacity-80 transition-opacity cursor-pointer" + style={sessionListStyles.worktreeCollapseButton} + title="Click to collapse worktrees" + >🤖 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 1799 - 1813, Fix the inconsistent indentation in the button JSX inside SessionList component: re-indent the onClick handler body, title attribute, and surrounding attributes to match the file's existing tab-based indentation style (use tabs, not spaces) so all attributes and children lines align with the rest of the button element; specifically update the JSX for the button that calls onToggleWorktreeExpanded(session.id) and uses sessionListStyles.worktreeCollapseButton, GitBranch, ChevronUp, and worktreeChildren so the attribute lines and inner lines have uniform tab indentation consistent with the file.src/renderer/components/AICommandsPanel.tsx (1)
209-259:⚠️ Potential issue | 🟡 MinorAdd tabIndex/outline-none to interactive controls.
The updated buttons still omit the focus requirements in renderer components. Please applytabIndexandoutline-none(and replicate to other buttons in this panel).Suggested patch
<button + tabIndex={0} onClick={() => setVariablesExpanded(!variablesExpanded)} - className="w-full px-3 py-2 flex items-center justify-between hover:bg-white/5 transition-colors" + className="w-full px-3 py-2 flex items-center justify-between hover:bg-white/5 transition-colors outline-none" > @@ <button + tabIndex={0} onClick={() => setIsCreating(true)} - className="flex items-center gap-2 px-4 py-2 rounded text-sm font-medium transition-all" + className="flex items-center gap-2 px-4 py-2 rounded text-sm font-medium transition-all outline-none" >As per coding guidelines:
src/renderer/**/*.{tsx,jsx}: Add tabIndex={0} or tabIndex={-1} and outline-none class to ensure focus works correctly. Use ref={(el) => el?.focus()} for auto-focus in React components.🤖 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 209 - 259, The interactive controls (the toggle button that calls setVariablesExpanded and the "Add Command" button that calls setIsCreating) are missing keyboard focus attributes; add tabIndex={0} and the Tailwind class "outline-none" to these button elements (and other buttons in this panel, including the variable items if they should be interactive) so they are focusable, and where a control should auto-focus (e.g., when opening creation UI setIsCreating true) use a ref callback like ref={(el) => el?.focus()} on that element; ensure you update the JSX around the variables toggle (variablesExpanded, setVariablesExpanded), TEMPLATE_VARIABLES_GENERAL items, and the Add Command button to include these changes.src/renderer/components/UsageDashboard/ChartSkeletons.tsx (1)
47-77:⚠️ Potential issue | 🟡 MinorAdd focus attributes to all skeleton root containers.
These skeleton components render top-level containers missingtabIndex,outline-none, and autofocus refs required by the renderer component guidelines. Apply focus attributes to each of the following skeleton containers:
SummaryCardsSkeleton(lines 52-57)AgentComparisonChartSkeleton(lines 86-89)SourceDistributionChartSkeleton(lines 133-136)ActivityHeatmapSkeleton(lines 183-186)DurationTrendsChartSkeleton(lines 264-267)AutoRunStatsSkeleton(lines 332-336)DashboardSkeleton(line 370)🔧 Example update
- <div - className="grid gap-4" + <div + className="grid gap-4 outline-none" + tabIndex={-1} + ref={(el) => el?.focus()} style={{ gridTemplateColumns: `repeat(${columns}, minmax(0, 1fr))`, }} data-testid="summary-cards-skeleton"Per coding guidelines: "src/renderer/**/*.{tsx,jsx}: Add tabIndex={0} or tabIndex={-1} and outline-none class to ensure focus works correctly. Use ref={(el) => el?.focus()} for auto-focus in React components."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/UsageDashboard/ChartSkeletons.tsx` around lines 47 - 77, The root containers of several skeleton components (SummaryCardsSkeleton, AgentComparisonChartSkeleton, SourceDistributionChartSkeleton, ActivityHeatmapSkeleton, DurationTrendsChartSkeleton, AutoRunStatsSkeleton, DashboardSkeleton) lack focus attributes; update each root <div> to include a focusable/tabbable setup by adding tabIndex={0} or tabIndex={-1} as appropriate, the "outline-none" class, and a ref that auto-focuses (ref={el => el?.focus()}) per renderer guidelines so keyboard/focus handling works; modify the root container in the listed components (e.g., the outer div in SummaryCardsSkeleton) to include these three changes.src/renderer/components/ExecutionQueueIndicator.tsx (1)
137-146:⚠️ Potential issue | 🟡 MinorAdd explicit focus affordances to comply with renderer accessibility rules.
The indicator button is interactive but lacks the required explicit focus attributes/classes. Please add a tabIndex and outline-none class here.
🔧 Proposed fix
<button ref={containerRef} onClick={onClick} + tabIndex={0} className="w-full mb-2 px-3 py-2 rounded-lg border flex items-center gap-2 text-sm transition-all hover:opacity-90 outline-none" style={{ backgroundColor: theme.colors.bgActivity, borderColor: theme.colors.border, color: theme.colors.textMain, }} >As per coding guidelines, all renderer components must include tabIndex and outline-none to ensure focus works correctly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/ExecutionQueueIndicator.tsx` around lines 137 - 146, The ExecutionQueueIndicator button lacks explicit focus affordances; update the JSX for the button element (the one using ref={containerRef} and onClick={onClick} inside the ExecutionQueueIndicator component) to include tabIndex={0} and add the "outline-none" class to its className so keyboard focus is enabled while preserving existing styles and classes.src/main/stats/stats-db.ts (1)
175-182:⚠️ Potential issue | 🟡 MinorHandle unexpected fs errors explicitly in size/path checks.
getDatabaseSizeandpathExistscurrently treat any error as “missing,” which can mask permission/IO failures and skip maintenance. Consider only suppressing ENOENT and rethrowing others.🛠️ Suggested guard for ENOENT-only handling
- async getDatabaseSize(): Promise<number> { - try { - const stats = await StatsDB.fsPromises.stat(this.dbPath); - return stats.size; - } catch { - return 0; - } - } + async getDatabaseSize(): Promise<number> { + try { + const stats = await StatsDB.fsPromises.stat(this.dbPath); + return stats.size; + } catch (error) { + if ((error as NodeJS.ErrnoException).code === 'ENOENT') { + return 0; + } + throw error; + } + } @@ - private async pathExists(targetPath: string): Promise<boolean> { - try { - await StatsDB.fsPromises.access(targetPath, fs.constants.F_OK); - return true; - } catch { - return false; - } - } + private async pathExists(targetPath: string): Promise<boolean> { + try { + await StatsDB.fsPromises.access(targetPath, fs.constants.F_OK); + return true; + } catch (error) { + if ((error as NodeJS.ErrnoException).code === 'ENOENT') { + return false; + } + throw error; + } + }As per coding guidelines, handle only expected/recoverable errors explicitly with specific error codes and let unexpected errors bubble up to Sentry.
Also applies to: 832-838
🤖 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 175 - 182, The current getDatabaseSize (using StatsDB.fsPromises.stat on this.dbPath) and pathExists swallow all errors; change both to catch errors explicitly and only suppress ENOENT: in getDatabaseSize, after awaiting StatsDB.fsPromises.stat(this.dbPath) catch the error, if error.code === 'ENOENT' return 0, otherwise rethrow the error; similarly in pathExists (the function that checks this.dbPath via fsPromises.access/stat) catch and return false only for error.code === 'ENOENT' and rethrow other errors so unexpected permission/IO failures surface to Sentry.src/main/history-manager.ts (1)
209-330:⚠️ Potential issue | 🟠 MajorSerialize async read‑modify‑write operations to avoid lost history entries.
Concurrent IPC calls to
addEntry,deleteEntry,updateEntry,clearSession, andupdateSessionNameByClaudeSessionIdcan interleave on the same session, causing last-write-wins data loss. Implement per-session write serialization using a Promise queue (as suggested in the comment) to preserve operation ordering.🧵 Suggested per-session write queue (example with addEntry)
@@ export class HistoryManager { private historyDir: string; private legacyFilePath: string; private migrationMarkerPath: string; private configDir: string; private watcher: fs.FSWatcher | null = null; + private writeQueues = new Map<string, Promise<unknown>>(); @@ + private enqueueWrite<T>(sessionId: string, task: () => Promise<T>): Promise<T> { + const prev = this.writeQueues.get(sessionId) ?? Promise.resolve(); + const next = prev.then(task, task); + const cleanup = next.finally(() => { + if (this.writeQueues.get(sessionId) === cleanup) { + this.writeQueues.delete(sessionId); + } + }); + this.writeQueues.set(sessionId, cleanup); + return next; + } @@ - async addEntry(sessionId: string, projectPath: string, entry: HistoryEntry): Promise<void> { - const filePath = this.getSessionFilePath(sessionId); - let data: HistoryFileData; - - if (await this.fileExists(filePath)) { - try { - data = JSON.parse(await fsPromises.readFile(filePath, 'utf-8')); - } catch { - data = { version: HISTORY_VERSION, sessionId, projectPath, entries: [] }; - } - } else { - data = { version: HISTORY_VERSION, sessionId, projectPath, entries: [] }; - } - - // Add to beginning (most recent first) - data.entries.unshift(entry); - - // Trim to max entries - if (data.entries.length > MAX_ENTRIES_PER_SESSION) { - data.entries = data.entries.slice(0, MAX_ENTRIES_PER_SESSION); - } - - // Update projectPath if it changed - data.projectPath = projectPath; - - try { - await fsPromises.writeFile(filePath, JSON.stringify(data, null, 2), 'utf-8'); - logger.debug(`Added history entry for session ${sessionId}`, LOG_CONTEXT); - } catch (error) { - logger.error(`Failed to write history for session ${sessionId}: ${error}`, LOG_CONTEXT); - captureException(error, { operation: 'history:write', sessionId }); - } - } + async addEntry(sessionId: string, projectPath: string, entry: HistoryEntry): Promise<void> { + return this.enqueueWrite(sessionId, async () => { + const filePath = this.getSessionFilePath(sessionId); + let data: HistoryFileData; + + if (await this.fileExists(filePath)) { + try { + data = JSON.parse(await fsPromises.readFile(filePath, 'utf-8')); + } catch { + data = { version: HISTORY_VERSION, sessionId, projectPath, entries: [] }; + } + } else { + data = { version: HISTORY_VERSION, sessionId, projectPath, entries: [] }; + } + + // Add to beginning (most recent first) + data.entries.unshift(entry); + + // Trim to max entries + if (data.entries.length > MAX_ENTRIES_PER_SESSION) { + data.entries = data.entries.slice(0, MAX_ENTRIES_PER_SESSION); + } + + // Update projectPath if it changed + data.projectPath = projectPath; + + try { + await fsPromises.writeFile(filePath, JSON.stringify(data, null, 2), 'utf-8'); + logger.debug(`Added history entry for session ${sessionId}`, LOG_CONTEXT); + } catch (error) { + logger.error(`Failed to write history for session ${sessionId}: ${error}`, LOG_CONTEXT); + captureException(error, { operation: 'history:write', sessionId }); + } + }); + }Apply the same queuing pattern to
deleteEntry,updateEntry,clearSession, andupdateSessionNameByClaudeSessionId.🤖 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 209 - 330, Concurrent read-modify-write operations on the same session can interleave and lose entries; implement a per-session Promise queue to serialize operations by adding a Map<string, Promise<any>> (e.g., sessionQueues) and, in each mutating method (addEntry, deleteEntry, updateEntry, clearSession and updateSessionNameByClaudeSessionId), replace the direct body with an enqueued task: set sessionQueues[sessionId] = (sessionQueues[sessionId] || Promise.resolve()).then(() => { /* original logic */ }); ensure the enqueued callback returns the final Promise so callers await completion, catch/log errors inside the task, and finally clear the map entry when the chain settles to avoid memory leaks; keep the existing read/write/error handling but run it inside the queued task to preserve ordering.src/main/ipc/handlers/tabNaming.ts (1)
197-263:⚠️ Potential issue | 🟡 MinorClean up listeners/timeouts on spawn failure to avoid leaks.
Ifspawnrejects, the timeout and listeners remain registered and may fire after the Promise is already rejected.🧹 Proposed fix
- (async () => { - try { - await processManager.spawn({ - sessionId, - toolType: config.agentType, - cwd, - command, - args: finalArgs, - prompt: fullPrompt, - customEnvVars, - sendPromptViaStdin: shouldSendPromptViaStdin, - }); - } catch (error) { - reject(error); - } - })(); + (async () => { + try { + await processManager.spawn({ + sessionId, + toolType: config.agentType, + cwd, + command, + args: finalArgs, + prompt: fullPrompt, + customEnvVars, + sendPromptViaStdin: shouldSendPromptViaStdin, + }); + } catch (error) { + if (!resolved) { + resolved = true; + clearTimeout(timeoutId); + processManager.off('data', onData); + processManager.off('exit', onExit); + } + reject(error); + } + })();🤖 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 197 - 263, If processManager.spawn rejects, the timeout and listeners (onData/onExit) are left registered causing leaks; update the spawn error handler inside the async IIFE so it clears the timeout (timeoutId), removes listeners via processManager.off('data', onData) and processManager.off('exit', onExit), and ensures the promise is settled exactly once by checking/setting the resolved flag before rejecting (or resolving) with the error; reference processManager.spawn, onData, onExit, timeoutId, resolved and sessionId to locate and modify the handler.src/main/web-server/routes/apiRoutes.ts (1)
326-338:⚠️ Potential issue | 🟠 MajorCapture history failures via Sentry instead of swallowing the error.
The new async
getHistorycan reject; the current catch returns a 500 without Sentry reporting. Please capture (or rethrow) unexpected errors so they aren’t lost.Suggested fix
+import { captureException } from '../../utils/sentry'; ... } catch (error: any) { + captureException(error, { + tags: { context: 'WebServer:API /history' }, + extra: { projectPath, sessionId }, + }); return reply.code(500).send({ error: 'Internal Server Error', message: `Failed to fetch history: ${error.message}`, timestamp: Date.now(), }); }As per coding guidelines, “Do NOT silently swallow exceptions with try-catch-console.error blocks. Let unhandled exceptions bubble up to Sentry for error tracking. Use Sentry utilities (captureException, captureMessage) from 'src/utils/sentry' for explicit error reporting with context.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/web-server/routes/apiRoutes.ts` around lines 326 - 338, The catch block around this.callbacks.getHistory in the route handler currently returns a 500 without reporting the error; update the handler to call the Sentry utilities (import captureException and/or captureMessage from src/utils/sentry) when an unexpected error occurs in getHistory (or rethrow after reporting) so failures are reported to Sentry; specifically, in the catch for the getHistory call (references: getHistory, this.callbacks.getHistory) call captureException(error, { extra: { projectPath, sessionId } }) or captureMessage with context before sending the reply (or rethrow) to ensure the error is captured by Sentry.src/main/process-manager/utils/imageUtils.ts (2)
33-45:⚠️ Potential issue | 🟠 MajorReport write failures to Sentry instead of only logging.
The catch block logs and returns null, which can mask unexpected IO errors without Sentry visibility. Please capture exceptions (or rethrow unexpected ones) with context.
Suggested fix
+import { captureException } from '../../utils/sentry'; ... } catch (error) { + captureException(error, { + tags: { context: 'ProcessManager:saveImageToTempFile' }, + extra: { tempPath }, + }); logger.error('[ProcessManager] Failed to save image to temp file', 'ProcessManager', { error: String(error), }); return null; }As per coding guidelines, “Do NOT silently swallow exceptions with try-catch-console.error blocks. Let unhandled exceptions bubble up to Sentry for error tracking. Use Sentry utilities (captureException, captureMessage) from 'src/utils/sentry' for explicit error reporting with context.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/process-manager/utils/imageUtils.ts` around lines 33 - 45, The catch block around Buffer.from(parsed.base64) / fsPromises.writeFile currently only logs and returns null; change it to capture the exception with the Sentry helper and let it bubble instead of silently swallowing it: import captureException from 'src/utils/sentry', call captureException(error, { extra: { tempPath, parsedBase64Preview: parsed.base64?.slice(0,100), attemptedSize: buffer?.length } }) (or similar contextual fields) inside the catch, keep or augment the logger.error, and then rethrow the caught error (throw error) so it surfaces to calling code/Sentry rather than returning null. Ensure you reference parsed.base64, tempPath and the write operation (fsPromises.writeFile) when adding context.
22-36:⚠️ Potential issue | 🟠 MajorError handling must use Sentry for explicit error reporting.
The function correctly awaits at its call site (ChildProcessSpawner.ts:112), but the catch block (lines 41-46) silently swallows the exception using only
logger.error. Per coding guidelines, use Sentry utilities fromsrc/utils/sentryto explicitly report file write failures with context, then return null or rethrow as appropriate.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/process-manager/utils/imageUtils.ts` around lines 22 - 36, The catch block in saveImageToTempFile currently only logs the error; update it to import the Sentry utilities from src/utils/sentry and call the appropriate reporting function (e.g., captureException or reportError) inside the catch that surrounds fsPromises.writeFile, passing the caught exception plus context (function name saveImageToTempFile, index, tempPath, ext, and parsed.mediaType or a safe subset of dataUrl info) so failures are explicitly reported to Sentry, then return null (preserving current behavior) instead of silently swallowing the error.
🧹 Nitpick comments (7)
src/renderer/components/UsageDashboard/LongestAutoRunsTable.tsx (1)
229-235: Inline style objects recreated on each row render.The
onMouseEnter/onMouseLeavehandlers setbackgroundColordirectly on the element, which is fine for hover state. However, the initialstyleobject on lines 225-228 is recreated every render for every row. For a table capped at 25 rows this is minor, but if you want to squeeze out more perf, consider CSS:nth-child(even)or:hoverpseudo-classes instead of JS-driven hover and alternating styles.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/UsageDashboard/LongestAutoRunsTable.tsx` around lines 229 - 235, The per-row inline style and the JS hover handlers (onMouseEnter/onMouseLeave) in LongestAutoRunsTable recreate style objects on every render; replace the JS-driven alternating-row and hover styling with CSS rules instead (use a class on the row element and define :nth-child(even) for alternating background and :hover for hover state using theme colors or CSS variables derived from theme.colors), or if you must keep inline styles, memoize the base style per index (e.g., compute once using a stable helper) and remove the onMouseEnter/onMouseLeave backgroundColor mutations so the row style is static and not recreated on each render.src/renderer/components/UsageDashboard/EmptyState.tsx (1)
36-36: Consider extracting inline styles to constants.The inline style objects (
{ color: theme.colors.textDim },{ opacity: 0.3 }, etc.) create new references on each render, which can cause unnecessary child re-renders even when values haven't changed. For full memoization benefit, consider extracting static styles and usinguseMemofor theme-dependent ones.♻️ Optional: Extract inline styles
+import { memo, useMemo } from 'react'; -import { memo } from 'react'; import { BarChart3 } from 'lucide-react'; import type { Theme } from '../../types'; +const illustrationStyle = { opacity: 0.3 }; +const decorativeSvgStyle = { opacity: 0.5 }; + export const EmptyState = memo(function EmptyState({ theme, title = 'No usage data yet', message = 'Start using Maestro to see your stats!', }: EmptyStateProps) { + const textDimStyle = useMemo(() => ({ color: theme.colors.textDim }), [theme.colors.textDim]); + const textMainStyle = useMemo(() => ({ color: theme.colors.textMain }), [theme.colors.textMain]); + return ( <div className="h-full flex flex-col items-center justify-center gap-4" - style={{ color: theme.colors.textDim }} + style={textDimStyle} data-testid="usage-dashboard-empty" > {/* Subtle chart illustration */} - <div className="relative" style={{ opacity: 0.3 }}> + <div className="relative" style={illustrationStyle}>Also applies to: 40-40, 49-49, 59-59
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/UsageDashboard/EmptyState.tsx` at line 36, The inline style objects in the EmptyState component (e.g., style={{ color: theme.colors.textDim }}, style={{ opacity: 0.3 }}, and other similar inline styles) should be extracted so they don't get re-created on every render; move static styles to module-level constants and wrap theme-dependent style objects in a useMemo inside the EmptyState function (referencing theme) and replace the inline style props on the relevant JSX elements with those memoized constants (look for the style props in EmptyState.tsx around the color/opacity usages).src/renderer/components/QRCode.tsx (1)
57-61: Consider reporting error to Sentry instead of only logging to console.The error is caught and displayed to the user, which is good. However, per coding guidelines, consider using Sentry utilities from
src/utils/sentryfor error reporting with context, rather than only logging to console. This would help track QR generation failures in production.Suggested change
.catch((err) => { - console.error('Failed to generate QR code:', err); + // QR generation failure is recoverable - show error UI setError('Failed to generate QR code'); setDataUrl(null); });Or if tracking is desired:
import { captureException } from '@/utils/sentry'; // ... .catch((err) => { captureException(err, { extra: { value, size } }); setError('Failed to generate QR code'); setDataUrl(null); });As per coding guidelines: "Do NOT silently swallow exceptions with try-catch-console.error blocks" and "Use Sentry utilities (captureException, captureMessage) from 'src/utils/sentry' for explicit error reporting with context."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/QRCode.tsx` around lines 57 - 61, Replace the console.error-only catch in QRCode.tsx with a Sentry report: import captureException from src/utils/sentry and call captureException(err, { extra: { value, size } }) (or appropriate local variables used for QR generation) inside the .catch block, then keep the existing setError('Failed to generate QR code') and setDataUrl(null) so the UI behavior is unchanged; ensure the import and the use of captureException reference the same names used elsewhere (captureException) and include relevant contextual fields.src/renderer/components/UsageDashboard/AutoRunStats.tsx (1)
90-130: Consider memoizing MetricCard for additional gains.
MetricCardis rendered 6 times within this component. Since the parent is now memoized, wrappingMetricCardwithReact.memocould prevent unnecessary re-renders when only individual card props change.♻️ Optional: Memoize MetricCard
+const MetricCard = memo(function MetricCard({ icon, label, value, subValue, theme }: MetricCardProps) { -function MetricCard({ icon, label, value, subValue, theme }: MetricCardProps) { return ( <div className="p-4 rounded-lg flex items-start gap-3" // ... rest unchanged </div> ); -} +});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/UsageDashboard/AutoRunStats.tsx` around lines 90 - 130, MetricCard is a plain functional component rendered multiple times so wrap it with React.memo to avoid unnecessary re-renders; update the MetricCard export/definition to use React.memo(MetricCard) (or assign const MemoMetricCard = React.memo(MetricCard) and use that) and ensure any props used (icon, label, value, subValue, theme) are stable or provide a custom comparison function to React.memo if needed to control re-rendering.src/renderer/components/SessionList.tsx (1)
2002-2003: Misleading comment about effect dependencies.The comment on lines 2002-2003 states "Keep dependencies limited to sessionFilter and sessions" but the dependency array at line 2040 only includes
[sessionFilter]. Thesessionsvariable is used in the effect body (line 2008) but intentionally excluded from dependencies.The current behavior is likely intentional (only react to filter changes, using latest
sessionsfrom that render), but the comment should be updated to accurately reflect this design choice.Suggested comment fix
- // Note: Keep dependencies limited to sessionFilter and sessions to avoid unnecessary effect churn + // Note: Intentionally depends only on sessionFilter - sessions is accessed from the render closure + // when the filter changes, ensuring fresh session data without re-running on every session update useEffect(() => {Also applies to: 2040-2040
🤖 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 2002 - 2003, The existing comment above the useEffect in SessionList is misleading: it says "Keep dependencies limited to sessionFilter and sessions" while the dependency array only includes [sessionFilter] and sessions is intentionally omitted; update the comment to clearly state that the effect only runs when sessionFilter changes and that sessions is intentionally read from the current render (i.e., excluded from dependencies on purpose) so future readers won't assume sessions should be in the array; reference the useEffect block, the sessionFilter variable, and the sessions variable when making this clarification.src/renderer/components/AutoRun.tsx (1)
690-699: Minor:searchQueryis redundant insearchRegexdependencies.
escapedSearchQueryalready depends onsearchQuery, so including both in the dependency array is unnecessary. This doesn't cause bugs, just a small clarity nit.♻️ Optional cleanup
const searchRegex = useMemo(() => { if (!searchQuery.trim()) { return null; } return new RegExp(escapedSearchQuery, 'gi'); - }, [escapedSearchQuery, searchQuery]); + }, [escapedSearchQuery]);🤖 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 690 - 699, The useMemo for searchRegex redundantly lists searchQuery in its dependency array even though escapedSearchQuery already updates from searchQuery; update the useMemo for searchRegex (the function named searchRegex created with useMemo) to depend only on escapedSearchQuery (remove searchQuery from the dependency array) so the memo is correctly tied to the derived value.src/main/debug-package/collectors/group-chats.ts (1)
54-86: Consider parallelizing file processing for improved collection speed.The current implementation processes group chat files sequentially. For debug package collection with many files, parallel processing could speed this up:
♻️ Optional: Parallel file processing
- for (const file of files) { - if (!file.endsWith('.json') || file.endsWith('.log.json')) { - continue; - } - - const filePath = path.join(groupChatsPath, file); - - try { - const content = await fsPromises.readFile(filePath, 'utf-8'); - // ... rest of processing - } 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) => { + const filePath = path.join(groupChatsPath, file); + try { + const content = await fsPromises.readFile(filePath, 'utf-8'); + const chat = JSON.parse(content); + const logPath = path.join(groupChatsPath, `${path.basename(file, '.json')}.log.json`); + const messageCount = await countMessages(logPath); + return { + id: chat.id || path.basename(file, '.json'), + moderatorAgentId: chat.moderatorAgentId || chat.moderator?.agentId || 'unknown', + participantCount: Array.isArray(chat.participants) ? chat.participants.length : 0, + participants: Array.isArray(chat.participants) + ? chat.participants.map((p: any) => ({ agentId: p.agentId || 'unknown' })) + : [], + messageCount, + createdAt: chat.createdAt || 0, + updatedAt: chat.updatedAt || 0, + } as GroupChatInfo; + } catch { + return null; + } + }) + ); + + return results.filter((r): r is GroupChatInfo => r !== null);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/debug-package/collectors/group-chats.ts` around lines 54 - 86, The current for-loop in the group chat collector processes files sequentially (iterating over files, reading with fsPromises.readFile, parsing JSON, calling countMessages, and pushing into groupChats), which is slow for many files; refactor collect logic (the loop that builds chatInfo) to run in parallel by mapping files to async tasks and using Promise.all (or a bounded concurrency utility like p-limit) to run readFile/JSON.parse/countMessages concurrently, ensure each task catches/parses its own errors so a single failure doesn't reject the entire Promise.all, and finally aggregate successful chatInfo results into the groupChats array.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.ai-audit/annotations.jsonl:
- Line 11: The entry with session_id "1a945b20-4249-4534-a69b-91262e1d9afa" is a
start event without a corresponding end; add a matching JSONL end event object
for that session (type "session", event "end", same session_id and
environment_hash, a later timestamp and same assurance_level/description as
appropriate) so the start/end pair is complete, or remove the incomplete start
line if the session should not be recorded.
In `@src/main/history-manager.ts`:
- Around line 179-185: The fileExists method currently treats any
fsPromises.access error as "missing"; update fileExists to only return false
when the caught error has code 'ENOENT' and rethrow all other errors so
permission/IO errors surface to Sentry. Specifically, inside fileExists (which
calls fsPromises.access with fs.constants.F_OK), inspect the caught error's code
and return false for 'ENOENT' only—otherwise throw the error. Keep the async
signature and behavior unchanged for the ENOENT case.
In `@src/main/ipc/handlers/agentSessions.ts`:
- Around line 241-278: The catch blocks in the session discovery flow (around
fs.access, fs.readdir and inside processProjectDir) are swallowing all errors;
update each catch to only handle expected filesystem error codes (e.g., ENOENT,
ENOTDIR, EACCES) and rethrow any other errors so they bubble up (use the same
pattern for the other catches noted around lines 327-395). In places where you
intentionally handle a recoverable error, log/report it using the Sentry helpers
(captureException or captureMessage from src/utils/sentry) with context (include
claudeProjectsDir, projectDir or filePath and the operation like
fs.stat/fs.readdir), and for all other unknown error.code values rethrow the
error instead of returning empty results (affecting functions/processes
including processProjectDir, the initial fs.access block, and the per-file
fs.stat handling).
In `@src/main/parsers/codex-output-parser.ts`:
- Around line 246-250: The empty .catch on loadCodexConfigCached() swallows all
errors; update the catch in the promise chain that calls
loadCodexConfigCached().then(...).catch(...) to explicitly ignore only expected
"config not found" errors (check error.code === 'ENOENT' or error.message
contains 'not found' or similar), and for any other error call your logger (or
console.error) with context (e.g., "Failed to load Codex config" and the error)
before rethrowing or returning; reference the loadCodexConfigCached and
this.applyCodexConfig call sites to locate where to add the error discrimination
and logging.
In `@src/main/utils/context-groomer.ts`:
- Around line 320-342: The GroomingProcessManager.spawn signature is currently
synchronous but the code calls await processManager.spawn(...); update the
GroomingProcessManager interface method spawn to return Promise<{ pid: number;
success?: boolean } | null> (i.e., make it async) to match the usage in
context-groomer where processManager.spawn is awaited; then adjust any
implementing classes/types to return a Promise as well so implementations
conform to the new async signature (refer to the spawn method on
GroomingProcessManager and the usage processManager.spawn in the spawning
block).
In `@src/main/web-server/types.ts`:
- Around line 294-297: Update the test callback implementations to satisfy the
GetHistoryCallback signature which returns Promise<HistoryEntry[]>: locate uses
in CallbackRegistry.test.ts (the functions passed as history callbacks) and
change their returns from raw arrays (e.g., () => []) to promise-returning ones
(e.g., async () => [] or () => Promise.resolve([])); ensure any helper mocks
used by CallbackRegistry.getHistory() are updated similarly so the type matches
GetHistoryCallback and avoids returning non-Promise values.
In `@src/renderer/components/FilePreview.tsx`:
- Around line 673-680: The regex is built from the raw searchQuery while
hasSearchQuery uses trimmedSearchQuery, causing mismatches; update the useMemo
for searchRegex to construct the RegExp from trimmedSearchQuery (not
searchQuery) so the gating (hasSearchQuery) and pattern creation align—adjust
the dependency list to include trimmedSearchQuery and keep
escapeRegexCharacters, useMemo, trimmedSearchQuery, and hasSearchQuery as the
referenced symbols.
- Around line 289-297: The CODE_FENCE_BOUNDARY_REGEX and HEADING_CAPTURE_REGEX
don't accept the up-to-3 leading spaces allowed by CommonMark, so update
CODE_FENCE_BOUNDARY_REGEX (used in FilePreview) to allow optional 0–3 leading
spaces before the fence marker (e.g., prefix with a non-capturing group like (?:
{0,3})) and update HEADING_CAPTURE_REGEX to allow 0–3 leading spaces before the
ATX hashes (e.g., ^(?: {0,3})(#{1,6})\s+(.+)$) while preserving multiline
behavior; keep other regexes (MARKDOWN_OPEN_TASK_REGEX,
MARKDOWN_CLOSED_TASK_REGEX, etc.) as-is.
---
Outside diff comments:
In `@src/main/history-manager.ts`:
- Around line 209-330: Concurrent read-modify-write operations on the same
session can interleave and lose entries; implement a per-session Promise queue
to serialize operations by adding a Map<string, Promise<any>> (e.g.,
sessionQueues) and, in each mutating method (addEntry, deleteEntry, updateEntry,
clearSession and updateSessionNameByClaudeSessionId), replace the direct body
with an enqueued task: set sessionQueues[sessionId] = (sessionQueues[sessionId]
|| Promise.resolve()).then(() => { /* original logic */ }); ensure the enqueued
callback returns the final Promise so callers await completion, catch/log errors
inside the task, and finally clear the map entry when the chain settles to avoid
memory leaks; keep the existing read/write/error handling but run it inside the
queued task to preserve ordering.
In `@src/main/ipc/handlers/tabNaming.ts`:
- Around line 197-263: If processManager.spawn rejects, the timeout and
listeners (onData/onExit) are left registered causing leaks; update the spawn
error handler inside the async IIFE so it clears the timeout (timeoutId),
removes listeners via processManager.off('data', onData) and
processManager.off('exit', onExit), and ensures the promise is settled exactly
once by checking/setting the resolved flag before rejecting (or resolving) with
the error; reference processManager.spawn, onData, onExit, timeoutId, resolved
and sessionId to locate and modify the handler.
In `@src/main/process-manager/utils/imageUtils.ts`:
- Around line 33-45: The catch block around Buffer.from(parsed.base64) /
fsPromises.writeFile currently only logs and returns null; change it to capture
the exception with the Sentry helper and let it bubble instead of silently
swallowing it: import captureException from 'src/utils/sentry', call
captureException(error, { extra: { tempPath, parsedBase64Preview:
parsed.base64?.slice(0,100), attemptedSize: buffer?.length } }) (or similar
contextual fields) inside the catch, keep or augment the logger.error, and then
rethrow the caught error (throw error) so it surfaces to calling code/Sentry
rather than returning null. Ensure you reference parsed.base64, tempPath and the
write operation (fsPromises.writeFile) when adding context.
- Around line 22-36: The catch block in saveImageToTempFile currently only logs
the error; update it to import the Sentry utilities from src/utils/sentry and
call the appropriate reporting function (e.g., captureException or reportError)
inside the catch that surrounds fsPromises.writeFile, passing the caught
exception plus context (function name saveImageToTempFile, index, tempPath, ext,
and parsed.mediaType or a safe subset of dataUrl info) so failures are
explicitly reported to Sentry, then return null (preserving current behavior)
instead of silently swallowing the error.
In `@src/main/stats/stats-db.ts`:
- Around line 175-182: The current getDatabaseSize (using
StatsDB.fsPromises.stat on this.dbPath) and pathExists swallow all errors;
change both to catch errors explicitly and only suppress ENOENT: in
getDatabaseSize, after awaiting StatsDB.fsPromises.stat(this.dbPath) catch the
error, if error.code === 'ENOENT' return 0, otherwise rethrow the error;
similarly in pathExists (the function that checks this.dbPath via
fsPromises.access/stat) catch and return false only for error.code === 'ENOENT'
and rethrow other errors so unexpected permission/IO failures surface to Sentry.
In `@src/main/web-server/routes/apiRoutes.ts`:
- Around line 326-338: The catch block around this.callbacks.getHistory in the
route handler currently returns a 500 without reporting the error; update the
handler to call the Sentry utilities (import captureException and/or
captureMessage from src/utils/sentry) when an unexpected error occurs in
getHistory (or rethrow after reporting) so failures are reported to Sentry;
specifically, in the catch for the getHistory call (references: getHistory,
this.callbacks.getHistory) call captureException(error, { extra: { projectPath,
sessionId } }) or captureMessage with context before sending the reply (or
rethrow) to ensure the error is captured by Sentry.
In `@src/renderer/components/AICommandsPanel.tsx`:
- Around line 209-259: The interactive controls (the toggle button that calls
setVariablesExpanded and the "Add Command" button that calls setIsCreating) are
missing keyboard focus attributes; add tabIndex={0} and the Tailwind class
"outline-none" to these button elements (and other buttons in this panel,
including the variable items if they should be interactive) so they are
focusable, and where a control should auto-focus (e.g., when opening creation UI
setIsCreating true) use a ref callback like ref={(el) => el?.focus()} on that
element; ensure you update the JSX around the variables toggle
(variablesExpanded, setVariablesExpanded), TEMPLATE_VARIABLES_GENERAL items, and
the Add Command button to include these changes.
In `@src/renderer/components/ExecutionQueueIndicator.tsx`:
- Around line 137-146: The ExecutionQueueIndicator button lacks explicit focus
affordances; update the JSX for the button element (the one using
ref={containerRef} and onClick={onClick} inside the ExecutionQueueIndicator
component) to include tabIndex={0} and add the "outline-none" class to its
className so keyboard focus is enabled while preserving existing styles and
classes.
In `@src/renderer/components/SessionList.tsx`:
- Around line 1799-1813: Fix the inconsistent indentation in the button JSX
inside SessionList component: re-indent the onClick handler body, title
attribute, and surrounding attributes to match the file's existing tab-based
indentation style (use tabs, not spaces) so all attributes and children lines
align with the rest of the button element; specifically update the JSX for the
button that calls onToggleWorktreeExpanded(session.id) and uses
sessionListStyles.worktreeCollapseButton, GitBranch, ChevronUp, and
worktreeChildren so the attribute lines and inner lines have uniform tab
indentation consistent with the file.
In `@src/renderer/components/UsageDashboard/ChartSkeletons.tsx`:
- Around line 47-77: The root containers of several skeleton components
(SummaryCardsSkeleton, AgentComparisonChartSkeleton,
SourceDistributionChartSkeleton, ActivityHeatmapSkeleton,
DurationTrendsChartSkeleton, AutoRunStatsSkeleton, DashboardSkeleton) lack focus
attributes; update each root <div> to include a focusable/tabbable setup by
adding tabIndex={0} or tabIndex={-1} as appropriate, the "outline-none" class,
and a ref that auto-focuses (ref={el => el?.focus()}) per renderer guidelines so
keyboard/focus handling works; modify the root container in the listed
components (e.g., the outer div in SummaryCardsSkeleton) to include these three
changes.
In `@src/renderer/components/UsageDashboard/LongestAutoRunsTable.tsx`:
- Around line 128-138: The fetchData function currently swallows errors in its
catch by only calling console.error; replace that with error reporting to Sentry
or rethrowing so exceptions are not silently ignored. Update the catch in
fetchData (wrapping window.maestro.stats.getAutoRunSessions) to call the
project's Sentry utility (e.g., Sentry.captureException or the app's
captureError helper) with the caught err and a helpful message, then rethrow or
allow the error to bubble instead of solely logging; ensure setLoading(false)
still runs in finally and preserve existing setSessions behavior.
---
Nitpick comments:
In `@src/main/debug-package/collectors/group-chats.ts`:
- Around line 54-86: The current for-loop in the group chat collector processes
files sequentially (iterating over files, reading with fsPromises.readFile,
parsing JSON, calling countMessages, and pushing into groupChats), which is slow
for many files; refactor collect logic (the loop that builds chatInfo) to run in
parallel by mapping files to async tasks and using Promise.all (or a bounded
concurrency utility like p-limit) to run readFile/JSON.parse/countMessages
concurrently, ensure each task catches/parses its own errors so a single failure
doesn't reject the entire Promise.all, and finally aggregate successful chatInfo
results into the groupChats array.
In `@src/renderer/components/AutoRun.tsx`:
- Around line 690-699: The useMemo for searchRegex redundantly lists searchQuery
in its dependency array even though escapedSearchQuery already updates from
searchQuery; update the useMemo for searchRegex (the function named searchRegex
created with useMemo) to depend only on escapedSearchQuery (remove searchQuery
from the dependency array) so the memo is correctly tied to the derived value.
In `@src/renderer/components/QRCode.tsx`:
- Around line 57-61: Replace the console.error-only catch in QRCode.tsx with a
Sentry report: import captureException from src/utils/sentry and call
captureException(err, { extra: { value, size } }) (or appropriate local
variables used for QR generation) inside the .catch block, then keep the
existing setError('Failed to generate QR code') and setDataUrl(null) so the UI
behavior is unchanged; ensure the import and the use of captureException
reference the same names used elsewhere (captureException) and include relevant
contextual fields.
In `@src/renderer/components/SessionList.tsx`:
- Around line 2002-2003: The existing comment above the useEffect in SessionList
is misleading: it says "Keep dependencies limited to sessionFilter and sessions"
while the dependency array only includes [sessionFilter] and sessions is
intentionally omitted; update the comment to clearly state that the effect only
runs when sessionFilter changes and that sessions is intentionally read from the
current render (i.e., excluded from dependencies on purpose) so future readers
won't assume sessions should be in the array; reference the useEffect block, the
sessionFilter variable, and the sessions variable when making this
clarification.
In `@src/renderer/components/UsageDashboard/AutoRunStats.tsx`:
- Around line 90-130: MetricCard is a plain functional component rendered
multiple times so wrap it with React.memo to avoid unnecessary re-renders;
update the MetricCard export/definition to use React.memo(MetricCard) (or assign
const MemoMetricCard = React.memo(MetricCard) and use that) and ensure any props
used (icon, label, value, subValue, theme) are stable or provide a custom
comparison function to React.memo if needed to control re-rendering.
In `@src/renderer/components/UsageDashboard/EmptyState.tsx`:
- Line 36: The inline style objects in the EmptyState component (e.g., style={{
color: theme.colors.textDim }}, style={{ opacity: 0.3 }}, and other similar
inline styles) should be extracted so they don't get re-created on every render;
move static styles to module-level constants and wrap theme-dependent style
objects in a useMemo inside the EmptyState function (referencing theme) and
replace the inline style props on the relevant JSX elements with those memoized
constants (look for the style props in EmptyState.tsx around the color/opacity
usages).
In `@src/renderer/components/UsageDashboard/LongestAutoRunsTable.tsx`:
- Around line 229-235: The per-row inline style and the JS hover handlers
(onMouseEnter/onMouseLeave) in LongestAutoRunsTable recreate style objects on
every render; replace the JS-driven alternating-row and hover styling with CSS
rules instead (use a class on the row element and define :nth-child(even) for
alternating background and :hover for hover state using theme colors or CSS
variables derived from theme.colors), or if you must keep inline styles, memoize
the base style per index (e.g., compute once using a stable helper) and remove
the onMouseEnter/onMouseLeave backgroundColor mutations so the row style is
static and not recreated on each render.
Comment on lines
+289
to
+297
| const MARKDOWN_OPEN_TASK_REGEX = /^[\s]*[-*]\s*\[\s*\]/gm; | ||
| const MARKDOWN_CLOSED_TASK_REGEX = /^[\s]*[-*]\s*\[[xX]\]/gm; | ||
| const CODE_FENCE_BOUNDARY_REGEX = /^(`{3,}|~{3,})/; | ||
| const HEADING_CAPTURE_REGEX = /^(#{1,6})\s+(.+)$/; | ||
| const MARKDOWN_HIGHLIGHT_REGEX = /==([^=]+)==/g; | ||
| const INLINE_CODE_LANGUAGE_REGEX = /language-(\w+)/; | ||
| const REGEX_SPECIAL_CHARS = /[.*+?^${}()|[\]\\]/g; | ||
|
|
||
| const escapeRegexCharacters = (value: string): string => value.replace(REGEX_SPECIAL_CHARS, '\\$&'); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, verify the file exists and check the exact lines mentioned
head -n 300 src/renderer/components/FilePreview.tsx | tail -n 15Repository: RunMaestro/Maestro
Length of output: 614
🌐 Web query:
CommonMark specification indented code fences headings leading spaces
💡 Result:
In CommonMark, “leading spaces” matter because most block constructs allow up to 3 spaces of indentation, but 4 spaces generally switches interpretation to an indented code block.
- ATX headings (
#): the opening#may be preceded by up to 3 spaces; with 4 spaces, it’s not a heading (it becomes indented code / literal text). [1][2] - Setext headings:
- The content line must not have more than 3 leading spaces. [1]
- The underline (
---/===) may be preceded by up to 3 spaces; 4 spaces is too many. [1][2]
- Indented code blocks: any non-blank line with 4+ leading spaces is an indented code “chunk” (and the content is taken literally, not parsed as Markdown). [3]
- Fenced code blocks (``` / ~~~): the opening fence must be preceded by up to 3 spaces; 4 spaces is too many (the fence line is then just literal text inside an indented code block). Closing fences also only allow up to 3 spaces. [4][5]
Sources
- CommonMark Spec 0.30 – ATX + Setext heading indentation rules: (spec.commonmark.org)
- CommonMark Spec 0.30 – examples showing “four spaces is too many” for headings: (spec.commonmark.org)
- CommonMark Spec 0.30 – indented code block definition (4+ spaces): (spec.commonmark.org)
- CommonMark Spec 0.30 – fenced code blocks begin with fence indented up to 3 spaces: (spec.commonmark.org)
- CommonMark Spec 0.30 – examples showing 4-space-indented fences aren’t fences (and closing fence can’t be indented 4): (spec.commonmark.org)
Citations:
- 1: https://spec.commonmark.org/0.30/?utm_source=openai
- 2: https://spec.commonmark.org/0.30/
- 3: https://spec.commonmark.org/0.30/
- 4: https://spec.commonmark.org/0.30/
- 5: https://spec.commonmark.org/0.30/
🏁 Script executed:
# Find where CODE_FENCE_BOUNDARY_REGEX and HEADING_CAPTURE_REGEX are used
rg -n "CODE_FENCE_BOUNDARY_REGEX|HEADING_CAPTURE_REGEX" src/renderer/components/FilePreview.tsxRepository: RunMaestro/Maestro
Length of output: 277
🏁 Script executed:
# Get the function context where these regexes are used
sed -n '310,335p' src/renderer/components/FilePreview.tsxRepository: RunMaestro/Maestro
Length of output: 749
🏁 Script executed:
# Get more context - find the function signature and how 'lines' is created
sed -n '290,315p' src/renderer/components/FilePreview.tsx | head -n 30Repository: RunMaestro/Maestro
Length of output: 1029
🏁 Script executed:
# Check how extractHeadings is called - find callers of this function
rg -n "extractHeadings" src/renderer/components/FilePreview.tsx -A 2 -B 2Repository: RunMaestro/Maestro
Length of output: 461
🏁 Script executed:
# Check if there's any preprocessing of 'line' before regex testing
sed -n '307,335p' src/renderer/components/FilePreview.tsxRepository: RunMaestro/Maestro
Length of output: 878
Handle indented fences and headings in TOC parsing.
CommonMark allows up to 3 leading spaces before fenced code blocks and ATX headings. With the current regexes, lines with 1–3 leading spaces won't be detected, causing indented fences to go untracked and headings inside code fences to leak into the TOC. Update the regexes to allow optional leading whitespace.
🔧 Suggested regex fix
-const CODE_FENCE_BOUNDARY_REGEX = /^(`{3,}|~{3,})/;
-const HEADING_CAPTURE_REGEX = /^(#{1,6})\s+(.+)$/;
+const CODE_FENCE_BOUNDARY_REGEX = /^\s{0,3}(`{3,}|~{3,})/;
+const HEADING_CAPTURE_REGEX = /^\s{0,3}(#{1,6})\s+(.+)$/;🤖 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 289 - 297, The
CODE_FENCE_BOUNDARY_REGEX and HEADING_CAPTURE_REGEX don't accept the up-to-3
leading spaces allowed by CommonMark, so update CODE_FENCE_BOUNDARY_REGEX (used
in FilePreview) to allow optional 0–3 leading spaces before the fence marker
(e.g., prefix with a non-capturing group like (?: {0,3})) and update
HEADING_CAPTURE_REGEX to allow 0–3 leading spaces before the ATX hashes (e.g.,
^(?: {0,3})(#{1,6})\s+(.+)$) while preserving multiline behavior; keep other
regexes (MARKDOWN_OPEN_TASK_REGEX, MARKDOWN_CLOSED_TASK_REGEX, etc.) as-is.
Collaborator
PR Review — Performance Optimization Sweep@openasocket — solid, systematic work here. The async conversions are thorough (all IPC callers properly Merge Conflicts (4 files)This PR is currently in a
Action: Rebase onto Blocking Issues1. Remove
|
Contributor
Author
|
Whoopsies, ai-audit will get removed. Thats from an upcoming pr/feature |
pedramamini
added a commit
that referenced
this pull request
Feb 24, 2026
React.memo wraps for 29 components (7 core + 22 UsageDashboard charts) to prevent unnecessary re-renders on parent state changes. Add compound DB indexes (start_time, agent_type/project_path/source) for common query patterns in stats aggregation. Parallelize sequential git rev-parse calls in worktreeInfo and worktreeSetup handlers with Promise.all (~4x fewer round-trips). Cache Date.getTime() results in useFilteredAndSortedSessions sort comparator to avoid repeated date parsing during sort operations. Replace inline debounce in useStats with shared useDebouncedCallback hook, adding proper cleanup on unmount.
pedramamini
added a commit
that referenced
this pull request
Feb 25, 2026
* perf: remove streaming debug logs and fix index-based React keys
Remove 7 console.log calls from useSessionDebounce that fired at high
frequency during AI streaming. Replace index-based keys with stable
identifiers in 6 components where items can be removed or filtered
(staged images, diff tabs, log entries, quit modal agents).
* fix: address PR review feedback from CodeRabbit
- QuitConfirmModal: use composite key `${name}-${index}` to prevent
duplicate React keys when agents share display names
- extensionColors: early-return accent fallback in colorblind mode so
we never serve non-colorblind-safe colors from EXTENSION_MAP
* perf: prevent session bloat by stripping file tree and preview history from persistence
Users with large working directories (100K+ files) caused sessions.json to
balloon to 300MB+. Root cause: the full file tree (FileTreeNode[]) was persisted
for every session with no size limits.
Three changes:
1. Strip fileTree, fileTreeStats, filePreviewHistory from persistence — these
are ephemeral cache data that re-scan automatically on session activation
2. Add configurable local ignore patterns setting (default: .git, node_modules,
__pycache__) with UI in Settings > Display, reusing a new generic
IgnorePatternsSection component extracted from SshRemoteIgnoreSection
3. Wire local ignore patterns through loadFileTree for local (non-SSH) scans
* feat: honor local .gitignore patterns in file tree indexing
Add localHonorGitignore setting (default: true) that parses .gitignore
files and merges their patterns with the user's local ignore patterns
when building file trees. Mirrors the existing SSH remote gitignore
support for local filesystem scans.
* refactor: clean up file-scale PR with shared gitignore parser, options object, and rescan effect
- Extract parseGitignoreContent() shared between local/remote gitignore handling
- Replace 7 positional params in loadFileTree with LocalFileTreeOptions object
- Export DEFAULT_LOCAL_IGNORE_PATTERNS from settingsStore, use in SettingsModal
- Fix deprecated onKeyPress -> onKeyDown in IgnorePatternsSection
- Add role="checkbox" and aria-checked to honor-gitignore toggle
- Add useEffect to auto-rescan file tree when local ignore patterns change
- Update tests for new loadFileTree signature
* fix: enforce read-only env vars for OpenCode to prevent file writes in plan mode (#447)
OpenCode's defaultEnvVars always set OPENCODE_CONFIG_CONTENT with
"*":"allow" blanket permissions. In read-only mode, CLI YOLO flags were
stripped but the env var granting auto-approval was not. Add
readOnlyEnvOverrides to AgentConfig so agents can specify env var
overrides for read-only mode. OpenCode's override strips blanket
permissions while keeping question tool disabled.
* fix: use fixed icon sizes in FilePreview button panel for compact scrolled header
* fix: keep BatchRunner open when navigating to WorktreeConfig modal (#451)
WorktreeConfigModal now layers on top (priority 752 > 720) instead of
replacing the BatchRunner. User returns to their Auto Run setup after
configuring worktrees. Bumped WorktreeConfigModal z-index above
BatchRunner's z-[9999].
* perf: cherry-pick low-risk optimizations from PR #385
React.memo wraps for 29 components (7 core + 22 UsageDashboard charts)
to prevent unnecessary re-renders on parent state changes.
Add compound DB indexes (start_time, agent_type/project_path/source)
for common query patterns in stats aggregation.
Parallelize sequential git rev-parse calls in worktreeInfo and
worktreeSetup handlers with Promise.all (~4x fewer round-trips).
Cache Date.getTime() results in useFilteredAndSortedSessions sort
comparator to avoid repeated date parsing during sort operations.
Replace inline debounce in useStats with shared useDebouncedCallback
hook, adding proper cleanup on unmount.
* fix: make blocked Symphony issues selectable for document exploration
Blocked issues were completely non-interactive. Now they can be clicked
and keyboard-navigated to explore their documents, while the Start
Symphony button remains hidden to prevent starting blocked work.
* feat: add group @mentions in group chat and fix navigation history
Group @mentions: typing @ in group chat now shows agent groups in the
dropdown (visually differentiated with icon and member count badge).
Selecting a group expands to individual @mentions for all member agents.
Navigation history: group chats now participate in back/forward breadcrumb
navigation. Fixed pre-existing dual-instance bug where useNavigationHistory
was instantiated independently in App.tsx and useSessionLifecycle.ts,
creating separate stacks that couldn't communicate.
* feat: allow changing agent provider in Edit Agent modal
Replace the read-only provider display with a dropdown selector.
When provider changes, tabs reset to a fresh session and provider-
specific config (path, args, env, model) is cleared. History data
persists since it's keyed by Maestro session ID.
* feat: archive state for group chat (#377)
* feat: add archive state to group-chats
* fix: deduplicate GroupChat return type in global.d.ts, make archive callback optional
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
* fix: address PR #450 review feedback across stats, UI, and accessibility
Bug fixes: use tasksTotal for AutoRunStats count, add division-by-zero
guard in WeekdayComparisonChart, fix useStats effect cleanup, add v4
stats migration for compound indexes.
Sentry compliance: replace console.error with captureException in
QRCode, AutoRunStats, LongestAutoRunsTable, TasksByHourChart.
Accessibility: add keyboard handlers to AutoRunStats bar items, add
target guard and focus-visible ring to SymphonyModal.
Performance: memo MetricCard, useMemo for PeakHoursChart hasData,
useId for DurationTrendsChart gradient, stable opacities in
ChartSkeletons, stable onRemove ref in Toast, consistent variable
naming in git handler.
* fix: move blocked issue message to footer with greyed-out Start button
Removed inline blocked info box from the detail header. The footer now
shows for blocked issues with a disabled Start Symphony button and a
lock-icon message explaining the dependency block, consistent with the
normal footer layout.
* fix: prevent context menus from rendering off-screen
Replace ad-hoc hardcoded pixel buffers across 6 context menu
implementations with a shared useContextMenuPosition hook that
measures actual rendered menu size via getBoundingClientRect in
useLayoutEffect, clamping position to keep menus fully visible.
Two menus (SessionActivityGraph, GroupChatHistoryPanel) previously
had no viewport detection at all.
---------
Co-authored-by: Sam Shpuntoff <82393483+sshpuntoff@users.noreply.github.com>
Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
pedramamini
added a commit
that referenced
this pull request
Feb 25, 2026
* perf: remove streaming debug logs and fix index-based React keys
Remove 7 console.log calls from useSessionDebounce that fired at high
frequency during AI streaming. Replace index-based keys with stable
identifiers in 6 components where items can be removed or filtered
(staged images, diff tabs, log entries, quit modal agents).
* fix: address PR review feedback from CodeRabbit
- QuitConfirmModal: use composite key `${name}-${index}` to prevent
duplicate React keys when agents share display names
- extensionColors: early-return accent fallback in colorblind mode so
we never serve non-colorblind-safe colors from EXTENSION_MAP
* perf: prevent session bloat by stripping file tree and preview history from persistence
Users with large working directories (100K+ files) caused sessions.json to
balloon to 300MB+. Root cause: the full file tree (FileTreeNode[]) was persisted
for every session with no size limits.
Three changes:
1. Strip fileTree, fileTreeStats, filePreviewHistory from persistence — these
are ephemeral cache data that re-scan automatically on session activation
2. Add configurable local ignore patterns setting (default: .git, node_modules,
__pycache__) with UI in Settings > Display, reusing a new generic
IgnorePatternsSection component extracted from SshRemoteIgnoreSection
3. Wire local ignore patterns through loadFileTree for local (non-SSH) scans
* feat: honor local .gitignore patterns in file tree indexing
Add localHonorGitignore setting (default: true) that parses .gitignore
files and merges their patterns with the user's local ignore patterns
when building file trees. Mirrors the existing SSH remote gitignore
support for local filesystem scans.
* refactor: clean up file-scale PR with shared gitignore parser, options object, and rescan effect
- Extract parseGitignoreContent() shared between local/remote gitignore handling
- Replace 7 positional params in loadFileTree with LocalFileTreeOptions object
- Export DEFAULT_LOCAL_IGNORE_PATTERNS from settingsStore, use in SettingsModal
- Fix deprecated onKeyPress -> onKeyDown in IgnorePatternsSection
- Add role="checkbox" and aria-checked to honor-gitignore toggle
- Add useEffect to auto-rescan file tree when local ignore patterns change
- Update tests for new loadFileTree signature
* fix: enforce read-only env vars for OpenCode to prevent file writes in plan mode (#447)
OpenCode's defaultEnvVars always set OPENCODE_CONFIG_CONTENT with
"*":"allow" blanket permissions. In read-only mode, CLI YOLO flags were
stripped but the env var granting auto-approval was not. Add
readOnlyEnvOverrides to AgentConfig so agents can specify env var
overrides for read-only mode. OpenCode's override strips blanket
permissions while keeping question tool disabled.
* fix: use fixed icon sizes in FilePreview button panel for compact scrolled header
* fix: keep BatchRunner open when navigating to WorktreeConfig modal (#451)
WorktreeConfigModal now layers on top (priority 752 > 720) instead of
replacing the BatchRunner. User returns to their Auto Run setup after
configuring worktrees. Bumped WorktreeConfigModal z-index above
BatchRunner's z-[9999].
* perf: cherry-pick low-risk optimizations from PR #385
React.memo wraps for 29 components (7 core + 22 UsageDashboard charts)
to prevent unnecessary re-renders on parent state changes.
Add compound DB indexes (start_time, agent_type/project_path/source)
for common query patterns in stats aggregation.
Parallelize sequential git rev-parse calls in worktreeInfo and
worktreeSetup handlers with Promise.all (~4x fewer round-trips).
Cache Date.getTime() results in useFilteredAndSortedSessions sort
comparator to avoid repeated date parsing during sort operations.
Replace inline debounce in useStats with shared useDebouncedCallback
hook, adding proper cleanup on unmount.
* fix: make blocked Symphony issues selectable for document exploration
Blocked issues were completely non-interactive. Now they can be clicked
and keyboard-navigated to explore their documents, while the Start
Symphony button remains hidden to prevent starting blocked work.
* feat: add group @mentions in group chat and fix navigation history
Group @mentions: typing @ in group chat now shows agent groups in the
dropdown (visually differentiated with icon and member count badge).
Selecting a group expands to individual @mentions for all member agents.
Navigation history: group chats now participate in back/forward breadcrumb
navigation. Fixed pre-existing dual-instance bug where useNavigationHistory
was instantiated independently in App.tsx and useSessionLifecycle.ts,
creating separate stacks that couldn't communicate.
* feat: allow changing agent provider in Edit Agent modal
Replace the read-only provider display with a dropdown selector.
When provider changes, tabs reset to a fresh session and provider-
specific config (path, args, env, model) is cleared. History data
persists since it's keyed by Maestro session ID.
* feat: archive state for group chat (#377)
* feat: add archive state to group-chats
* fix: deduplicate GroupChat return type in global.d.ts, make archive callback optional
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
* fix: address PR #450 review feedback across stats, UI, and accessibility
Bug fixes: use tasksTotal for AutoRunStats count, add division-by-zero
guard in WeekdayComparisonChart, fix useStats effect cleanup, add v4
stats migration for compound indexes.
Sentry compliance: replace console.error with captureException in
QRCode, AutoRunStats, LongestAutoRunsTable, TasksByHourChart.
Accessibility: add keyboard handlers to AutoRunStats bar items, add
target guard and focus-visible ring to SymphonyModal.
Performance: memo MetricCard, useMemo for PeakHoursChart hasData,
useId for DurationTrendsChart gradient, stable opacities in
ChartSkeletons, stable onRemove ref in Toast, consistent variable
naming in git handler.
* fix: move blocked issue message to footer with greyed-out Start button
Removed inline blocked info box from the detail header. The footer now
shows for blocked issues with a disabled Start Symphony button and a
lock-icon message explaining the dependency block, consistent with the
normal footer layout.
* fix: prevent context menus from rendering off-screen
Replace ad-hoc hardcoded pixel buffers across 6 context menu
implementations with a shared useContextMenuPosition hook that
measures actual rendered menu size via getBoundingClientRect in
useLayoutEffect, clamping position to keep menus fully visible.
Two menus (SessionActivityGraph, GroupChatHistoryPanel) previously
had no viewport detection at all.
* fix: prevent orphaned file preview when tab bar is hidden
File preview rendering was independent of inputMode, so switching to
terminal mode (via authenticateAfterError or remote mode switch) without
clearing activeFileTabId left the file preview visible with no tab bar.
The corrupted state persisted across app restarts.
Guard file preview with inputMode='ai', clear activeFileTabId on all
terminal-mode transitions, and auto-heal on session restoration.
* fix: address PR review — missed onRemoteCommand path and weak test
- Clear activeFileTabId in onRemoteCommand mode-sync path (same bug
as onRemoteSwitchMode, caught by CodeRabbit)
- Fix "preserves activeFileTabId" test to use non-null starting value
so it actually tests preservation behavior
- Add test for onRemoteCommand clearing activeFileTabId on terminal sync
* polish: UI refinements for header z-index, status dot opacity, and Director's Notes beta badge
- Add z-20 to header container to prevent overlap issues
- Dim agent status dots for inactive sessions (0.45 opacity)
- Add beta badge to Director's Notes in Encore Features settings
* polish: reduce collapsed sidebar indicator opacity to 0.25 for less visual noise
---------
Co-authored-by: Sam Shpuntoff <82393483+sshpuntoff@users.noreply.github.com>
Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
pedramamini
added a commit
that referenced
this pull request
Feb 25, 2026
React.memo wraps for 29 components (7 core + 22 UsageDashboard charts) to prevent unnecessary re-renders on parent state changes. Add compound DB indexes (start_time, agent_type/project_path/source) for common query patterns in stats aggregation. Parallelize sequential git rev-parse calls in worktreeInfo and worktreeSetup handlers with Promise.all (~4x fewer round-trips). Cache Date.getTime() results in useFilteredAndSortedSessions sort comparator to avoid repeated date parsing during sort operations. Replace inline debounce in useStats with shared useDebouncedCallback hook, adding proper cleanup on unmount.
pedramamini
added a commit
that referenced
this pull request
Feb 25, 2026
* perf: remove streaming debug logs and fix index-based React keys
Remove 7 console.log calls from useSessionDebounce that fired at high
frequency during AI streaming. Replace index-based keys with stable
identifiers in 6 components where items can be removed or filtered
(staged images, diff tabs, log entries, quit modal agents).
* fix: address PR review feedback from CodeRabbit
- QuitConfirmModal: use composite key `${name}-${index}` to prevent
duplicate React keys when agents share display names
- extensionColors: early-return accent fallback in colorblind mode so
we never serve non-colorblind-safe colors from EXTENSION_MAP
* perf: prevent session bloat by stripping file tree and preview history from persistence
Users with large working directories (100K+ files) caused sessions.json to
balloon to 300MB+. Root cause: the full file tree (FileTreeNode[]) was persisted
for every session with no size limits.
Three changes:
1. Strip fileTree, fileTreeStats, filePreviewHistory from persistence — these
are ephemeral cache data that re-scan automatically on session activation
2. Add configurable local ignore patterns setting (default: .git, node_modules,
__pycache__) with UI in Settings > Display, reusing a new generic
IgnorePatternsSection component extracted from SshRemoteIgnoreSection
3. Wire local ignore patterns through loadFileTree for local (non-SSH) scans
* feat: honor local .gitignore patterns in file tree indexing
Add localHonorGitignore setting (default: true) that parses .gitignore
files and merges their patterns with the user's local ignore patterns
when building file trees. Mirrors the existing SSH remote gitignore
support for local filesystem scans.
* refactor: clean up file-scale PR with shared gitignore parser, options object, and rescan effect
- Extract parseGitignoreContent() shared between local/remote gitignore handling
- Replace 7 positional params in loadFileTree with LocalFileTreeOptions object
- Export DEFAULT_LOCAL_IGNORE_PATTERNS from settingsStore, use in SettingsModal
- Fix deprecated onKeyPress -> onKeyDown in IgnorePatternsSection
- Add role="checkbox" and aria-checked to honor-gitignore toggle
- Add useEffect to auto-rescan file tree when local ignore patterns change
- Update tests for new loadFileTree signature
* fix: enforce read-only env vars for OpenCode to prevent file writes in plan mode (#447)
OpenCode's defaultEnvVars always set OPENCODE_CONFIG_CONTENT with
"*":"allow" blanket permissions. In read-only mode, CLI YOLO flags were
stripped but the env var granting auto-approval was not. Add
readOnlyEnvOverrides to AgentConfig so agents can specify env var
overrides for read-only mode. OpenCode's override strips blanket
permissions while keeping question tool disabled.
* perf: cherry-pick low-risk optimizations from PR #385
React.memo wraps for 29 components (7 core + 22 UsageDashboard charts)
to prevent unnecessary re-renders on parent state changes.
Add compound DB indexes (start_time, agent_type/project_path/source)
for common query patterns in stats aggregation.
Parallelize sequential git rev-parse calls in worktreeInfo and
worktreeSetup handlers with Promise.all (~4x fewer round-trips).
Cache Date.getTime() results in useFilteredAndSortedSessions sort
comparator to avoid repeated date parsing during sort operations.
Replace inline debounce in useStats with shared useDebouncedCallback
hook, adding proper cleanup on unmount.
* fix: make blocked Symphony issues selectable for document exploration
Blocked issues were completely non-interactive. Now they can be clicked
and keyboard-navigated to explore their documents, while the Start
Symphony button remains hidden to prevent starting blocked work.
* feat: add group @mentions in group chat and fix navigation history
Group @mentions: typing @ in group chat now shows agent groups in the
dropdown (visually differentiated with icon and member count badge).
Selecting a group expands to individual @mentions for all member agents.
Navigation history: group chats now participate in back/forward breadcrumb
navigation. Fixed pre-existing dual-instance bug where useNavigationHistory
was instantiated independently in App.tsx and useSessionLifecycle.ts,
creating separate stacks that couldn't communicate.
* feat: allow changing agent provider in Edit Agent modal
Replace the read-only provider display with a dropdown selector.
When provider changes, tabs reset to a fresh session and provider-
specific config (path, args, env, model) is cleared. History data
persists since it's keyed by Maestro session ID.
* feat: archive state for group chat (#377)
* feat: add archive state to group-chats
* fix: deduplicate GroupChat return type in global.d.ts, make archive callback optional
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
* fix: address PR #450 review feedback across stats, UI, and accessibility
Bug fixes: use tasksTotal for AutoRunStats count, add division-by-zero
guard in WeekdayComparisonChart, fix useStats effect cleanup, add v4
stats migration for compound indexes.
Sentry compliance: replace console.error with captureException in
QRCode, AutoRunStats, LongestAutoRunsTable, TasksByHourChart.
Accessibility: add keyboard handlers to AutoRunStats bar items, add
target guard and focus-visible ring to SymphonyModal.
Performance: memo MetricCard, useMemo for PeakHoursChart hasData,
useId for DurationTrendsChart gradient, stable opacities in
ChartSkeletons, stable onRemove ref in Toast, consistent variable
naming in git handler.
* fix: move blocked issue message to footer with greyed-out Start button
Removed inline blocked info box from the detail header. The footer now
shows for blocked issues with a disabled Start Symphony button and a
lock-icon message explaining the dependency block, consistent with the
normal footer layout.
* fix: prevent context menus from rendering off-screen
Replace ad-hoc hardcoded pixel buffers across 6 context menu
implementations with a shared useContextMenuPosition hook that
measures actual rendered menu size via getBoundingClientRect in
useLayoutEffect, clamping position to keep menus fully visible.
Two menus (SessionActivityGraph, GroupChatHistoryPanel) previously
had no viewport detection at all.
* fix: prevent orphaned file preview when tab bar is hidden
File preview rendering was independent of inputMode, so switching to
terminal mode (via authenticateAfterError or remote mode switch) without
clearing activeFileTabId left the file preview visible with no tab bar.
The corrupted state persisted across app restarts.
Guard file preview with inputMode='ai', clear activeFileTabId on all
terminal-mode transitions, and auto-heal on session restoration.
* fix: address PR review — missed onRemoteCommand path and weak test
- Clear activeFileTabId in onRemoteCommand mode-sync path (same bug
as onRemoteSwitchMode, caught by CodeRabbit)
- Fix "preserves activeFileTabId" test to use non-null starting value
so it actually tests preservation behavior
- Add test for onRemoteCommand clearing activeFileTabId on terminal sync
* polish: UI refinements for header z-index, status dot opacity, and Director's Notes beta badge
- Add z-20 to header container to prevent overlap issues
- Dim agent status dots for inactive sessions (0.45 opacity)
- Add beta badge to Director's Notes in Encore Features settings
* polish: reduce collapsed sidebar indicator opacity to 0.25 for less visual noise
* polish: Director's Notes keeps old content visible during regeneration
Cache no longer keys on lookbackDays — once generated, notes persist
until explicit Regenerate click. During regeneration, old notes stay
visible and scrollable with stats bar, timestamp, and Save/Copy buttons
remaining enabled. Progress bar only shows on first-ever generation.
Renamed "Refresh" to "Regenerate" for clearer intent.
* fix: update prop extraction hooks for rebased branch compatibility
Add ~37 missing props to useMainPanelProps interface, useMemo return,
and dependency array. Remove state props from useRightPanelProps call
in App.tsx that the refactored hook no longer accepts (RightPanel now
reads state directly from Zustand stores).
---------
Co-authored-by: Sam Shpuntoff <82393483+sshpuntoff@users.noreply.github.com>
Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
pedramamini
added a commit
that referenced
this pull request
Feb 26, 2026
React.memo wraps for 29 components (7 core + 22 UsageDashboard charts) to prevent unnecessary re-renders on parent state changes. Add compound DB indexes (start_time, agent_type/project_path/source) for common query patterns in stats aggregation. Parallelize sequential git rev-parse calls in worktreeInfo and worktreeSetup handlers with Promise.all (~4x fewer round-trips). Cache Date.getTime() results in useFilteredAndSortedSessions sort comparator to avoid repeated date parsing during sort operations. Replace inline debounce in useStats with shared useDebouncedCallback hook, adding proper cleanup on unmount.
pedramamini
added a commit
that referenced
this pull request
Feb 27, 2026
React.memo wraps for 29 components (7 core + 22 UsageDashboard charts) to prevent unnecessary re-renders on parent state changes. Add compound DB indexes (start_time, agent_type/project_path/source) for common query patterns in stats aggregation. Parallelize sequential git rev-parse calls in worktreeInfo and worktreeSetup handlers with Promise.all (~4x fewer round-trips). Cache Date.getTime() results in useFilteredAndSortedSessions sort comparator to avoid repeated date parsing during sort operations. Replace inline debounce in useStats with shared useDebouncedCallback hook, adding proper cleanup on unmount.
pedramamini
added a commit
that referenced
this pull request
Feb 28, 2026
React.memo wraps for 29 components (7 core + 22 UsageDashboard charts) to prevent unnecessary re-renders on parent state changes. Add compound DB indexes (start_time, agent_type/project_path/source) for common query patterns in stats aggregation. Parallelize sequential git rev-parse calls in worktreeInfo and worktreeSetup handlers with Promise.all (~4x fewer round-trips). Cache Date.getTime() results in useFilteredAndSortedSessions sort comparator to avoid repeated date parsing during sort operations. Replace inline debounce in useStats with shared useDebouncedCallback hook, adding proper cleanup on unmount.
pedramamini
added a commit
that referenced
this pull request
Mar 1, 2026
* perf: remove streaming debug logs and fix index-based React keys
Remove 7 console.log calls from useSessionDebounce that fired at high
frequency during AI streaming. Replace index-based keys with stable
identifiers in 6 components where items can be removed or filtered
(staged images, diff tabs, log entries, quit modal agents).
* fix: address PR review feedback from CodeRabbit
- QuitConfirmModal: use composite key `${name}-${index}` to prevent
duplicate React keys when agents share display names
- extensionColors: early-return accent fallback in colorblind mode so
we never serve non-colorblind-safe colors from EXTENSION_MAP
* perf: prevent session bloat by stripping file tree and preview history from persistence
Users with large working directories (100K+ files) caused sessions.json to
balloon to 300MB+. Root cause: the full file tree (FileTreeNode[]) was persisted
for every session with no size limits.
Three changes:
1. Strip fileTree, fileTreeStats, filePreviewHistory from persistence — these
are ephemeral cache data that re-scan automatically on session activation
2. Add configurable local ignore patterns setting (default: .git, node_modules,
__pycache__) with UI in Settings > Display, reusing a new generic
IgnorePatternsSection component extracted from SshRemoteIgnoreSection
3. Wire local ignore patterns through loadFileTree for local (non-SSH) scans
* feat: honor local .gitignore patterns in file tree indexing
Add localHonorGitignore setting (default: true) that parses .gitignore
files and merges their patterns with the user's local ignore patterns
when building file trees. Mirrors the existing SSH remote gitignore
support for local filesystem scans.
* refactor: clean up file-scale PR with shared gitignore parser, options object, and rescan effect
- Extract parseGitignoreContent() shared between local/remote gitignore handling
- Replace 7 positional params in loadFileTree with LocalFileTreeOptions object
- Export DEFAULT_LOCAL_IGNORE_PATTERNS from settingsStore, use in SettingsModal
- Fix deprecated onKeyPress -> onKeyDown in IgnorePatternsSection
- Add role="checkbox" and aria-checked to honor-gitignore toggle
- Add useEffect to auto-rescan file tree when local ignore patterns change
- Update tests for new loadFileTree signature
* perf: cherry-pick low-risk optimizations from PR #385
React.memo wraps for 29 components (7 core + 22 UsageDashboard charts)
to prevent unnecessary re-renders on parent state changes.
Add compound DB indexes (start_time, agent_type/project_path/source)
for common query patterns in stats aggregation.
Parallelize sequential git rev-parse calls in worktreeInfo and
worktreeSetup handlers with Promise.all (~4x fewer round-trips).
Cache Date.getTime() results in useFilteredAndSortedSessions sort
comparator to avoid repeated date parsing during sort operations.
Replace inline debounce in useStats with shared useDebouncedCallback
hook, adding proper cleanup on unmount.
* fix: make blocked Symphony issues selectable for document exploration
Blocked issues were completely non-interactive. Now they can be clicked
and keyboard-navigated to explore their documents, while the Start
Symphony button remains hidden to prevent starting blocked work.
* feat: add group @mentions in group chat and fix navigation history
Group @mentions: typing @ in group chat now shows agent groups in the
dropdown (visually differentiated with icon and member count badge).
Selecting a group expands to individual @mentions for all member agents.
Navigation history: group chats now participate in back/forward breadcrumb
navigation. Fixed pre-existing dual-instance bug where useNavigationHistory
was instantiated independently in App.tsx and useSessionLifecycle.ts,
creating separate stacks that couldn't communicate.
* feat: allow changing agent provider in Edit Agent modal
Replace the read-only provider display with a dropdown selector.
When provider changes, tabs reset to a fresh session and provider-
specific config (path, args, env, model) is cleared. History data
persists since it's keyed by Maestro session ID.
* feat: archive state for group chat (#377)
* feat: add archive state to group-chats
* fix: deduplicate GroupChat return type in global.d.ts, make archive callback optional
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
* fix: address PR #450 review feedback across stats, UI, and accessibility
Bug fixes: use tasksTotal for AutoRunStats count, add division-by-zero
guard in WeekdayComparisonChart, fix useStats effect cleanup, add v4
stats migration for compound indexes.
Sentry compliance: replace console.error with captureException in
QRCode, AutoRunStats, LongestAutoRunsTable, TasksByHourChart.
Accessibility: add keyboard handlers to AutoRunStats bar items, add
target guard and focus-visible ring to SymphonyModal.
Performance: memo MetricCard, useMemo for PeakHoursChart hasData,
useId for DurationTrendsChart gradient, stable opacities in
ChartSkeletons, stable onRemove ref in Toast, consistent variable
naming in git handler.
* fix: move blocked issue message to footer with greyed-out Start button
Removed inline blocked info box from the detail header. The footer now
shows for blocked issues with a disabled Start Symphony button and a
lock-icon message explaining the dependency block, consistent with the
normal footer layout.
* fix: prevent context menus from rendering off-screen
Replace ad-hoc hardcoded pixel buffers across 6 context menu
implementations with a shared useContextMenuPosition hook that
measures actual rendered menu size via getBoundingClientRect in
useLayoutEffect, clamping position to keep menus fully visible.
Two menus (SessionActivityGraph, GroupChatHistoryPanel) previously
had no viewport detection at all.
* fix: prevent orphaned file preview when tab bar is hidden
File preview rendering was independent of inputMode, so switching to
terminal mode (via authenticateAfterError or remote mode switch) without
clearing activeFileTabId left the file preview visible with no tab bar.
The corrupted state persisted across app restarts.
Guard file preview with inputMode='ai', clear activeFileTabId on all
terminal-mode transitions, and auto-heal on session restoration.
* fix: address PR review — missed onRemoteCommand path and weak test
- Clear activeFileTabId in onRemoteCommand mode-sync path (same bug
as onRemoteSwitchMode, caught by CodeRabbit)
- Fix "preserves activeFileTabId" test to use non-null starting value
so it actually tests preservation behavior
- Add test for onRemoteCommand clearing activeFileTabId on terminal sync
* polish: UI refinements for header z-index, status dot opacity, and Director's Notes beta badge
- Add z-20 to header container to prevent overlap issues
- Dim agent status dots for inactive sessions (0.45 opacity)
- Add beta badge to Director's Notes in Encore Features settings
* polish: Director's Notes keeps old content visible during regeneration
Cache no longer keys on lookbackDays — once generated, notes persist
until explicit Regenerate click. During regeneration, old notes stay
visible and scrollable with stats bar, timestamp, and Save/Copy buttons
remaining enabled. Progress bar only shows on first-ever generation.
Renamed "Refresh" to "Regenerate" for clearer intent.
* fix: update prop extraction hooks for rebased branch compatibility
Add ~37 missing props to useMainPanelProps interface, useMemo return,
and dependency array. Remove state props from useRightPanelProps call
in App.tsx that the refactored hook no longer accepts (RightPanel now
reads state directly from Zustand stores).
* feat: add layout algorithm options to Document Graph (Mind Map, Radial, Force-Directed)
Adds a dropdown in the Document Graph header to switch between three
layout algorithms. Layout choice persists per-agent (via Session) with
global default fallback (via settingsStore).
- Extract layout code from MindMap.tsx into mindMapLayouts.ts
- Add Radial layout (concentric rings) and Force-Directed layout (d3-force)
- Add layout type dropdown UI with layer stack Escape handling
- Wire persistence through App.tsx (session + global default)
- Add tests for all three algorithms, settingsStore, and props
* polish: add "Load full file" button to truncated file preview banner
* fix: worktree auto-run spin-out creates duplicate agents and fails to start
Two bugs in the worktree "Run in Worktree" flow:
1. Duplicate agents: spawnWorktreeAgentAndDispatch didn't mark the
worktree path as recently created, so the file watcher in
useWorktreeHandlers raced and created a second session. Fixed by
introducing a shared worktreeDedup module checked by both hooks.
2. Auto-run never starts: startBatchRun uses sessionsRef which only
updates on React re-render, but is called synchronously after
Zustand store mutation. Added fallback to useSessionStore.getState()
for immediate access to just-created sessions.
* fix: lint cleanup, auto-scroll empty-tab guard, and worktree batch improvements
- Fix ESLint errors: empty catch block comment, unused eslint-disable directive,
unused variable prefixing in App.tsx and DirectorNotesModal
- Hide auto-scroll button when tab has no content (filteredLogs.length > 0 guard)
- Fix worktree auto-run: skip setupWorktree when worktreeTarget already created
the worktree, preventing "belongs to different repository" false positive
- Add "Auto Run Started" toast notification with task/document counts
- Record PR creation results in agent history panel
- Update auto-scroll and batch processor tests for new behavior
* feat: add "Open in Finder/Explorer" button to Storage Location settings
Opens the active storage folder in the native file manager. Adds
getOpenInLabel() to platformUtils for platform-aware button text.
* fix: prefix unused destructured vars to satisfy ESLint after rebase
After rebasing on main, three variables (terminalWidth, setTerminalWidth,
disableConfetti) from useSettings were destructured but not yet consumed
in App.tsx, triggering no-unused-vars warnings.
* feat: add "View history" link to Auto Run batch progress footer
Helps users discover the History tab for viewing completed Auto Run
work entries. Link appears only when the Auto Run tab is active.
* feat: show issue counts on Symphony tiles via GitHub Search API
Uses a single GitHub Search API call to batch-fetch open issue counts
for all registered repos (instead of 2 calls per repo). Tiles now show
"View #N Issues" when issues exist, "No Issues" (ghosted at 45% opacity)
when none, and the original "View Issues" while counts are loading.
- New IPC handler: symphony:getIssueCounts (30-min cache TTL)
- New fetchIssueCounts() using /search/issues with multiple repo: qualifiers
- Counts fetched automatically when registry loads
- Updated RepositoryTile, useSymphony hook, preload bridge, global.d.ts
* fix: show task summary on TodoWrite tool cards in AI terminal
TodoWrite tool cards rendered as bare "TodoWrite ●" with no detail
because the todos array input wasn't handled in the tool detail
extraction chain. Added summarizeTodos helper that shows the
in-progress task's activeForm with completion progress (e.g.,
"Running tests (2/5)").
* fix: remove close button from group chat header, fix agent switching
The X button in the group chat header was unnecessary since users can
switch away by clicking another agent or group chat. SessionList was
bypassing App.tsx's setActiveSessionId wrapper (which dismisses group
chat) by reading directly from the session store, so clicking an agent
in the sidebar didn't exit the group chat view. Now SessionList wraps
the call to also clear activeGroupChatId.
* fix: skip archived group chats during keyboard navigation cycling
Archived group chats were included in the visual order built by
useCycleSession, causing keyboard shortcuts (Cmd+]/Cmd+[) to land
on hidden archived items. Now filters them out before building the
navigation list.
* fix: file explorer dedup, worktree timer race, ENOENT handling, type fixes
- Add dual-layer deduplication to file explorer (readDir + flatten) to
prevent duplicate entries and React key collisions
- Fix worktree dedup timer race condition by clearing old timers on
re-mark and on explicit clear
- Return null instead of throwing for ENOENT in fs:readFile IPC handler
to avoid noisy Electron error logging for expected missing files
- Add isFile field to DirectoryEntry in preload and renderer types to
match global.d.ts and runtime data
- Extract DocumentGraphLayoutType alias to reduce inline union repetition
- Add targetBranch to PRCreationResult so batch history logs show the
resolved target branch instead of a hardcoded fallback
- Strengthen tests: mind map side assertion, FilePreview re-query after
state change, settings fallback persistence, new dedup and timer tests
Claude ID: 174e0697-1313-4c5b-af72-64595f238cd9
Maestro ID: 21845e77-8847-4578-a785-1cd3c3556b6c
---------
Co-authored-by: Sam Shpuntoff <82393483+sshpuntoff@users.noreply.github.com>
Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
Codex session metadata (audit.db, annotations, config) are runtime artifacts, not source code. Remove tracked files and prevent future accidental commits. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The interface declared spawn() with a sync return type but the implementation (ProcessManager.spawn) is async and callers use await. Align the interface with the actual async contract. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Previously both methods used bare catch blocks that silently swallowed all errors including permission and IO errors. Now only ENOENT (file not found) is suppressed; other errors bubble up to Sentry. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Removed extra level of indentation from the Promise.all block inside the pathExists check in the worktree handler (~line 611). Lines were at 6-tab depth when they should have been at 5-tab depth to match surrounding code. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ed FS errors
Replace silent catch {} blocks in discoverClaudeSessionFiles() and
discoverCodexSessionFiles() with filtered error handling that only
suppresses expected filesystem errors (ENOENT, ENOTDIR, EACCES).
Unexpected errors (EIO, EPERM, etc.) now bubble up to Sentry.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/stats/stats-db.ts (1)
281-289:⚠️ Potential issue | 🟠 MajorDo not update
last_vacuum_atwhen VACUUM fails.At Line 283,
result.vacuumedis true when threshold is exceeded, even if the VACUUM operation itself failed. Updating the timestamp in that case can suppress retries for a week.Suggested fix
- if (result.vacuumed) { + if (result.vacuumed && result.result?.success) { // Update timestamp in _meta table this.database .prepare("INSERT OR REPLACE INTO _meta (key, value) VALUES ('last_vacuum_at', ?)") .run(String(now)); logger.info('Updated VACUUM timestamp in _meta table', LOG_CONTEXT); }🤖 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 281 - 289, The code currently updates the _meta last_vacuum_at whenever result.vacuumed is true even if the VACUUM operation failed; change the logic so the timestamp is only written after a successful VACUUM. Update vacuumIfNeeded() to return an explicit success flag (e.g., { vacuumed: boolean, success?: boolean, error?: Error }) or ensure vacuumed is only true on success, then here check that success (or absence of error) in addition to result.vacuumed before calling the INSERT OR REPLACE into _meta; reference vacuumIfNeeded(), result.vacuumed, and the _meta timestamp update to locate and fix the behavior.src/main/history-manager.ts (1)
210-237:⚠️ Potential issue | 🔴 CriticalConcurrent async mutations can lose history entries.
After the async migration,
addEntryis now a read-modify-write sequence without per-session serialization. Parallel calls can overwrite each other (lost updates). The same risk applies todeleteEntry/updateEntry/clearSession.🛠️ Suggested fix: per-session write queue (serialize mutating operations)
export class HistoryManager { + private sessionWriteChains = new Map<string, Promise<unknown>>(); + + private enqueueSessionWrite<T>(sessionId: string, work: () => Promise<T>): Promise<T> { + const previous = this.sessionWriteChains.get(sessionId) ?? Promise.resolve(); + const next = previous.then(work, work); + this.sessionWriteChains.set( + sessionId, + next.finally(() => { + if (this.sessionWriteChains.get(sessionId) === next) { + this.sessionWriteChains.delete(sessionId); + } + }) + ); + return next; + }- async addEntry(sessionId: string, projectPath: string, entry: HistoryEntry): Promise<void> { - const filePath = this.getSessionFilePath(sessionId); - let data: HistoryFileData; - ... - await fsPromises.writeFile(filePath, JSON.stringify(data, null, 2), 'utf-8'); - } + async addEntry(sessionId: string, projectPath: string, entry: HistoryEntry): Promise<void> { + await this.enqueueSessionWrite(sessionId, async () => { + const filePath = this.getSessionFilePath(sessionId); + let data: HistoryFileData; + ... + await fsPromises.writeFile(filePath, JSON.stringify(data, null, 2), 'utf-8'); + }); + }Apply the same queue wrapper to
deleteEntry,updateEntry, andclearSession.🤖 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 210 - 237, addEntry and other mutating methods perform a read-modify-write without serialization, so parallel calls can clobber each other; wrap per-session mutating operations (addEntry, deleteEntry, updateEntry, clearSession) with a per-session write queue/mutex (e.g., a Map keyed by sessionId storing a Promise chain or lock) so each operation awaits the previous for that session before doing fileExists, read, modify, and write (use getSessionFilePath and fileExists to locate files), ensure the queue unwraps on success/failure to avoid leaks, and apply the same wrapper to deleteEntry/updateEntry/clearSession to serialize their mutations and preserve history entries.
🧹 Nitpick comments (1)
src/main/utils/context-groomer.ts (1)
358-373: Overall timeout timer is never cleared on early completion.The
setTimeoutat line 358 is not stored and cleared incleanup(). When the operation completes early (via process exit, idle timeout, or cancellation), this timer remains scheduled. While the!resolvedcheck prevents harmful effects, the closure keeps references alive longer than necessary.♻️ Proposed fix to clear the overall timeout
let idleCheckInterval: NodeJS.Timeout | null = null; +let overallTimeoutId: NodeJS.Timeout | null = null; let resolved = false; ... const cleanup = () => { if (idleCheckInterval) { clearInterval(idleCheckInterval); idleCheckInterval = null; } + if (overallTimeoutId) { + clearTimeout(overallTimeoutId); + overallTimeoutId = null; + } processManager.off('data', onData); ... }; ... // Overall timeout -setTimeout(() => { +overallTimeoutId = setTimeout(() => { if (!resolved) { ... } }, timeoutMs);🤖 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 358 - 373, The overall timeout created with setTimeout is not stored or cleared, so keep its timer id and clear it when the operation finishes; modify the block that calls setTimeout to assign the return value to a variable (e.g., overallTimeoutId) and ensure cleanup() clears that timer (clearTimeout(overallTimeoutId)) alongside existing cleanup logic; update functions that end the flow (finishWithResponse, reject path, and any existing cleanup callers) to rely on cleanup() to clear the timer so the closure and responseBuffer references are released promptly.
🤖 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 489-501: The loop that writes updated session data exits early
because of the `break` after a successful write, so other entries with the same
`agentSessionId` are skipped; remove the `break` and let the loop continue
scanning all entries so every matching file is updated, ensure `updatedCount`
remains correct (aggregate increments across files) and leave the
`modified`/write/error handling (`fsPromises.writeFile`, `logger.debug`,
`logger.warn`, `captureException`) intact for each iteration where changes
occur.
In `@src/main/stats/stats-db.ts`:
- Around line 96-99: The initialize() method is vulnerable to concurrent
re-entry: add a single-flight in-flight promise guard (e.g.
this._initializePromise) so only the first caller performs the async work and
others await the same promise; set this._initializePromise at start of
initialize(), clear or null it on failure, set this.initialized and clear the
promise on success. Apply the same guard for any other code path that performs
the full DB initialization/migrations/backup/vacuum logic (the second
initialization block in the same class), ensuring both use the shared in-flight
promise and update this.initialized consistently.
---
Outside diff comments:
In `@src/main/history-manager.ts`:
- Around line 210-237: addEntry and other mutating methods perform a
read-modify-write without serialization, so parallel calls can clobber each
other; wrap per-session mutating operations (addEntry, deleteEntry, updateEntry,
clearSession) with a per-session write queue/mutex (e.g., a Map keyed by
sessionId storing a Promise chain or lock) so each operation awaits the previous
for that session before doing fileExists, read, modify, and write (use
getSessionFilePath and fileExists to locate files), ensure the queue unwraps on
success/failure to avoid leaks, and apply the same wrapper to
deleteEntry/updateEntry/clearSession to serialize their mutations and preserve
history entries.
In `@src/main/stats/stats-db.ts`:
- Around line 281-289: The code currently updates the _meta last_vacuum_at
whenever result.vacuumed is true even if the VACUUM operation failed; change the
logic so the timestamp is only written after a successful VACUUM. Update
vacuumIfNeeded() to return an explicit success flag (e.g., { vacuumed: boolean,
success?: boolean, error?: Error }) or ensure vacuumed is only true on success,
then here check that success (or absence of error) in addition to
result.vacuumed before calling the INSERT OR REPLACE into _meta; reference
vacuumIfNeeded(), result.vacuumed, and the _meta timestamp update to locate and
fix the behavior.
---
Nitpick comments:
In `@src/main/utils/context-groomer.ts`:
- Around line 358-373: The overall timeout created with setTimeout is not stored
or cleared, so keep its timer id and clear it when the operation finishes;
modify the block that calls setTimeout to assign the return value to a variable
(e.g., overallTimeoutId) and ensure cleanup() clears that timer
(clearTimeout(overallTimeoutId)) alongside existing cleanup logic; update
functions that end the flow (finishWithResponse, reject path, and any existing
cleanup callers) to rely on cleanup() to clear the timer so the closure and
responseBuffer references are released promptly.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/main/history-manager.tssrc/main/ipc/handlers/git.tssrc/main/stats/stats-db.tssrc/main/utils/context-groomer.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/ipc/handlers/git.ts
Comment on lines
+489
to
501
| if (modified) { | ||
| try { | ||
| await fsPromises.writeFile(filePath, JSON.stringify(data, null, 2), 'utf-8'); | ||
| logger.debug( | ||
| `Updated ${updatedCount} entries for agentSessionId ${agentSessionId} in session ${sessionId}`, | ||
| LOG_CONTEXT | ||
| ); | ||
| } catch (error) { | ||
| logger.warn(`Failed to write updates for session ${sessionId}: ${error}`, LOG_CONTEXT); | ||
| captureException(error, { operation: 'history:updateSessionNameWrite', sessionId }); | ||
| } | ||
| } catch (error) { | ||
| logger.warn(`Failed to update sessionName in session ${sessionId}: ${error}`, LOG_CONTEXT); | ||
| captureException(error, { operation: 'history:updateSessionName', sessionId }); | ||
| break; // Stop scanning once the target session is updated | ||
| } |
There was a problem hiding this comment.
Line 500 exits early, but the method contract says “all matching entries.”
The break stops after the first modified file, so additional files with the same agentSessionId are skipped.
🛠️ Suggested fix
- break; // Stop scanning once the target session is updated
+ // Continue scanning all session files to update all matches.🤖 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 489 - 501, The loop that writes
updated session data exits early because of the `break` after a successful
write, so other entries with the same `agentSessionId` are skipped; remove the
`break` and let the loop continue scanning all entries so every matching file is
updated, ensure `updatedCount` remains correct (aggregate increments across
files) and leave the `modified`/write/error handling (`fsPromises.writeFile`,
`logger.debug`, `logger.warn`, `captureException`) intact for each iteration
where changes occur.
Comment on lines
+96
to
99
| async initialize(): Promise<void> { | ||
| if (this.initialized) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Guard initialize() against concurrent re-entry.
Line 97 only checks this.initialized, but overlapping calls before Line 126 can both run full initialization (open DB, migrations, backup/vacuum path). Add an in-flight promise guard to make initialization single-flight.
Suggested fix
export class StatsDB {
private db: Database.Database | null = null;
private dbPath: string;
private initialized = false;
+ private initPromise: Promise<void> | null = null;
private static readonly fsPromises = fs.promises;
@@
async initialize(): Promise<void> {
if (this.initialized) {
return;
}
+ if (this.initPromise) {
+ return this.initPromise;
+ }
- try {
+ this.initPromise = (async () => {
+ try {
const dir = path.dirname(this.dbPath);
await StatsDB.fsPromises.mkdir(dir, { recursive: true });
@@
// Schedule VACUUM to run weekly instead of on every startup
await this.vacuumIfNeededWeekly();
} catch (error) {
logger.error(`Failed to initialize stats database: ${error}`, LOG_CONTEXT);
throw error;
+ } finally {
+ this.initPromise = null;
}
+ })();
+ return this.initPromise;
}Also applies to: 126-134
🤖 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 96 - 99, The initialize() method is
vulnerable to concurrent re-entry: add a single-flight in-flight promise guard
(e.g. this._initializePromise) so only the first caller performs the async work
and others await the same promise; set this._initializePromise at start of
initialize(), clear or null it on failure, set this.initialized and clear the
promise on success. Apply the same guard for any other code path that performs
the full DB initialization/migrations/backup/vacuum logic (the second
initialization block in the same class), ensuring both use the shared in-flight
promise and update this.initialized consistently.
…y discarding
Replace three `.catch(() => {})` patterns in codex-output-parser.ts that
silently swallowed all config loading errors. Now ENOENT errors (missing
config file) are handled gracefully while unexpected errors (EPERM, EIO,
etc.) are reported to Sentry via captureException. Adds 2 tests for the
error reporting behavior.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… type Tests were providing sync callbacks and not awaiting the async getHistory() return, causing assertions against Promise objects instead of resolved values. Aligned all getHistory tests with the async pattern used by other async callbacks (executeCommand, switchMode, etc.) in the same test suite. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/main/ipc/handlers/agentSessions.ts (1)
258-259:⚠️ Potential issue | 🟡 MinorGuard top-level
readdircalls against expected FS races.Line 258 and Line 339 run
fs.readdir(...)outsidetry/catch. If permissions or directory presence changes afterfs.access, expected codes (ENOENT/ENOTDIR/EACCES) can still fail the whole global stats pass.💡 Suggested fix
- const projectDirs = await fs.readdir(claudeProjectsDir); + let projectDirs: string[]; + try { + projectDirs = await fs.readdir(claudeProjectsDir); + } catch (e: unknown) { + if (isExpectedFsError(e)) { + setCachedSessionFiles('claude-code', files); + return files; + } + throw e; + } ... - const years = await fs.readdir(codexSessionsDir); + let years: string[]; + try { + years = await fs.readdir(codexSessionsDir); + } catch (e: unknown) { + if (isExpectedFsError(e)) { + setCachedSessionFiles('codex', files); + return files; + } + throw e; + }#!/bin/bash set -euo pipefail # Verify both root readdir calls are currently outside local try/catch guards. nl -ba src/main/ipc/handlers/agentSessions.ts | sed -n '244,266p' nl -ba src/main/ipc/handlers/agentSessions.ts | sed -n '334,346p'Expected result:
await fs.readdir(claudeProjectsDir)andawait fs.readdir(codexSessionsDir)appear as direct statements, not wrapped.
Based on learnings: Do not silently swallow exceptions in try-catch blocks; explicitly handle known/recoverable failure modes and let unexpected errors bubble.Also applies to: 339-340
🤖 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 258 - 259, The top-level fs.readdir calls (e.g., the statement creating projectDirs from claudeProjectsDir and the similar call for codexSessionsDir) must be guarded so transient FS races don't blow up the global stats pass: wrap each await fs.readdir(...) in a try/catch, explicitly handle known recoverable errors (ENOENT, ENOTDIR, EACCES) by treating them as "no entries" (e.g., set projectDirs/codexDirs to an empty array or skip processing) and rethrow any other unexpected errors so they bubble up; update the code around the projectDirs and codexSessionsDir reads to use these guarded reads rather than unprotected top-level readdir calls.
🧹 Nitpick comments (4)
src/main/ipc/handlers/agentSessions.ts (1)
121-133: Consider defensive copying in session discovery cache helpers.Line 124 and Line 131 currently share the same array reference across cache and callers. Returning/storing shallow copies avoids accidental mutation of cache state.
♻️ Suggested refactor
function getCachedSessionFiles(provider: SessionDiscoveryProvider): SessionFileInfo[] | null { const cached = sessionDiscoveryCache[provider]; if (cached && cached.expiresAt > Date.now()) { - return cached.files; + return [...cached.files]; } return null; } function setCachedSessionFiles(provider: SessionDiscoveryProvider, files: SessionFileInfo[]): void { sessionDiscoveryCache[provider] = { - files, + files: [...files], expiresAt: Date.now() + SESSION_DISCOVERY_CACHE_TTL_MS, }; }🤖 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 121 - 133, getCachedSessionFiles and setCachedSessionFiles currently share the same array reference between cache and callers which allows accidental mutation; change setCachedSessionFiles to store a shallow copy of the incoming files (e.g., clone the array) and change getCachedSessionFiles to return a shallow copy of cached.files instead of the original reference so the sessionDiscoveryCache and callers cannot mutate each other's arrays; reference the functions getCachedSessionFiles, setCachedSessionFiles and the sessionDiscoveryCache/SESSION_DISCOVERY_CACHE_TTL_MS constants when making this change.src/__tests__/main/parsers/codex-output-parser.test.ts (2)
584-586: Avoid testing private state via double-cast.Lines 584-586 couple tests to internals. The public assertion at Line 598 already validates effective behavior and is less brittle.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/main/parsers/codex-output-parser.test.ts` around lines 584 - 586, Remove the brittle double-cast assertion that inspects private state: delete the expect(...) checking (parserWithConfig as unknown as { contextWindow: number }).contextWindow and instead rely on the existing public behavior assertion (the later expect around Line 598) to validate effective behavior of the parser; search for parserWithConfig and contextWindow in the test and remove the internal-state check so tests only assert via public API/outputs.
510-517: Make the Sentry assertion deterministic.Lines 512-517 can pass even when error reporting regresses. Inject a deterministic non-ENOENT read failure (e.g., mocked
fs.readFilewithcode: 'EACCES') and assertcaptureExceptionunconditionally.🧪 Proposed test hardening
it('should report non-ENOENT config read errors to Sentry', async () => { const captureSpy = vi.spyOn(sentry, 'captureException').mockResolvedValue(); const tempDir = await fs.mkdtemp(path.join(os.tmpdir(), 'codex-config-err-')); const configPath = path.join(tempDir, 'config.toml'); - // Write a file so fs.access succeeds, then make it unreadable + // Write a file so the config path exists await fs.writeFile(configPath, 'model = "gpt-4"'); - await fs.chmod(configPath, 0o000); + const readFileSpy = vi + .spyOn(fs, 'readFile') + .mockRejectedValue(Object.assign(new Error('EACCES'), { code: 'EACCES' })); @@ - // On systems where root runs tests, chmod(0o000) may not prevent reads. - // Skip the assertion if captureException wasn't called (root or similar). - if (captureSpy.mock.calls.length > 0) { - expect(captureSpy).toHaveBeenCalledWith( - expect.any(Error), - expect.objectContaining({ context: 'codex-config-read' }) - ); - } + expect(captureSpy).toHaveBeenCalledWith( + expect.any(Error), + expect.objectContaining({ context: 'codex-config-read' }) + ); } finally { - // Restore permissions so cleanup works - await fs.chmod(configPath, 0o644); + readFileSpy.mockRestore(); await fs.rm(tempDir, { recursive: true, force: true });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/main/parsers/codex-output-parser.test.ts` around lines 510 - 517, The Sentry assertion is flaky because it only checks captureSpy when a real read failure occurs; instead, mock fs.readFile to deterministically throw an Error with code 'EACCES' before running the test so the read always fails, remove the conditional on captureSpy.mock.calls, and assert unconditionally that captureSpy was called with expect.any(Error) and expect.objectContaining({ context: 'codex-config-read' }); target the existing test block that sets up captureSpy and replace the real fs read with a mock (mocking fs.readFile or the module used to read the config) to produce the EACCES error.src/main/parsers/codex-output-parser.ts (1)
187-195: Keep__codexConfigTestUtilsout of the runtime public API.Exporting test helpers from production code increases API surface and invites accidental runtime coupling. Prefer a test-only helper module or test-side mocking around
loadCodexConfigCached.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/parsers/codex-output-parser.ts` around lines 187 - 195, Remove the runtime export of __codexConfigTestUtils and instead provide these helpers from a test-only module or via test-side mocking; specifically, stop exporting __codexConfigTestUtils from this file (leave or remove the symbol internally), and move the implementations that call clearCodexConfigCache, reference codexConfigLoadPromise and codexConfigCache, and any helpers around loadCodexConfigCached into a dedicated test helper file imported only by tests (or mock loadCodexConfigCached in tests). Update tests to import the new test helper or mock instead of the production module so the production public API no longer exposes __codexConfigTestUtils.
🤖 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/parsers/codex-output-parser.ts`:
- Around line 122-126: Remove the pre-check using fs.access(fsConstants.R_OK)
against configPath and its surrounding try/catch so that errors aren't
swallowed; instead rely on the subsequent fs.readFile call in the same function
(where the error handling and captureException logic exists) to surface
permission/IO errors to the existing handler (e.g., the readFile call and the
captureException block around it), ensuring non-ENOENT failures get reported to
Sentry.
---
Duplicate comments:
In `@src/main/ipc/handlers/agentSessions.ts`:
- Around line 258-259: The top-level fs.readdir calls (e.g., the statement
creating projectDirs from claudeProjectsDir and the similar call for
codexSessionsDir) must be guarded so transient FS races don't blow up the global
stats pass: wrap each await fs.readdir(...) in a try/catch, explicitly handle
known recoverable errors (ENOENT, ENOTDIR, EACCES) by treating them as "no
entries" (e.g., set projectDirs/codexDirs to an empty array or skip processing)
and rethrow any other unexpected errors so they bubble up; update the code
around the projectDirs and codexSessionsDir reads to use these guarded reads
rather than unprotected top-level readdir calls.
---
Nitpick comments:
In `@src/__tests__/main/parsers/codex-output-parser.test.ts`:
- Around line 584-586: Remove the brittle double-cast assertion that inspects
private state: delete the expect(...) checking (parserWithConfig as unknown as {
contextWindow: number }).contextWindow and instead rely on the existing public
behavior assertion (the later expect around Line 598) to validate effective
behavior of the parser; search for parserWithConfig and contextWindow in the
test and remove the internal-state check so tests only assert via public
API/outputs.
- Around line 510-517: The Sentry assertion is flaky because it only checks
captureSpy when a real read failure occurs; instead, mock fs.readFile to
deterministically throw an Error with code 'EACCES' before running the test so
the read always fails, remove the conditional on captureSpy.mock.calls, and
assert unconditionally that captureSpy was called with expect.any(Error) and
expect.objectContaining({ context: 'codex-config-read' }); target the existing
test block that sets up captureSpy and replace the real fs read with a mock
(mocking fs.readFile or the module used to read the config) to produce the
EACCES error.
In `@src/main/ipc/handlers/agentSessions.ts`:
- Around line 121-133: getCachedSessionFiles and setCachedSessionFiles currently
share the same array reference between cache and callers which allows accidental
mutation; change setCachedSessionFiles to store a shallow copy of the incoming
files (e.g., clone the array) and change getCachedSessionFiles to return a
shallow copy of cached.files instead of the original reference so the
sessionDiscoveryCache and callers cannot mutate each other's arrays; reference
the functions getCachedSessionFiles, setCachedSessionFiles and the
sessionDiscoveryCache/SESSION_DISCOVERY_CACHE_TTL_MS constants when making this
change.
In `@src/main/parsers/codex-output-parser.ts`:
- Around line 187-195: Remove the runtime export of __codexConfigTestUtils and
instead provide these helpers from a test-only module or via test-side mocking;
specifically, stop exporting __codexConfigTestUtils from this file (leave or
remove the symbol internally), and move the implementations that call
clearCodexConfigCache, reference codexConfigLoadPromise and codexConfigCache,
and any helpers around loadCodexConfigCached into a dedicated test helper file
imported only by tests (or mock loadCodexConfigCached in tests). Update tests to
import the new test helper or mock instead of the production module so the
production public API no longer exposes __codexConfigTestUtils.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/__tests__/main/ipc/handlers/agentSessions.test.tssrc/__tests__/main/parsers/codex-output-parser.test.tssrc/main/ipc/handlers/agentSessions.tssrc/main/parsers/codex-output-parser.ts
Comment on lines
+122
to
+126
| try { | ||
| await fs.access(configPath, fsConstants.R_OK); | ||
| } catch { | ||
| return {}; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/main/parsers/codex-output-parser.ts | head -200Repository: RunMaestro/Maestro
Length of output: 7411
Don't suppress non-ENOENT failures in the config pre-check.
Lines 122–126 use a bare catch on fs.access, which suppresses all errors including permission (EACCES) and I/O failures. These never reach the captureException path at line 150, making them invisible to Sentry.
Remove the pre-check entirely. The readFile call at line 129 will naturally fail with the appropriate error code, which the handler at lines 146–151 can then properly inspect and report.
🔧 Proposed fix
-import { constants as fsConstants, promises as fs } from 'fs';
+import { promises as fs } from 'fs';
...
async function readCodexConfigFromDisk(): Promise<CodexConfig> {
const codexHome = process.env.CODEX_HOME || path.join(os.homedir(), '.codex');
const configPath = path.join(codexHome, 'config.toml');
-
- try {
- await fs.access(configPath, fsConstants.R_OK);
- } catch {
- return {};
- }
try {
const content = await fs.readFile(configPath, 'utf8');
const result: CodexConfig = {};
@@
} catch (e: unknown) {
const code = (e as NodeJS.ErrnoException)?.code;
- if (code === 'ENOENT') return {};
+ if (code === 'ENOENT' || code === 'ENOTDIR') return {};
// Unexpected error reading config — report to Sentry, fall back to defaults
void captureException(e, { context: 'codex-config-read', configPath });
return {};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/parsers/codex-output-parser.ts` around lines 122 - 126, Remove the
pre-check using fs.access(fsConstants.R_OK) against configPath and its
surrounding try/catch so that errors aren't swallowed; instead rely on the
subsequent fs.readFile call in the same function (where the error handling and
captureException logic exists) to surface permission/IO errors to the existing
handler (e.g., the readFile call and the captureException block around it),
ensuring non-ENOENT failures get reported to Sentry.
searchRegex was built from raw searchQuery (with potential leading/trailing whitespace) but gated by trimmedSearchQuery. This meant a query like " foo " would pass the hasSearchQuery check but create a regex with whitespace that wouldn't match "foo" in the content. Now uses trimmedSearchQuery for regex construction. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace console.error with Sentry.captureException in LongestAutoRunsTable fetchData catch block so Auto Run session fetch errors are reported to Sentry instead of silently logged to console. Added test verifying error reporting with correct context metadata. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…lapse button Align the worktree collapse button (lines 1799-1813) to consistent 6-tab indentation, matching the collapsed button pattern above it. The button tag and its attributes were at mixed indentation levels. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…utput-parser Replace test-only __codexConfigTestUtils object with direct imports of clearCodexConfigCache() and loadCodexConfigCached(), now exported as proper named functions. Tests use these directly instead of a bundled test utility wrapper. Removed redundant cache state assertions in favor of existing parser behavior checks. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/renderer/components/SessionList.tsx (1)
1981-2019:⚠️ Potential issue | 🟡 MinorComment does not match the dependency array.
The comment states "Keep dependencies limited to sessionFilter and sessions" but the dependency array only includes
[sessionFilter]. Additionally,sessionFilterOpenis read at line 2014 but not listed, which could cause theelse if (sessionFilterOpen)branch to use a stale value.If intentionally excluding
sessionsandsessionFilterOpento avoid re-running on every change, update the comment to reflect the actual behavior. Otherwise, consider including the missing dependencies.Suggested comment fix
- // Note: Keep dependencies limited to sessionFilter and sessions to avoid unnecessary effect churn + // Note: Only react to sessionFilter changes - sessions/sessionFilterOpen reads use current values + // but don't trigger re-runs to avoid unnecessary effect churn🤖 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 1981 - 2019, The comment above the useEffect is inaccurate: it says "Keep dependencies limited to sessionFilter and sessions" but the dependency array only contains [sessionFilter] while the effect reads sessions and sessionFilterOpen; update the code by either (A) including the missing dependencies—add sessions and sessionFilterOpen to the dependency array of the useEffect so the effect re-runs when they change, or (B) if you intentionally want only sessionFilter to trigger the effect, change the comment to explicitly state that sessions and sessionFilterOpen are omitted intentionally and why, ensuring you still reference the same symbols (useEffect, sessionFilter, sessions, sessionFilterOpen, setGroups, setBookmarksCollapsed) so reviewers can verify the intent.src/renderer/components/UsageDashboard/LongestAutoRunsTable.tsx (1)
129-134:⚠️ Potential issue | 🟠 MajorGuard against stale async responses overwriting current table state.
The
fetchDatafunction can be triggered from two independent sources: theuseEffecton line 144 whentimeRangechanges, and theonStatsUpdatecallback on line 147. This creates a race condition where overlapping requests can complete out of order—a slower older request will overwrite newer state. State updates on lines 133 and 139 lack freshness checks to prevent this.For example: if
timeRangechanges from "week" to "month", but a pending "week" request completes after the "month" request, stale "week" data will overwrite the fresh result.Implement request sequencing by tracking a unique ID for each fetch and validating it before state updates:
Suggested fix
-import { memo, useState, useEffect, useMemo, useCallback } from 'react'; +import { memo, useState, useEffect, useMemo, useCallback, useRef } from 'react'; @@ const [sessions, setSessions] = useState<AutoRunSession[]>([]); const [loading, setLoading] = useState(true); + const latestRequestId = useRef(0); @@ const fetchData = useCallback(async () => { + const requestId = ++latestRequestId.current; setLoading(true); try { const autoRunSessions = await window.maestro.stats.getAutoRunSessions(timeRange); + if (requestId !== latestRequestId.current) return; setSessions(autoRunSessions); } catch (err) { Sentry.captureException(err, { extra: { context: 'LongestAutoRunsTable.fetchData', timeRange }, }); } finally { + if (requestId !== latestRequestId.current) return; setLoading(false); } }, [timeRange]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/UsageDashboard/LongestAutoRunsTable.tsx` around lines 129 - 134, The fetchData function can suffer from out-of-order async responses overwriting newer state; fix by adding a monotonic request id (e.g. a useRef named fetchCounter/currentFetchId) that you increment at the start of fetchData, capture into a local const (reqId) before awaiting window.maestro.stats.getAutoRunSessions, and only call setSessions and setLoading(false) if reqId === currentFetchId.current; apply the same guard when fetchData is invoked from onStatsUpdate so stale responses can't overwrite newer table state.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/renderer/components/SessionList.tsx`:
- Around line 1981-2019: The comment above the useEffect is inaccurate: it says
"Keep dependencies limited to sessionFilter and sessions" but the dependency
array only contains [sessionFilter] while the effect reads sessions and
sessionFilterOpen; update the code by either (A) including the missing
dependencies—add sessions and sessionFilterOpen to the dependency array of the
useEffect so the effect re-runs when they change, or (B) if you intentionally
want only sessionFilter to trigger the effect, change the comment to explicitly
state that sessions and sessionFilterOpen are omitted intentionally and why,
ensuring you still reference the same symbols (useEffect, sessionFilter,
sessions, sessionFilterOpen, setGroups, setBookmarksCollapsed) so reviewers can
verify the intent.
In `@src/renderer/components/UsageDashboard/LongestAutoRunsTable.tsx`:
- Around line 129-134: The fetchData function can suffer from out-of-order async
responses overwriting newer state; fix by adding a monotonic request id (e.g. a
useRef named fetchCounter/currentFetchId) that you increment at the start of
fetchData, capture into a local const (reqId) before awaiting
window.maestro.stats.getAutoRunSessions, and only call setSessions and
setLoading(false) if reqId === currentFetchId.current; apply the same guard when
fetchData is invoked from onStatsUpdate so stale responses can't overwrite newer
table state.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/__tests__/main/web-server/managers/CallbackRegistry.test.tssrc/__tests__/renderer/components/FilePreview.test.tsxsrc/__tests__/renderer/components/UsageDashboardModal.test.tsxsrc/renderer/components/FilePreview.tsxsrc/renderer/components/SessionList.tsxsrc/renderer/components/UsageDashboard/LongestAutoRunsTable.tsx
pedramamini
added a commit
that referenced
this pull request
Mar 1, 2026
React.memo wraps for 29 components (7 core + 22 UsageDashboard charts) to prevent unnecessary re-renders on parent state changes. Add compound DB indexes (start_time, agent_type/project_path/source) for common query patterns in stats aggregation. Parallelize sequential git rev-parse calls in worktreeInfo and worktreeSetup handlers with Promise.all (~4x fewer round-trips). Cache Date.getTime() results in useFilteredAndSortedSessions sort comparator to avoid repeated date parsing during sort operations. Replace inline debounce in useStats with shared useDebouncedCallback hook, adding proper cleanup on unmount.
pedramamini
added a commit
that referenced
this pull request
Mar 1, 2026
* perf: remove streaming debug logs and fix index-based React keys
Remove 7 console.log calls from useSessionDebounce that fired at high
frequency during AI streaming. Replace index-based keys with stable
identifiers in 6 components where items can be removed or filtered
(staged images, diff tabs, log entries, quit modal agents).
* fix: address PR review feedback from CodeRabbit
- QuitConfirmModal: use composite key `${name}-${index}` to prevent
duplicate React keys when agents share display names
- extensionColors: early-return accent fallback in colorblind mode so
we never serve non-colorblind-safe colors from EXTENSION_MAP
* perf: prevent session bloat by stripping file tree and preview history from persistence
Users with large working directories (100K+ files) caused sessions.json to
balloon to 300MB+. Root cause: the full file tree (FileTreeNode[]) was persisted
for every session with no size limits.
Three changes:
1. Strip fileTree, fileTreeStats, filePreviewHistory from persistence — these
are ephemeral cache data that re-scan automatically on session activation
2. Add configurable local ignore patterns setting (default: .git, node_modules,
__pycache__) with UI in Settings > Display, reusing a new generic
IgnorePatternsSection component extracted from SshRemoteIgnoreSection
3. Wire local ignore patterns through loadFileTree for local (non-SSH) scans
* feat: honor local .gitignore patterns in file tree indexing
Add localHonorGitignore setting (default: true) that parses .gitignore
files and merges their patterns with the user's local ignore patterns
when building file trees. Mirrors the existing SSH remote gitignore
support for local filesystem scans.
* refactor: clean up file-scale PR with shared gitignore parser, options object, and rescan effect
- Extract parseGitignoreContent() shared between local/remote gitignore handling
- Replace 7 positional params in loadFileTree with LocalFileTreeOptions object
- Export DEFAULT_LOCAL_IGNORE_PATTERNS from settingsStore, use in SettingsModal
- Fix deprecated onKeyPress -> onKeyDown in IgnorePatternsSection
- Add role="checkbox" and aria-checked to honor-gitignore toggle
- Add useEffect to auto-rescan file tree when local ignore patterns change
- Update tests for new loadFileTree signature
* perf: cherry-pick low-risk optimizations from PR #385
React.memo wraps for 29 components (7 core + 22 UsageDashboard charts)
to prevent unnecessary re-renders on parent state changes.
Add compound DB indexes (start_time, agent_type/project_path/source)
for common query patterns in stats aggregation.
Parallelize sequential git rev-parse calls in worktreeInfo and
worktreeSetup handlers with Promise.all (~4x fewer round-trips).
Cache Date.getTime() results in useFilteredAndSortedSessions sort
comparator to avoid repeated date parsing during sort operations.
Replace inline debounce in useStats with shared useDebouncedCallback
hook, adding proper cleanup on unmount.
* fix: make blocked Symphony issues selectable for document exploration
Blocked issues were completely non-interactive. Now they can be clicked
and keyboard-navigated to explore their documents, while the Start
Symphony button remains hidden to prevent starting blocked work.
* feat: add group @mentions in group chat and fix navigation history
Group @mentions: typing @ in group chat now shows agent groups in the
dropdown (visually differentiated with icon and member count badge).
Selecting a group expands to individual @mentions for all member agents.
Navigation history: group chats now participate in back/forward breadcrumb
navigation. Fixed pre-existing dual-instance bug where useNavigationHistory
was instantiated independently in App.tsx and useSessionLifecycle.ts,
creating separate stacks that couldn't communicate.
* feat: allow changing agent provider in Edit Agent modal
Replace the read-only provider display with a dropdown selector.
When provider changes, tabs reset to a fresh session and provider-
specific config (path, args, env, model) is cleared. History data
persists since it's keyed by Maestro session ID.
* feat: archive state for group chat (#377)
* feat: add archive state to group-chats
* fix: deduplicate GroupChat return type in global.d.ts, make archive callback optional
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
* fix: address PR #450 review feedback across stats, UI, and accessibility
Bug fixes: use tasksTotal for AutoRunStats count, add division-by-zero
guard in WeekdayComparisonChart, fix useStats effect cleanup, add v4
stats migration for compound indexes.
Sentry compliance: replace console.error with captureException in
QRCode, AutoRunStats, LongestAutoRunsTable, TasksByHourChart.
Accessibility: add keyboard handlers to AutoRunStats bar items, add
target guard and focus-visible ring to SymphonyModal.
Performance: memo MetricCard, useMemo for PeakHoursChart hasData,
useId for DurationTrendsChart gradient, stable opacities in
ChartSkeletons, stable onRemove ref in Toast, consistent variable
naming in git handler.
* fix: move blocked issue message to footer with greyed-out Start button
Removed inline blocked info box from the detail header. The footer now
shows for blocked issues with a disabled Start Symphony button and a
lock-icon message explaining the dependency block, consistent with the
normal footer layout.
* fix: prevent context menus from rendering off-screen
Replace ad-hoc hardcoded pixel buffers across 6 context menu
implementations with a shared useContextMenuPosition hook that
measures actual rendered menu size via getBoundingClientRect in
useLayoutEffect, clamping position to keep menus fully visible.
Two menus (SessionActivityGraph, GroupChatHistoryPanel) previously
had no viewport detection at all.
* fix: prevent orphaned file preview when tab bar is hidden
File preview rendering was independent of inputMode, so switching to
terminal mode (via authenticateAfterError or remote mode switch) without
clearing activeFileTabId left the file preview visible with no tab bar.
The corrupted state persisted across app restarts.
Guard file preview with inputMode='ai', clear activeFileTabId on all
terminal-mode transitions, and auto-heal on session restoration.
* fix: address PR review — missed onRemoteCommand path and weak test
- Clear activeFileTabId in onRemoteCommand mode-sync path (same bug
as onRemoteSwitchMode, caught by CodeRabbit)
- Fix "preserves activeFileTabId" test to use non-null starting value
so it actually tests preservation behavior
- Add test for onRemoteCommand clearing activeFileTabId on terminal sync
* polish: UI refinements for header z-index, status dot opacity, and Director's Notes beta badge
- Add z-20 to header container to prevent overlap issues
- Dim agent status dots for inactive sessions (0.45 opacity)
- Add beta badge to Director's Notes in Encore Features settings
* polish: Director's Notes keeps old content visible during regeneration
Cache no longer keys on lookbackDays — once generated, notes persist
until explicit Regenerate click. During regeneration, old notes stay
visible and scrollable with stats bar, timestamp, and Save/Copy buttons
remaining enabled. Progress bar only shows on first-ever generation.
Renamed "Refresh" to "Regenerate" for clearer intent.
* fix: update prop extraction hooks for rebased branch compatibility
Add ~37 missing props to useMainPanelProps interface, useMemo return,
and dependency array. Remove state props from useRightPanelProps call
in App.tsx that the refactored hook no longer accepts (RightPanel now
reads state directly from Zustand stores).
* feat: add layout algorithm options to Document Graph (Mind Map, Radial, Force-Directed)
Adds a dropdown in the Document Graph header to switch between three
layout algorithms. Layout choice persists per-agent (via Session) with
global default fallback (via settingsStore).
- Extract layout code from MindMap.tsx into mindMapLayouts.ts
- Add Radial layout (concentric rings) and Force-Directed layout (d3-force)
- Add layout type dropdown UI with layer stack Escape handling
- Wire persistence through App.tsx (session + global default)
- Add tests for all three algorithms, settingsStore, and props
* fix: lint cleanup, auto-scroll empty-tab guard, and worktree batch improvements
- Fix ESLint errors: empty catch block comment, unused eslint-disable directive,
unused variable prefixing in App.tsx and DirectorNotesModal
- Hide auto-scroll button when tab has no content (filteredLogs.length > 0 guard)
- Fix worktree auto-run: skip setupWorktree when worktreeTarget already created
the worktree, preventing "belongs to different repository" false positive
- Add "Auto Run Started" toast notification with task/document counts
- Record PR creation results in agent history panel
- Update auto-scroll and batch processor tests for new behavior
* feat: show issue counts on Symphony tiles via GitHub Search API
Uses a single GitHub Search API call to batch-fetch open issue counts
for all registered repos (instead of 2 calls per repo). Tiles now show
"View #N Issues" when issues exist, "No Issues" (ghosted at 45% opacity)
when none, and the original "View Issues" while counts are loading.
- New IPC handler: symphony:getIssueCounts (30-min cache TTL)
- New fetchIssueCounts() using /search/issues with multiple repo: qualifiers
- Counts fetched automatically when registry loads
- Updated RepositoryTile, useSymphony hook, preload bridge, global.d.ts
* fix: address PR #485 review feedback — null safety, cache validation, a11y, formatting
- readFile ENOENT null return: update global.d.ts type to string | null,
add null guards across 12 call sites (MarkdownRenderer, FilePreview,
AutoRun, DocumentEditor, DocumentGenerationView, ImageDiffViewer,
DocumentGraphView, DebugWizardModal, useAutoRunImageHandling,
useFileExplorerEffects, useTabHandlers, useAppHandlers)
- Symphony issue counts: add pagination (up to 1,000 results), validate
cache against requested repoSlugs, clear stale counts on failure/empty
- Worktree fixes: clear dedup marker on worktreeSetup throw, propagate
targetBranch in PR creation catch path
- GroupChatHeader: add keyboard a11y (tabIndex, role, onKeyDown)
- Settings: extract DOCUMENT_GRAPH_LAYOUT_TYPES constant, dedup validation
- DocumentGraphView: sync layoutType state with defaultLayoutType prop
- Tests: fix vi.hoisted() TDZ, proper Shortcut mock shape, clearAllMocks,
assert console.warn in dedup tests, update symphony cache type test
- Prettier: format all CI-flagged files
---------
Co-authored-by: Sam Shpuntoff <82393483+sshpuntoff@users.noreply.github.com>
Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
- imageUtils.test.ts: Add missing buildImagePromptPrefix() export - MarkdownRenderer.test.tsx: Fix tests to match actual rehype-raw behavior (component uses rehype-raw, not DOMPurify for HTML sanitization) - auto-scroll.test.tsx: Remove tests for unimplemented auto-scroll pin/unpin feature (autoScrollAiMode props don't exist on TerminalOutput) - SymphonyModal.test.tsx: Remove tests for unimplemented gh CLI pre-flight check flow (component has no checkGhCli integration) - agent-spawner.test.ts: Fix vi.resetModules() mock mismatch by using module-level mock functions with closure delegation for fs.promises.stat and fs.promises.access; fix missing fs.constants export in mock factory Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
Author
Review Fixes AppliedAll review items addressed: 2 blocking (pedramamini), 2 should-fix (pedramamini), 4 CodeRabbit actionable, 3 minor/nitpicks, plus 28 test failures fixed. Branch merges cleanly with main (verified via Blocking Fixes (pedramamini)
Should-Fix (pedramamini)
CodeRabbit Actionable Fixes
Minor / Nitpicks
Test Suite Fixes (28 failures → 0)Fixed 28 test failures across 5 test files that were pre-existing or surfaced during verification:
Decision on test file removal: Verification
|
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
2 participants
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.
Closes #371
Summary
Systematic UX performance sweep targeting React rendering efficiency, main-process responsiveness, and database query performance. All changes are low-risk, behavior-preserving optimizations.
Changes
React Memoization (PERF-01 / PERF-02 / PERF-03)
React.memowraps for high-churn components:SessionListItem,ExecutionQueueIndicator,Toast,ConfirmModal,RenameTabModal,QRCode,QuickActionsModal,HistoryHelpModal,ContextWarningSash, and all 14UsageDashboardchart componentsuseMemofor expensive computations: session list sorts, AutoRun regex compilation, FilePreview regex/formatting, CsvTableRenderer regex, date tooltip formattingSessionListItem,SessionList,AICommandsPanelto prevent re-render allocationMain Process Async I/O (PERF-04 / PERF-05)
history-manager.ts: converted all syncfs.*calls tofs.promises.*, added async helper utilities, updated all IPC/director-notes/web-server callers to await the async flowsstats-db.ts/singleton.ts: async initialization, backup, vacuum, and directory creation; full test suite updatedgroup-chats.ts,storage.ts): async file readscodex-output-parser.ts: async config cachingimageUtils.ts: async temp file writeswslDetector.ts: async detectionuseStatshook: reuse shared debounced callback to reduce redundant fetchesDatabase & Query Performance (PERF-06)
stats-dbschema for common access patternsPromise.allfor independent git queries in IPC handlersVIBES Metadata
.ai-audit/directory with annotations, config, and manifest for AI code provenance trackingTest Plan
npx vitest run src/__tests__/main/history-manager.test.ts src/__tests__/main/ipc/handlers/history.test.ts src/__tests__/main/ipc/handlers/director-notes.test.ts src/__tests__/main/stats/*.test.tsnpx vitest run— full suite regression checknpm run buildsucceedsRisk Assessment
All changes are behavior-preserving:
React.memowraps cannot break behavior (only skip unnecessary re-renders)useMemocaches pure computations🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Performance
Refactor
UX / Reliability