fix: prevent orphaned file preview when tab bar is hidden#456
Merged
pedramamini merged 22 commits intomainfrom Feb 25, 2026
Merged
fix: prevent orphaned file preview when tab bar is hidden#456pedramamini merged 22 commits intomainfrom
pedramamini merged 22 commits intomainfrom
Conversation
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).
- 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
…y 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
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.
…s 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
…n 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.
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.
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.
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.
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: 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>
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.
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.
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.
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.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review infoConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughClear orphaned file previews by nullifying Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
Greptile SummaryThis PR fixes a critical UI bug where file previews became orphaned (visible without a tab bar) when switching to terminal mode, creating an unnavigable dead-end state that persisted across app restarts. Core Fix (commit
Test Coverage:
Other Changes in PR:
Confidence Score: 5/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[User Action] --> B{Input Mode Change?}
B -->|Auth Error| C[agentStore.authenticateAfterError]
B -->|Remote Mode Switch| D[useRemoteIntegration]
B -->|Manual Toggle| E[App.tsx toggleInputMode]
C --> F{Switch to Terminal?}
D --> F
E --> F
F -->|Yes| G[Clear activeFileTabId]
F -->|No| H[Preserve/Restore activeFileTabId]
I[App Restart] --> J[useSessionRestoration.restoreSession]
J --> K{Corrupted State?}
K -->|inputMode != 'ai' BUT activeFileTabId exists| L[Auto-heal: Clear activeFileTabId]
K -->|Valid State| M[Preserve State]
G --> N[MainPanel Render]
H --> N
L --> N
M --> N
N --> O{inputMode === 'ai' AND activeFileTabId?}
O -->|Yes| P[Render File Preview]
O -->|No| Q[Render Terminal Output]
style G fill:#90EE90
style L fill:#90EE90
style P fill:#ADD8E6
style Q fill:#ADD8E6
Last reviewed commit: 3396b77 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
src/renderer/components/Toast.tsx (1)
263-266:⚠️ Potential issue | 🟡 MinorClose button: missing
aria-labelandtype="button".The close button is icon-only (SVG, no visible text) and has no
aria-label, so screen readers will announce it as an unlabelled button. It also lackstype="button", which is inconsistent with the action URL button at line 222 and creates a latent risk if this component is ever wrapped in a form.♿ Proposed fix
- <button - onClick={handleClose} - className="flex-shrink-0 p-1 rounded hover:bg-opacity-10 transition-colors" - style={{ color: theme.colors.textDim }} - > + <button + type="button" + aria-label="Dismiss notification" + onClick={handleClose} + className="flex-shrink-0 p-1 rounded hover:bg-opacity-10 transition-colors" + style={{ color: theme.colors.textDim }} + >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/Toast.tsx` around lines 263 - 266, The close button inside the Toast component needs accessibility and form-safety attributes: update the button rendered where onClick={handleClose} (the icon-only close control) to include type="button" and an appropriate aria-label (e.g., aria-label="Close" or aria-label prop passed down) so screen readers can identify it; ensure this change is applied where the close SVG button is rendered in Toast.tsx alongside the existing className and style props.src/renderer/components/QRCode.tsx (1)
38-62:⚠️ Potential issue | 🟡 Minor
setErroris not reset whenvaluebecomes empty, leaving stale error UI visible.When
!valuethe early-return path only callssetDataUrl(null)but leaves any pre-existingerrorstate intact. Because the error branch renders before the loading-placeholder branch (line 65 vs 76), a component that previously failed will continue showing "Failed to generate QR code" even aftervalueis cleared — the loading placeholder is never reached.🐛 Proposed fix
useEffect(() => { if (!value) { setDataUrl(null); + setError(null); return; }🤖 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 38 - 62, The useEffect early-return path doesn't clear prior errors, so when value is falsy the component still shows a stale error; update the useEffect so that when !value you call setError(null) in addition to setDataUrl(null) (same block that returns), ensuring any previous error from QRCodeLib.toDataURL (and captureException) is cleared; locate the effect using useEffect and the handlers setDataUrl/setError and QRCodeLib.toDataURL to apply the change.src/renderer/components/UsageDashboard/LongestAutoRunsTable.tsx (1)
126-146:⚠️ Potential issue | 🟠 MajorRace condition: concurrent
fetchDatacalls can clobber each other's results.
fetchData()is called immediately on mount, and again each timeonStatsUpdatefires. If the stat-update event arrives while the initial fetch (or a prior update's fetch) is still in-flight, both calls race tosetSessions/setLoading. Whichever resolves last wins — this can overwrite a fresher result with older API data.The standard fix is an
AbortController(or anisCurrentboolean guard) that marks the previous fetch stale before a new one starts:🐛 Proposed fix — abort stale in-flight fetches
- const fetchData = useCallback(async () => { - setLoading(true); - try { - const autoRunSessions = await window.maestro.stats.getAutoRunSessions(timeRange); - setSessions(autoRunSessions); - } catch (err) { - captureException(err); - } finally { - setLoading(false); - } - }, [timeRange]); - - useEffect(() => { - fetchData(); - - const unsubscribe = window.maestro.stats.onStatsUpdate(() => { - fetchData(); - }); - - return () => unsubscribe(); - }, [fetchData]); + const fetchData = useCallback(async (signal: AbortSignal) => { + setLoading(true); + try { + const autoRunSessions = await window.maestro.stats.getAutoRunSessions(timeRange); + if (!signal.aborted) { + setSessions(autoRunSessions); + } + } catch (err) { + if (!signal.aborted) { + captureException(err); + } + } finally { + if (!signal.aborted) { + setLoading(false); + } + } + }, [timeRange]); + + useEffect(() => { + const controller = new AbortController(); + fetchData(controller.signal); + + const unsubscribe = window.maestro.stats.onStatsUpdate(() => { + controller.abort(); // cancel any in-flight fetch + fetchData(new AbortController().signal); + }); + + return () => { + controller.abort(); + unsubscribe(); + }; + }, [fetchData]);Alternatively, keep a
latestFetchIdref (increment on each call, ignore results whose id ≠ current) ifgetAutoRunSessionsdoesn't respect anAbortSignal.🤖 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 126 - 146, fetchData can be called concurrently (from mount and onStatsUpdate) causing stale responses to overwrite newer ones; modify fetchData/useEffect to cancel or ignore prior in-flight requests by introducing an AbortController (passed to window.maestro.stats.getAutoRunSessions if supported) or a request-id/isCurrent ref: increment a latestFetchId ref before each fetch and capture its value locally, then only call setSessions/setLoading if the captured id matches the latest; ensure the cleanup returned from useEffect also aborts or marks the current fetch stale and wire this into the onStatsUpdate handler so each new invocation invalidates previous in-flight work.src/renderer/components/UsageDashboard/WeekdayComparisonChart.tsx (2)
301-303:⚠️ Potential issue | 🟡 MinorMisleading fallback copy after the weekend-guard change.
After the new
weekendAvg > 0guard, theelsebranch is now only reached whenweekendAvgQueriesPerDay === 0(weekend days exist in the dataset but no queries were recorded) — or, after the fix above, when either average is zero. "Similar activity on weekdays and weekends" is inaccurate in both cases; something like "Insufficient data to compare weekday and weekend activity" or "No weekend activity recorded in this period" would be more precise.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/UsageDashboard/WeekdayComparisonChart.tsx` around lines 301 - 303, The fallback text in WeekdayComparisonChart is misleading after adding the weekendAvg > 0 guard: update the else branch that currently renders "Similar activity on weekdays and weekends" to show a precise message based on the actual condition (e.g., "Insufficient data to compare weekday and weekend activity" or "No weekend activity recorded in this period"). Locate the rendering logic in the WeekdayComparisonChart component where weekendAvg and weekendAvgQueriesPerDay are used and replace the generic copy with conditional copy that reflects zero averages or no weekend queries.
287-300:⚠️ Potential issue | 🟠 MajorDivision-by-zero:
weekdayAvgQueriesPerDayis unguarded in the "more active on weekends" branch.In Branch 2 the condition only checks
weekendAvg > 0, but the denominator of the ratio expression isweekday.avgQueriesPerDay. If a period contains weekend-only activity (weekdayStats.count === 0whileweekdayStats.days > 0),weekdayAvgQueriesPerDayis0and the result isInfinity. Since(Infinity).toFixed(0)returns the string"Infinity", the UI renders "You're Infinity% more active on weekends".Branch 1 correctly guards its denominator (
weekendAvg > 0) — Branch 2 needs the symmetric guard.🐛 Proposed fix
- ) : comparisonData.weekend.avgQueriesPerDay > 0 ? ( + ) : comparisonData.weekend.avgQueriesPerDay > 0 && + comparisonData.weekday.avgQueriesPerDay > 0 ? ( <span> You're{' '} <strong style={{ color: theme.colors.textMain }}> {( (comparisonData.weekend.avgQueriesPerDay / comparisonData.weekday.avgQueriesPerDay) * 100 - 100 ).toFixed(0)} % </strong>{' '} more active on weekends </span>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/UsageDashboard/WeekdayComparisonChart.tsx` around lines 287 - 300, The "more active on weekends" JSX branch uses comparisonData.weekend.avgQueriesPerDay as a truthy guard but divides by comparisonData.weekday.avgQueriesPerDay without checking for zero; update the condition in WeekdayComparisonChart's render (the branch that currently reads comparisonData.weekend.avgQueriesPerDay > 0 ? ...) to also ensure comparisonData.weekday.avgQueriesPerDay > 0 before computing the percentage (or, if weekday avg is zero, render a fallback message/value instead of performing the division) so the division-by-zero and resulting "Infinity%" are avoided.src/renderer/components/UsageDashboard/AutoRunStats.tsx (1)
150-157:⚠️ Potential issue | 🟠 MajorPreserve attempted counts when
tasksTotalis missing.Line 150 treats missing
tasksTotalas0, which can drop valid days at Line 156 even whentasksCompletedexists (legacy/incomplete records). Use a fallback totasksCompletedto avoid undercounting.🔧 Proposed fix
- grouped[date].count += session.tasksTotal ?? 0; + const attempted = session.tasksTotal ?? session.tasksCompleted ?? 0; + grouped[date].count += attempted; grouped[date].successCount += session.tasksCompleted ?? 0;🤖 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 150 - 157, The code currently uses session.tasksTotal ?? 0 when accumulating grouped[date].count, which drops days that only have tasksCompleted recorded; update the accumulation so grouped[date].count adds session.tasksTotal if present otherwise falls back to session.tasksCompleted (e.g., grouped[date].count += session.tasksTotal ?? session.tasksCompleted ?? 0) so days with only completed counts are preserved before the .filter(entry => entry.count > 0) step; keep successCount accumulation as-is (grouped[date].successCount += session.tasksCompleted ?? 0).src/renderer/components/FileExplorerPanel.tsx (1)
1382-1391:⚠️ Potential issue | 🟡 MinorFile tree context menu should be keyboard-focusable on mount.
Line 1382 renders the menu, but focus is not moved into it.
Suggested accessibility fix
<div - ref={contextMenuRef} - className="fixed z-[10000] rounded-lg shadow-xl border overflow-hidden" + ref={(el) => { + contextMenuRef.current = el; + el?.focus(); + }} + tabIndex={-1} + className="fixed z-[10000] rounded-lg shadow-xl border overflow-hidden outline-none"As per coding guidelines: "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/FileExplorerPanel.tsx` around lines 1382 - 1391, The context menu element rendered in FileExplorerPanel isn't keyboard-focusable on mount; update the rendered element (where ref={contextMenuRef} and style uses contextMenuPos) to include tabIndex={-1} and the 'outline-none' class and change the ref to a callback ref that assigns to contextMenuRef.current and calls el?.focus() only when contextMenuPos.ready is true (so the menu is focused when it becomes visible). This uses the existing contextMenuRef and contextMenuPos symbols and ensures the menu receives keyboard focus on mount/visibility.src/renderer/components/DocumentGraph/NodeContextMenu.tsx (1)
119-126:⚠️ Potential issue | 🟡 MinorContext menu should take focus when it opens.
Line 119 renders an interactive context menu, but it does not receive focus, which hurts keyboard usability.
Suggested accessibility fix
<div - ref={menuRef} - className="fixed z-[10000] py-1 rounded-md shadow-xl border" + ref={(el) => { + menuRef.current = el; + el?.focus(); + }} + tabIndex={-1} + className="fixed z-[10000] py-1 rounded-md shadow-xl border outline-none"As per coding guidelines: "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/DocumentGraph/NodeContextMenu.tsx` around lines 119 - 126, The context menu rendered by NodeContextMenu doesn't receive keyboard focus; update the root div (ref=menuRef) to be focusable by adding tabIndex={-1} and the outline-none class, and programmatically focus it when it opens by calling menuRef.current?.focus() (e.g., in a useEffect that watches ready). Ensure you keep the existing ref (menuRef) and ready flag logic so focus only occurs when the menu is visible.
🧹 Nitpick comments (12)
src/renderer/hooks/agent/useFilteredAndSortedSessions.ts (1)
122-127: Make invalid-date handling deterministic in the comparator.If
modifiedAtis malformed,getTime()yieldsNaN, which can produce unstable sort ordering. Normalize invalid timestamps once and use a numeric fallback in sort.Proposed refactor
const sessionModifiedTimestamps = useMemo(() => { const timestamps = new Map<string, number>(); for (const session of sessions) { - timestamps.set(session.sessionId, new Date(session.modifiedAt).getTime()); + const ts = Date.parse(session.modifiedAt); + timestamps.set(session.sessionId, Number.isFinite(ts) ? ts : 0); } return timestamps; }, [sessions]); @@ - const aModifiedAt = - sessionModifiedTimestamps.get(a.sessionId) ?? new Date(a.modifiedAt).getTime(); - const bModifiedAt = - sessionModifiedTimestamps.get(b.sessionId) ?? new Date(b.modifiedAt).getTime(); + const aModifiedAt = sessionModifiedTimestamps.get(a.sessionId) ?? 0; + const bModifiedAt = sessionModifiedTimestamps.get(b.sessionId) ?? 0; return bModifiedAt - aModifiedAt;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/hooks/agent/useFilteredAndSortedSessions.ts` around lines 122 - 127, The comparator in useFilteredAndSortedSessions.ts can return NaN when a.modifiedAt or b.modifiedAt is malformed; make timestamp normalization deterministic by converting cached values or Date.parse(a.modifiedAt)/Date.parse(b.modifiedAt) to numbers and replacing non-finite results with a numeric fallback (e.g., 0) before subtraction. Locate the sort comparator that uses sessionModifiedTimestamps.get(... ) ?? new Date(...).getTime() and replace it with a safe-normalize step (e.g., const aTs = Number.isFinite(+val) ? +val : 0) for both a and b so the comparator always returns a finite number.src/renderer/components/Toast.tsx (1)
291-296:<style>tag duplicated per toast instance — consider hoisting.Each
ToastItemrenders its own<style>block containing the identical@keyframes shrinkrule. With multiple concurrent toasts this injects duplicate rules into the DOM on every render. This is pre-existing, but now thatToastItemis memoised it is a good time to hoist the keyframes to a single global stylesheet or a CSS module.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/Toast.tsx` around lines 291 - 296, The ToastItem component is injecting a duplicate <style> with the same `@keyframes` shrink per instance (see ToastItem in Toast.tsx); remove that inline <style> from the memoized ToastItem and hoist the `@keyframes` shrink into a single global stylesheet or a Toast CSS module (e.g., app.css or toast.module.css), then update the ToastItem to use the existing CSS class/animation name (shrink) instead of injecting styles. Ensure the animation class used by ToastItem matches the hoisted keyframes and that no other component still injects the same rule.src/renderer/components/ConfirmModal.tsx (1)
42-50: Remove explicitzIndex={10000}or derive it frompriority.
priority={MODAL_PRIORITIES.CONFIRM}(1000) is used only for layer stack management (Escape handling, focus), whilezIndex={10000}is a separate CSS property. The mismatch between priority (1000) and zIndex (10000) is confusing and suggests incomplete implementation. Either have Modal compute zIndex from priority for consistency, or remove zIndex and let the priority-based system handle visual stacking entirely.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/ConfirmModal.tsx` around lines 42 - 50, The Modal instance is passing an explicit zIndex={10000} that conflicts with the priority-based stacking (priority={MODAL_PRIORITIES.CONFIRM}); remove the hardcoded zIndex prop from this Modal usage and either let the Modal component compute its z-index from the provided priority (MODAL_PRIORITIES.CONFIRM) or, if you prefer visual override, add logic inside Modal to derive zIndex from priority (e.g., a mapping in Modal's render based on priority) so callers like the ConfirmModal (initialFocusRef={confirmButtonRef}, headerIcon, title) don't need to pass raw zIndex values.src/__tests__/renderer/components/QRCode.test.tsx (2)
22-24: Simplify the sentry mock — the wrapper function is unnecessary.
(...args: unknown[]) => mockCaptureException(...args)is functionally equivalent to assigning the mock directly, and the extra indirection serves no purpose here.♻️ Proposed simplification
-vi.mock('../../../renderer/utils/sentry', () => ({ - captureException: (...args: unknown[]) => mockCaptureException(...args), -})); +vi.mock('../../../renderer/utils/sentry', () => ({ + captureException: mockCaptureException, +}));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/renderer/components/QRCode.test.tsx` around lines 22 - 24, The vi.mock for '../../../renderer/utils/sentry' uses an unnecessary wrapper around the mock; replace the wrapper function in the mock declaration with a direct reference to the mock implementation by exporting captureException: mockCaptureException so the mockCaptureException is used directly (update the vi.mock call that currently returns captureException: (...args: unknown[]) => mockCaptureException(...args) to export captureException: mockCaptureException).
275-275: Remove staleconsole.errorspies — the component no longer usesconsole.error.These three
vi.spyOn(console, 'error').mockImplementation(() => {})calls are leftovers from the previous implementation. The component now callscaptureExceptioninstead ofconsole.error, so the spies are dead code. More critically,vi.clearAllMocks()inafterEachresets call counts but does not restore spy implementations — onlyvi.restoreAllMocks()does. Without restoration, the suppressedconsole.errorleaks into subsequent tests run in the same worker, silently hiding unexpected errors.♻️ Proposed cleanup
it('should apply error styling', async () => { mockToDataURL.mockRejectedValue(new Error('Generation failed')); - vi.spyOn(console, 'error').mockImplementation(() => {}); render(<QRCode value="https://example.com" size={150} />);it('should include custom className in error state', async () => { mockToDataURL.mockRejectedValue(new Error('Generation failed')); - vi.spyOn(console, 'error').mockImplementation(() => {}); render(<QRCode value="https://example.com" className="my-error-class" />);it('should clear error when value changes to valid', async () => { - vi.spyOn(console, 'error').mockImplementation(() => {}); mockToDataURL.mockRejectedValueOnce(new Error('Generation failed'));Also applies to: 287-287, 298-298
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/renderer/components/QRCode.test.tsx` at line 275, Remove the stale console.error spies (the vi.spyOn(console, 'error').mockImplementation(() => {}) calls) from the QRCode.test.tsx tests — the component now uses captureException so these spies are dead code — and either replace vi.clearAllMocks() in the afterEach hook with vi.restoreAllMocks() or keep clearAllMocks() but remove the console.error spies so no suppression leaks into other tests; look for the vi.spyOn(console, 'error'), afterEach, vi.clearAllMocks(), and consider vi.restoreAllMocks() to ensure proper restoration if any future spies are added.src/renderer/components/QRCode.tsx (1)
58-61:captureExceptionis called without context — consider adding identifying metadata.The catch block passes
errbare. Per project guidelines,captureExceptionshould be called "with context." Adding a scope tag or extra data (e.g. thevaluebeing encoded, component name) makes the Sentry event actionable without requiring a reproduction.♻️ Suggested improvement
- captureException(err); + captureException(err, { extra: { value, size } });🤖 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 58 - 61, The catch handler in the QRCode component calls captureException(err) without context; update the catch block that currently calls captureException to pass identifying metadata (e.g., tags: { component: 'QRCode' } and extra: { value: value }) or equivalent scope/extra data so Sentry events include the encoded value and component name, then keep the existing setError and setDataUrl calls unchanged.src/renderer/components/UsageDashboard/LongestAutoRunsTable.tsx (1)
185-185: RedundantMath.min—topSessionsis already bounded byMAX_ROWS.
topSessionsis produced by.slice(0, MAX_ROWS)(line 150), sotopSessions.lengthis always≤ MAX_ROWS. TheMath.mincall is a no-op and adds noise.♻️ Proposed simplification
- Top {Math.min(topSessions.length, MAX_ROWS)} Longest Auto Runs + Top {topSessions.length} Longest Auto Runs🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/UsageDashboard/LongestAutoRunsTable.tsx` at line 185, The heading uses a redundant Math.min because topSessions is already sliced to MAX_ROWS; replace "Top {Math.min(topSessions.length, MAX_ROWS)} Longest Auto Runs" with a simpler expression using topSessions.length (e.g., "Top {topSessions.length} Longest Auto Runs") in the LongestAutoRunsTable component to remove the no-op Math.min call.src/main/stats/schema.ts (1)
52-61: Compound indexes are duplicated betweenCREATE_QUERY_EVENTS_INDEXES_SQLandCREATE_COMPOUND_INDEXES_SQL.The three compound indexes added at Lines 58–60 are identical to those exported in
CREATE_COMPOUND_INDEXES_SQL(Lines 135–137). On fresh installs, migration v1 already creates them, so migration v4 becomes a no-op (harmless due toIF NOT EXISTS, but wasteful and misleading).Consider removing the compound indexes from
CREATE_QUERY_EVENTS_INDEXES_SQLand letting migration v4 be the single source of truth for these definitions. Alternatively, add a comment acknowledging the intentional duplication so future contributors aren't confused.♻️ Option: remove duplicates from v1 index set
export const CREATE_QUERY_EVENTS_INDEXES_SQL = ` CREATE INDEX IF NOT EXISTS idx_query_start_time ON query_events(start_time); CREATE INDEX IF NOT EXISTS idx_query_agent_type ON query_events(agent_type); CREATE INDEX IF NOT EXISTS idx_query_source ON query_events(source); CREATE INDEX IF NOT EXISTS idx_query_session ON query_events(session_id); - CREATE INDEX IF NOT EXISTS idx_query_project_path ON query_events(project_path); - CREATE INDEX IF NOT EXISTS idx_query_time_agent ON query_events(start_time, agent_type); - CREATE INDEX IF NOT EXISTS idx_query_time_project ON query_events(start_time, project_path); - CREATE INDEX IF NOT EXISTS idx_query_time_source ON query_events(start_time, source) + CREATE INDEX IF NOT EXISTS idx_query_project_path ON query_events(project_path) `;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/stats/schema.ts` around lines 52 - 61, The CREATE_QUERY_EVENTS_INDEXES_SQL constant currently re-declares three compound indexes that are already defined in CREATE_COMPOUND_INDEXES_SQL; remove the duplicate compound index lines (the idx_query_time_agent, idx_query_time_project, idx_query_time_source definitions) from CREATE_QUERY_EVENTS_INDEXES_SQL so migration v1 only contains simple indexes and migration v4 remains the single source of truth for compound indexes (or if you prefer to keep them, add a clear inline comment next to CREATE_QUERY_EVENTS_INDEXES_SQL explaining the intentional duplication and referencing CREATE_COMPOUND_INDEXES_SQL).src/renderer/hooks/session/useNavigationHistory.ts (1)
4-8: StrengthenNavHistoryEntryto disallow empty navigation targets.Line 4 currently allows
{}because bothsessionIdandgroupChatIdare optional. That makes it easy to push a non-navigable entry.Proposed type-tightening diff
-export interface NavHistoryEntry { - sessionId?: string; - tabId?: string; // Optional AI tab ID (only for AI mode with tabs) - groupChatId?: string; // Set when navigating to a group chat -} +export type NavHistoryEntry = + | { + sessionId: string; + tabId?: string; // Optional AI tab ID (only for AI mode with tabs) + groupChatId?: never; + } + | { + groupChatId: string; // Set when navigating to a group chat + sessionId?: never; + tabId?: never; + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/hooks/session/useNavigationHistory.ts` around lines 4 - 8, NavHistoryEntry currently allows an empty object because all fields are optional; change the type so an entry must target something by making it a union/discriminated type that requires either a session target or a group chat target (and keep tabId allowed only with a session); update the NavHistoryEntry declaration (and any callers in useNavigationHistory) to use something like a union of { sessionId: string; tabId?: string } | { groupChatId: string } so you cannot push a completely empty navigation entry.src/renderer/components/GroupChatList.tsx (1)
60-67: Align new interactive controls with the repo’s focus pattern.The new menu container/buttons should use the explicit focus conventions (
tabIndex,outline-none, and popup auto-focus) used in this codebase.💡 Suggested pattern update
- <div - ref={menuRef} - className="fixed z-50 py-1 rounded-md shadow-xl border" + <div + ref={(el) => { + menuRef.current = el; + el?.focus(); + }} + tabIndex={-1} + className="fixed z-50 py-1 rounded-md shadow-xl border outline-none" style={{ left, top, opacity: ready ? 1 : 0, @@ - <button + <button + tabIndex={0} onClick={() => { onArchive(); onClose(); }} - className="w-full text-left px-3 py-1.5 text-xs hover:bg-white/5 transition-colors flex items-center gap-2" + className="w-full text-left px-3 py-1.5 text-xs hover:bg-white/5 transition-colors flex items-center gap-2 outline-none" style={{ color: theme.colors.textMain }} > @@ - <button + <button + tabIndex={0} onClick={(e) => { e.stopPropagation(); setShowArchived(!showArchived); }} - className="px-2 py-0.5 rounded-full text-[10px] font-medium hover:opacity-80 transition-opacity flex items-center gap-1" + className="px-2 py-0.5 rounded-full text-[10px] font-medium hover:opacity-80 transition-opacity flex items-center gap-1 outline-none" @@ - <button + <button + tabIndex={0} onClick={(e) => { e.stopPropagation(); onNewGroupChat(); }} - className="px-2 py-0.5 rounded-full text-[10px] font-medium hover:opacity-80 transition-opacity flex items-center gap-1" + className="px-2 py-0.5 rounded-full text-[10px] font-medium hover:opacity-80 transition-opacity flex items-center gap-1 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."Also applies to: 95-109, 257-288
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/GroupChatList.tsx` around lines 60 - 67, The popup menu container in GroupChatList (the element using menuRef) must follow the repo focus pattern: add tabIndex={-1} or tabIndex={0} and include the "outline-none" class, and apply auto-focus by using ref={(el) => { menuRef.current = el; el?.focus(); }} (or inline ref callback) so the menu receives focus when opened; update the same pattern for the other popup/menu elements referenced around the diffs (the blocks at 95-109 and 257-288) to ensure consistent keyboard focus behavior across the component.src/renderer/global.d.ts (1)
158-198: Prefer aliasing the sharedGroupChattype over re-declaring it here.
GroupChatDatanow duplicates a large shared shape and can drift. Importing/aliasing the shared type would keep API contracts in sync.💡 Suggested refactor
+import type { GroupChat as SharedGroupChat } from '../shared/group-chat-types'; @@ -type GroupChatData = { - ... -}; +type GroupChatData = SharedGroupChat;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/global.d.ts` around lines 158 - 198, Replace the duplicated local type GroupChatData with an alias to the shared GroupChat type: remove the full re-declaration of GroupChatData and import/alias the single source-of-truth GroupChat (e.g., using an import type and then `type GroupChatData = GroupChat`) so the renderer global declaration references the shared GroupChat symbol instead of duplicating its shape; update any local references to continue using GroupChatData (now an alias) and remove the old nested type object to avoid drift.src/renderer/components/UsageDashboard/ChartSkeletons.tsx (1)
47-57: Consider clamping dynamic column counts before rendering.If
columnsis ever<= 0or non-integer from responsive math, grid/template behavior can degrade unexpectedly. A small defensive clamp would make these skeletons safer.🔧 Optional hardening
export const SummaryCardsSkeleton = memo(function SummaryCardsSkeleton({ theme, columns = 5 }: SkeletonProps & { columns?: number }) { + const safeColumns = Math.max(1, Math.floor(columns)); return ( <div className="grid gap-4" style={{ - gridTemplateColumns: `repeat(${columns}, minmax(0, 1fr))`, + gridTemplateColumns: `repeat(${safeColumns}, minmax(0, 1fr))`, }} data-testid="summary-cards-skeleton" > - {Array.from({ length: columns }).map((_, i) => ( + {Array.from({ length: safeColumns }).map((_, i) => (export const AutoRunStatsSkeleton = memo(function AutoRunStatsSkeleton({ theme, columns = 6 }: SkeletonProps & { columns?: number }) { + const safeColumns = Math.max(1, Math.floor(columns)); return ( <div className="grid gap-4" style={{ - gridTemplateColumns: `repeat(${columns}, minmax(0, 1fr))`, + gridTemplateColumns: `repeat(${safeColumns}, minmax(0, 1fr))`, }} data-testid="autorun-stats-skeleton" > - {Array.from({ length: columns }).map((_, i) => ( + {Array.from({ length: safeColumns }).map((_, i) => (Also applies to: 323-333
🤖 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 - 57, SummaryCardsSkeleton (and the other skeleton at lines 323-333) should defensively clamp and normalize the incoming columns prop before using it in gridTemplateColumns and Array.from: compute a safeColumns value such as const safeColumns = Math.max(1, Math.floor(Number(columns) || 0)) (or otherwise ensure it's a finite positive integer), then use safeColumns in the template string and in Array.from({ length: safeColumns }) so non-integer, zero, negative or non-finite inputs cannot break the layout.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/__tests__/renderer/components/DocumentGraph/NodeContextMenu.test.tsx`:
- Around line 126-149: The test overrides global viewport sizes by setting
window.innerWidth/innerHeight but never restores them; save the original values
before setting (e.g., const originalInnerWidth = window.innerWidth; const
originalInnerHeight = window.innerHeight) and restore them in the finally block
alongside restoring Element.prototype.getBoundingClientRect; update the test
surrounding createProps and render(<NodeContextMenu {...props} />) to restore
originalInnerWidth and originalInnerHeight to avoid cross-test leakage.
In `@src/__tests__/renderer/hooks/useRemoteIntegration.test.ts`:
- Around line 343-362: The test currently uses createMockSession with
activeFileTabId: null so it can't catch regressions; change the session setup in
the test for useRemoteIntegration to initialize activeFileTabId to a non-null
value (e.g., 'tab-1') via createMockSession({ id: 'session-1', inputMode:
'terminal', activeFileTabId: 'tab-1' }), then invoke
onRemoteSwitchModeHandler('session-1','ai') and assert via the deps.setSessions
updater that result[0].inputMode === 'ai' and result[0].activeFileTabId ===
'tab-1' to verify preservation (references: useRemoteIntegration,
onRemoteSwitchModeHandler, createMockSession, deps.setSessions).
In `@src/renderer/components/GroupChatHistoryPanel.tsx`:
- Around line 230-236: The context menu element rendered in
GroupChatHistoryPanel is not keyboard-focusable; update the element with
ref={contextMenuRef} to include tabIndex={-1} (or tabIndex={0}) and the
outline-none class, and ensure it is auto-focused when opened by either changing
the ref to a focus-callback (e.g. ref={(el) => { contextMenuRef.current = el; if
(contextMenuPos.ready) el?.focus(); }}) or by calling
contextMenuRef.current?.focus() inside a useEffect that watches
contextMenuPos.ready; keep references to contextMenuRef, contextMenuPos, and the
component GroupChatHistoryPanel when making the change.
In `@src/renderer/components/GroupChatInput.tsx`:
- Around line 262-279: The mention insertion can attempt to access
filteredMentions[selectedMentionIndex] after the list changes, causing undefined
to be passed into insertMention; update the keyboard handler where it handles
Tab/Enter (the block using showMentions, filteredMentions, selectedMentionIndex
and calling insertMention) to first check filteredMentions.length > 0 and
clamp/normalize selectedMentionIndex into the valid range (e.g., use
Math.min(selectedMentionIndex, filteredMentions.length - 1) or compute a safe
index) before reading filteredMentions[safeIndex]; if no valid mention exists,
bail out without calling insertMention and optionally reset
selectedMentionIndex.
In `@src/renderer/components/NewInstanceModal.tsx`:
- Around line 1257-1291: The effect in useEffect that loads agent info and
models can have stale async responses overwrite current state; prevent this by
introducing a per-run cancellation token (e.g., a local runId or an
AbortController) inside the effect and capture it before starting any async call
(for window.maestro.agents.detect, getModels and getConfig), then check the
token is still valid before calling setAgent, setAvailableModels, setAgentConfig
(or abort the fetches if the maestro API supports AbortSignal); ensure you
invalidate the token in the effect cleanup so older promises do not update
agent, availableModels, or agentConfig for a newly selectedToolType or closed
modal.
In `@src/renderer/components/SessionActivityGraph.tsx`:
- Around line 235-240: The positioned context menu container
(ref={contextMenuRef} with style using contextMenuPos) must be made focusable
and auto-focused when opened: add tabIndex={-1} (or 0) and include the
outline-none class in the className, and change the ref assignment to a callback
that assigns the element to contextMenuRef and calls el?.focus() when
contextMenuPos.ready is true so the menu receives keyboard focus on open; target
the contextMenuRef and contextMenuPos usage in SessionActivityGraph.tsx to
implement this.
In `@src/renderer/components/SymphonyModal.tsx`:
- Around line 306-307: The current isSelectable = isAvailable || isBlocked
allows non-available items with a blocking label to appear selectable; change
the logic to only allow selection when the item is available and not blocked by
replacing the expression with isSelectable = isAvailable && !isBlocked (update
the assignment where isSelectable is defined in SymphonyModal.tsx and verify any
downstream click/disabled behavior uses the updated value).
In `@src/renderer/components/UsageDashboard/AutoRunStats.tsx`:
- Line 438: The focusable bar element in AutoRunStats.tsx currently sets
tabIndex={0} but lacks the outline-none class; update the element (the JSX node
with tabIndex={0} in the AutoRunStats component) to include "outline-none" in
its className list so the keyboard focus style follows project guidelines,
ensuring you preserve existing classes and conditional logic when adding
outline-none.
- Around line 425-435: The keyboard handler toggles only hoveredBar so the
tooltip never appears because tooltipPos is never set; update the onKeyDown
handler in AutoRunStats (the block that currently calls setHoveredBar and
handleMouseLeave) to also set tooltipPos when Enter/Space is pressed (compute it
from the event target's boundingClientRect or from the same position logic used
by mouse handlers) and clear tooltipPos on Escape or blur so the tooltip renders
correctly; reference setHoveredBar, setTooltipPos, tooltipPos, handleMouseLeave
and the element with data-testid `task-bar-${day.date}` when making the change.
- Line 437: The aria-label in AutoRunStats (the JSX using
formatFullDate(day.date), day.count and day.successCount) uses "tasks attempted"
which conflicts with the chart title/tooltip; update that aria-label string to
use the same wording as the visible chart (e.g., "Tasks Completed") and keep the
existing interpolation and percentage calculation intact so the label becomes:
"<date>: <count> Tasks Completed, <successCount> successful (<percentage>%)".
In `@src/renderer/components/WorktreeConfigModal.tsx`:
- Line 180: WorktreeConfigModal currently hardcodes z-[10000], bypassing the
layer stack; update the component (WorktreeConfigModal) to either wrap its
content with the reusable Modal component (same pattern as ConfirmModal) so
z-index is managed by the layer system, or compute the inline z style from
MODAL_PRIORITIES.WORKTREE_CONFIG (752) via the central layer stack helper and
replace the hardcoded class on the div (the fixed inset-0 container) so z-index
honors the layer-priority system.
In `@src/renderer/hooks/groupChat/useGroupChatHandlers.ts`:
- Around line 450-457: handleArchiveGroupChat captures activeGroupChatId before
the await, causing a stale check after window.maestro.groupChat.archive
completes; to fix, remove the pre-await snapshot and instead read the current
activeGroupChatId from useGroupChatStore.getState() after the await (i.e., after
calling window.maestro.groupChat.archive and updating setGroupChats) and only
then call handleCloseGroupChat() if the freshly read activeGroupChatId equals id
and archived is true; keep existing references to setGroupChats and
window.maestro.groupChat.archive and preserve the async flow in
handleArchiveGroupChat.
In `@src/renderer/hooks/remote/useRemoteIntegration.ts`:
- Around line 158-164: In onRemoteCommand in useRemoteIntegration.ts, mirror the
terminal cleanup done in onRemoteSwitchMode by clearing activeFileTabId whenever
inputMode is set to 'terminal'; update the state update in onRemoteCommand to
return { ...s, inputMode: 'terminal', ...( 'terminal' === 'terminal' && {
activeFileTabId: null }) } (i.e., use the same conditional spread pattern or
equivalent) so that setting inputMode to 'terminal' via onRemoteCommand also
nulls activeFileTabId and keeps state consistent.
In `@src/renderer/hooks/session/useSessionLifecycle.ts`:
- Around line 179-182: Replace the silent catch around
window.maestro.process.kill(`${sessionId}-ai`) in useSessionLifecycle.ts so only
“process not found” errors are ignored; import captureException from
'src/utils/sentry' and in the catch inspect the error (e.g., error.code or
message) and if it’s not a known not-found case call captureException(error, {
extra: { sessionId, action: 'kill-ai-process' } }) and rethrow (or allow the
error to propagate) so unexpected failures are reported to Sentry rather than
being swallowed.
In `@src/renderer/hooks/session/useSessionNavigation.ts`:
- Around line 69-73: When handling a group chat entry in useSessionNavigation
(the branch that checks entry.groupChatId and calls onNavigateToGroupChat),
reset the navigation cycle state before returning: set cyclePositionRef.current
(and any related cycle state like cycleIndexRef or similar) back to its initial
value so cycle navigation does not remain stale after navigating into a group
chat; update the group-chat branch (the if (entry.groupChatId) { ... }) to reset
cyclePositionRef.current and then call
onNavigateToGroupChat?.(entry.groupChatId) and return.
In `@src/renderer/hooks/stats/useStats.ts`:
- Around line 138-144: In useStats, when enabled is false the effect skips
calling fetchStats and subscribing but leaves the loading state true; update the
effect so that when enabled is false you explicitly set loading to false (e.g.,
call the same state setter used by fetchStats) and avoid creating a
subscription; ensure the unsubscribe variable and cleanup logic around
window.maestro.stats.onStatsUpdate and debouncedUpdate remain intact so toggling
enabled on/off always clears or sets loading correctly.
---
Outside diff comments:
In `@src/renderer/components/DocumentGraph/NodeContextMenu.tsx`:
- Around line 119-126: The context menu rendered by NodeContextMenu doesn't
receive keyboard focus; update the root div (ref=menuRef) to be focusable by
adding tabIndex={-1} and the outline-none class, and programmatically focus it
when it opens by calling menuRef.current?.focus() (e.g., in a useEffect that
watches ready). Ensure you keep the existing ref (menuRef) and ready flag logic
so focus only occurs when the menu is visible.
In `@src/renderer/components/FileExplorerPanel.tsx`:
- Around line 1382-1391: The context menu element rendered in FileExplorerPanel
isn't keyboard-focusable on mount; update the rendered element (where
ref={contextMenuRef} and style uses contextMenuPos) to include tabIndex={-1} and
the 'outline-none' class and change the ref to a callback ref that assigns to
contextMenuRef.current and calls el?.focus() only when contextMenuPos.ready is
true (so the menu is focused when it becomes visible). This uses the existing
contextMenuRef and contextMenuPos symbols and ensures the menu receives keyboard
focus on mount/visibility.
In `@src/renderer/components/QRCode.tsx`:
- Around line 38-62: The useEffect early-return path doesn't clear prior errors,
so when value is falsy the component still shows a stale error; update the
useEffect so that when !value you call setError(null) in addition to
setDataUrl(null) (same block that returns), ensuring any previous error from
QRCodeLib.toDataURL (and captureException) is cleared; locate the effect using
useEffect and the handlers setDataUrl/setError and QRCodeLib.toDataURL to apply
the change.
In `@src/renderer/components/Toast.tsx`:
- Around line 263-266: The close button inside the Toast component needs
accessibility and form-safety attributes: update the button rendered where
onClick={handleClose} (the icon-only close control) to include type="button" and
an appropriate aria-label (e.g., aria-label="Close" or aria-label prop passed
down) so screen readers can identify it; ensure this change is applied where the
close SVG button is rendered in Toast.tsx alongside the existing className and
style props.
In `@src/renderer/components/UsageDashboard/AutoRunStats.tsx`:
- Around line 150-157: The code currently uses session.tasksTotal ?? 0 when
accumulating grouped[date].count, which drops days that only have tasksCompleted
recorded; update the accumulation so grouped[date].count adds session.tasksTotal
if present otherwise falls back to session.tasksCompleted (e.g.,
grouped[date].count += session.tasksTotal ?? session.tasksCompleted ?? 0) so
days with only completed counts are preserved before the .filter(entry =>
entry.count > 0) step; keep successCount accumulation as-is
(grouped[date].successCount += session.tasksCompleted ?? 0).
In `@src/renderer/components/UsageDashboard/LongestAutoRunsTable.tsx`:
- Around line 126-146: fetchData can be called concurrently (from mount and
onStatsUpdate) causing stale responses to overwrite newer ones; modify
fetchData/useEffect to cancel or ignore prior in-flight requests by introducing
an AbortController (passed to window.maestro.stats.getAutoRunSessions if
supported) or a request-id/isCurrent ref: increment a latestFetchId ref before
each fetch and capture its value locally, then only call setSessions/setLoading
if the captured id matches the latest; ensure the cleanup returned from
useEffect also aborts or marks the current fetch stale and wire this into the
onStatsUpdate handler so each new invocation invalidates previous in-flight
work.
In `@src/renderer/components/UsageDashboard/WeekdayComparisonChart.tsx`:
- Around line 301-303: The fallback text in WeekdayComparisonChart is misleading
after adding the weekendAvg > 0 guard: update the else branch that currently
renders "Similar activity on weekdays and weekends" to show a precise message
based on the actual condition (e.g., "Insufficient data to compare weekday and
weekend activity" or "No weekend activity recorded in this period"). Locate the
rendering logic in the WeekdayComparisonChart component where weekendAvg and
weekendAvgQueriesPerDay are used and replace the generic copy with conditional
copy that reflects zero averages or no weekend queries.
- Around line 287-300: The "more active on weekends" JSX branch uses
comparisonData.weekend.avgQueriesPerDay as a truthy guard but divides by
comparisonData.weekday.avgQueriesPerDay without checking for zero; update the
condition in WeekdayComparisonChart's render (the branch that currently reads
comparisonData.weekend.avgQueriesPerDay > 0 ? ...) to also ensure
comparisonData.weekday.avgQueriesPerDay > 0 before computing the percentage (or,
if weekday avg is zero, render a fallback message/value instead of performing
the division) so the division-by-zero and resulting "Infinity%" are avoided.
---
Nitpick comments:
In `@src/__tests__/renderer/components/QRCode.test.tsx`:
- Around line 22-24: The vi.mock for '../../../renderer/utils/sentry' uses an
unnecessary wrapper around the mock; replace the wrapper function in the mock
declaration with a direct reference to the mock implementation by exporting
captureException: mockCaptureException so the mockCaptureException is used
directly (update the vi.mock call that currently returns captureException:
(...args: unknown[]) => mockCaptureException(...args) to export
captureException: mockCaptureException).
- Line 275: Remove the stale console.error spies (the vi.spyOn(console,
'error').mockImplementation(() => {}) calls) from the QRCode.test.tsx tests —
the component now uses captureException so these spies are dead code — and
either replace vi.clearAllMocks() in the afterEach hook with
vi.restoreAllMocks() or keep clearAllMocks() but remove the console.error spies
so no suppression leaks into other tests; look for the vi.spyOn(console,
'error'), afterEach, vi.clearAllMocks(), and consider vi.restoreAllMocks() to
ensure proper restoration if any future spies are added.
In `@src/main/stats/schema.ts`:
- Around line 52-61: The CREATE_QUERY_EVENTS_INDEXES_SQL constant currently
re-declares three compound indexes that are already defined in
CREATE_COMPOUND_INDEXES_SQL; remove the duplicate compound index lines (the
idx_query_time_agent, idx_query_time_project, idx_query_time_source definitions)
from CREATE_QUERY_EVENTS_INDEXES_SQL so migration v1 only contains simple
indexes and migration v4 remains the single source of truth for compound indexes
(or if you prefer to keep them, add a clear inline comment next to
CREATE_QUERY_EVENTS_INDEXES_SQL explaining the intentional duplication and
referencing CREATE_COMPOUND_INDEXES_SQL).
In `@src/renderer/components/ConfirmModal.tsx`:
- Around line 42-50: The Modal instance is passing an explicit zIndex={10000}
that conflicts with the priority-based stacking
(priority={MODAL_PRIORITIES.CONFIRM}); remove the hardcoded zIndex prop from
this Modal usage and either let the Modal component compute its z-index from the
provided priority (MODAL_PRIORITIES.CONFIRM) or, if you prefer visual override,
add logic inside Modal to derive zIndex from priority (e.g., a mapping in
Modal's render based on priority) so callers like the ConfirmModal
(initialFocusRef={confirmButtonRef}, headerIcon, title) don't need to pass raw
zIndex values.
In `@src/renderer/components/GroupChatList.tsx`:
- Around line 60-67: The popup menu container in GroupChatList (the element
using menuRef) must follow the repo focus pattern: add tabIndex={-1} or
tabIndex={0} and include the "outline-none" class, and apply auto-focus by using
ref={(el) => { menuRef.current = el; el?.focus(); }} (or inline ref callback) so
the menu receives focus when opened; update the same pattern for the other
popup/menu elements referenced around the diffs (the blocks at 95-109 and
257-288) to ensure consistent keyboard focus behavior across the component.
In `@src/renderer/components/QRCode.tsx`:
- Around line 58-61: The catch handler in the QRCode component calls
captureException(err) without context; update the catch block that currently
calls captureException to pass identifying metadata (e.g., tags: { component:
'QRCode' } and extra: { value: value }) or equivalent scope/extra data so Sentry
events include the encoded value and component name, then keep the existing
setError and setDataUrl calls unchanged.
In `@src/renderer/components/Toast.tsx`:
- Around line 291-296: The ToastItem component is injecting a duplicate <style>
with the same `@keyframes` shrink per instance (see ToastItem in Toast.tsx);
remove that inline <style> from the memoized ToastItem and hoist the `@keyframes`
shrink into a single global stylesheet or a Toast CSS module (e.g., app.css or
toast.module.css), then update the ToastItem to use the existing CSS
class/animation name (shrink) instead of injecting styles. Ensure the animation
class used by ToastItem matches the hoisted keyframes and that no other
component still injects the same rule.
In `@src/renderer/components/UsageDashboard/ChartSkeletons.tsx`:
- Around line 47-57: SummaryCardsSkeleton (and the other skeleton at lines
323-333) should defensively clamp and normalize the incoming columns prop before
using it in gridTemplateColumns and Array.from: compute a safeColumns value such
as const safeColumns = Math.max(1, Math.floor(Number(columns) || 0)) (or
otherwise ensure it's a finite positive integer), then use safeColumns in the
template string and in Array.from({ length: safeColumns }) so non-integer, zero,
negative or non-finite inputs cannot break the layout.
In `@src/renderer/components/UsageDashboard/LongestAutoRunsTable.tsx`:
- Line 185: The heading uses a redundant Math.min because topSessions is already
sliced to MAX_ROWS; replace "Top {Math.min(topSessions.length, MAX_ROWS)}
Longest Auto Runs" with a simpler expression using topSessions.length (e.g.,
"Top {topSessions.length} Longest Auto Runs") in the LongestAutoRunsTable
component to remove the no-op Math.min call.
In `@src/renderer/global.d.ts`:
- Around line 158-198: Replace the duplicated local type GroupChatData with an
alias to the shared GroupChat type: remove the full re-declaration of
GroupChatData and import/alias the single source-of-truth GroupChat (e.g., using
an import type and then `type GroupChatData = GroupChat`) so the renderer global
declaration references the shared GroupChat symbol instead of duplicating its
shape; update any local references to continue using GroupChatData (now an
alias) and remove the old nested type object to avoid drift.
In `@src/renderer/hooks/agent/useFilteredAndSortedSessions.ts`:
- Around line 122-127: The comparator in useFilteredAndSortedSessions.ts can
return NaN when a.modifiedAt or b.modifiedAt is malformed; make timestamp
normalization deterministic by converting cached values or
Date.parse(a.modifiedAt)/Date.parse(b.modifiedAt) to numbers and replacing
non-finite results with a numeric fallback (e.g., 0) before subtraction. Locate
the sort comparator that uses sessionModifiedTimestamps.get(... ) ?? new
Date(...).getTime() and replace it with a safe-normalize step (e.g., const aTs =
Number.isFinite(+val) ? +val : 0) for both a and b so the comparator always
returns a finite number.
In `@src/renderer/hooks/session/useNavigationHistory.ts`:
- Around line 4-8: NavHistoryEntry currently allows an empty object because all
fields are optional; change the type so an entry must target something by making
it a union/discriminated type that requires either a session target or a group
chat target (and keep tabId allowed only with a session); update the
NavHistoryEntry declaration (and any callers in useNavigationHistory) to use
something like a union of { sessionId: string; tabId?: string } | { groupChatId:
string } so you cannot push a completely empty navigation entry.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (74)
src/__tests__/main/ipc/handlers/groupChat.test.tssrc/__tests__/main/ipc/handlers/process.test.tssrc/__tests__/main/stats/stats-db.test.tssrc/__tests__/renderer/components/DocumentGraph/NodeContextMenu.test.tsxsrc/__tests__/renderer/components/GroupChatInput.test.tsxsrc/__tests__/renderer/components/QRCode.test.tsxsrc/__tests__/renderer/hooks/useContextMenuPosition.test.tssrc/__tests__/renderer/hooks/useNavigationHistory.test.tssrc/__tests__/renderer/hooks/useRemoteIntegration.test.tssrc/__tests__/renderer/hooks/useSessionLifecycle.test.tssrc/__tests__/renderer/hooks/useSessionRestoration.test.tssrc/__tests__/renderer/stores/agentStore.test.tssrc/main/agents/definitions.tssrc/main/group-chat/group-chat-storage.tssrc/main/ipc/handlers/git.tssrc/main/ipc/handlers/groupChat.tssrc/main/ipc/handlers/process.tssrc/main/preload/groupChat.tssrc/main/stats/migrations.tssrc/main/stats/schema.tssrc/renderer/App.tsxsrc/renderer/components/AppModals.tsxsrc/renderer/components/BatchRunnerModal.tsxsrc/renderer/components/ConfirmModal.tsxsrc/renderer/components/ContextWarningSash.tsxsrc/renderer/components/DocumentGraph/NodeContextMenu.tsxsrc/renderer/components/FileExplorerPanel.tsxsrc/renderer/components/FilePreview.tsxsrc/renderer/components/GroupChatHistoryPanel.tsxsrc/renderer/components/GroupChatInput.tsxsrc/renderer/components/GroupChatList.tsxsrc/renderer/components/GroupChatPanel.tsxsrc/renderer/components/HistoryHelpModal.tsxsrc/renderer/components/MainPanel.tsxsrc/renderer/components/NewInstanceModal.tsxsrc/renderer/components/QRCode.tsxsrc/renderer/components/QuickActionsModal.tsxsrc/renderer/components/RenameTabModal.tsxsrc/renderer/components/SessionActivityGraph.tsxsrc/renderer/components/SessionList.tsxsrc/renderer/components/SymphonyModal.tsxsrc/renderer/components/Toast.tsxsrc/renderer/components/UsageDashboard/ActivityHeatmap.tsxsrc/renderer/components/UsageDashboard/AgentComparisonChart.tsxsrc/renderer/components/UsageDashboard/AgentEfficiencyChart.tsxsrc/renderer/components/UsageDashboard/AgentUsageChart.tsxsrc/renderer/components/UsageDashboard/AutoRunStats.tsxsrc/renderer/components/UsageDashboard/ChartSkeletons.tsxsrc/renderer/components/UsageDashboard/DurationTrendsChart.tsxsrc/renderer/components/UsageDashboard/EmptyState.tsxsrc/renderer/components/UsageDashboard/LocationDistributionChart.tsxsrc/renderer/components/UsageDashboard/LongestAutoRunsTable.tsxsrc/renderer/components/UsageDashboard/PeakHoursChart.tsxsrc/renderer/components/UsageDashboard/SessionStats.tsxsrc/renderer/components/UsageDashboard/SourceDistributionChart.tsxsrc/renderer/components/UsageDashboard/SummaryCards.tsxsrc/renderer/components/UsageDashboard/TasksByHourChart.tsxsrc/renderer/components/UsageDashboard/WeekdayComparisonChart.tsxsrc/renderer/components/WorktreeConfigModal.tsxsrc/renderer/global.d.tssrc/renderer/hooks/agent/useFilteredAndSortedSessions.tssrc/renderer/hooks/groupChat/useGroupChatHandlers.tssrc/renderer/hooks/props/useSessionListProps.tssrc/renderer/hooks/remote/useRemoteIntegration.tssrc/renderer/hooks/session/useNavigationHistory.tssrc/renderer/hooks/session/useSessionLifecycle.tssrc/renderer/hooks/session/useSessionNavigation.tssrc/renderer/hooks/session/useSessionRestoration.tssrc/renderer/hooks/stats/useStats.tssrc/renderer/hooks/ui/index.tssrc/renderer/hooks/ui/useContextMenuPosition.tssrc/renderer/stores/agentStore.tssrc/shared/group-chat-types.tssrc/shared/stats-types.ts
Comment on lines
126
to
149
| Object.defineProperty(window, 'innerWidth', { value: 800, configurable: true }); | ||
| Object.defineProperty(window, 'innerHeight', { value: 600, configurable: true }); | ||
|
|
||
| // Mock getBoundingClientRect to return realistic menu dimensions | ||
| const originalGetBCR = Element.prototype.getBoundingClientRect; | ||
| Element.prototype.getBoundingClientRect = function () { | ||
| return { width: 180, height: 150, top: 0, left: 0, right: 180, bottom: 150, x: 0, y: 0, toJSON: () => ({}) }; | ||
| }; | ||
|
|
||
| try { | ||
| // Position near bottom-right edge | ||
| const props = createProps({ x: 750, y: 550 }); | ||
| const { container } = render(<NodeContextMenu {...props} />); | ||
|
|
||
| const menu = container.firstChild as HTMLElement; | ||
| const left = parseInt(menu.style.left); | ||
| const top = parseInt(menu.style.top); | ||
|
|
||
| // Should be clamped so the menu stays within the viewport | ||
| expect(left).toBeLessThanOrEqual(800 - 180 - 8); | ||
| expect(top).toBeLessThanOrEqual(600 - 150 - 8); | ||
| } finally { | ||
| Element.prototype.getBoundingClientRect = originalGetBCR; | ||
| } |
There was a problem hiding this comment.
Restore mocked viewport dimensions to prevent cross-test leakage.
Line 126 and Line 127 override global window.innerWidth/innerHeight but never restore them.
Suggested test hardening diff
it('adjusts position to stay within viewport', () => {
+ const originalInnerWidth = window.innerWidth;
+ const originalInnerHeight = window.innerHeight;
// Set known viewport dimensions
Object.defineProperty(window, 'innerWidth', { value: 800, configurable: true });
Object.defineProperty(window, 'innerHeight', { value: 600, configurable: true });
@@
} finally {
+ Object.defineProperty(window, 'innerWidth', {
+ value: originalInnerWidth,
+ configurable: true,
+ });
+ Object.defineProperty(window, 'innerHeight', {
+ value: originalInnerHeight,
+ configurable: true,
+ });
Element.prototype.getBoundingClientRect = originalGetBCR;
}
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/__tests__/renderer/components/DocumentGraph/NodeContextMenu.test.tsx`
around lines 126 - 149, The test overrides global viewport sizes by setting
window.innerWidth/innerHeight but never restores them; save the original values
before setting (e.g., const originalInnerWidth = window.innerWidth; const
originalInnerHeight = window.innerHeight) and restore them in the finally block
alongside restoring Element.prototype.getBoundingClientRect; update the test
surrounding createProps and render(<NodeContextMenu {...props} />) to restore
originalInnerWidth and originalInnerHeight to avoid cross-test leakage.
Comment on lines
230
to
236
| ref={contextMenuRef} | ||
| className="fixed z-50 py-1 rounded border shadow-lg" | ||
| style={{ | ||
| left: contextMenu.x, | ||
| top: contextMenu.y, | ||
| left: contextMenuPos.left, | ||
| top: contextMenuPos.top, | ||
| opacity: contextMenuPos.ready ? 1 : 0, | ||
| backgroundColor: theme.colors.bgSidebar, |
There was a problem hiding this comment.
Make the lookback context menu focusable on open.
Line 230 renders an interactive menu, but it is not keyboard-focused when opened.
Suggested accessibility fix
- <div
- ref={contextMenuRef}
- className="fixed z-50 py-1 rounded border shadow-lg"
+ <div
+ ref={(el) => {
+ contextMenuRef.current = el;
+ el?.focus();
+ }}
+ tabIndex={-1}
+ className="fixed z-50 py-1 rounded border shadow-lg outline-none"As per coding guidelines: "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."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ref={contextMenuRef} | |
| className="fixed z-50 py-1 rounded border shadow-lg" | |
| style={{ | |
| left: contextMenu.x, | |
| top: contextMenu.y, | |
| left: contextMenuPos.left, | |
| top: contextMenuPos.top, | |
| opacity: contextMenuPos.ready ? 1 : 0, | |
| backgroundColor: theme.colors.bgSidebar, | |
| ref={(el) => { | |
| contextMenuRef.current = el; | |
| el?.focus(); | |
| }} | |
| tabIndex={-1} | |
| className="fixed z-50 py-1 rounded border shadow-lg outline-none" | |
| style={{ | |
| left: contextMenuPos.left, | |
| top: contextMenuPos.top, | |
| opacity: contextMenuPos.ready ? 1 : 0, | |
| backgroundColor: theme.colors.bgSidebar, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/components/GroupChatHistoryPanel.tsx` around lines 230 - 236,
The context menu element rendered in GroupChatHistoryPanel is not
keyboard-focusable; update the element with ref={contextMenuRef} to include
tabIndex={-1} (or tabIndex={0}) and the outline-none class, and ensure it is
auto-focused when opened by either changing the ref to a focus-callback (e.g.
ref={(el) => { contextMenuRef.current = el; if (contextMenuPos.ready)
el?.focus(); }}) or by calling contextMenuRef.current?.focus() inside a
useEffect that watches contextMenuPos.ready; keep references to contextMenuRef,
contextMenuPos, and the component GroupChatHistoryPanel when making the change.
Comment on lines
262
to
279
| if (showMentions && filteredMentions.length > 0) { | ||
| if (e.key === 'ArrowDown') { | ||
| e.preventDefault(); | ||
| e.stopPropagation(); | ||
| setSelectedMentionIndex((prev) => (prev < filteredAgents.length - 1 ? prev + 1 : 0)); | ||
| setSelectedMentionIndex((prev) => (prev < filteredMentions.length - 1 ? prev + 1 : 0)); | ||
| return; | ||
| } | ||
| if (e.key === 'ArrowUp') { | ||
| e.preventDefault(); | ||
| e.stopPropagation(); | ||
| setSelectedMentionIndex((prev) => (prev > 0 ? prev - 1 : filteredAgents.length - 1)); | ||
| setSelectedMentionIndex((prev) => (prev > 0 ? prev - 1 : filteredMentions.length - 1)); | ||
| return; | ||
| } | ||
| if (e.key === 'Tab' || (e.key === 'Enter' && !e.shiftKey)) { | ||
| e.preventDefault(); | ||
| e.stopPropagation(); | ||
| insertMention(filteredAgents[selectedMentionIndex].mentionName); | ||
| insertMention(filteredMentions[selectedMentionIndex]); | ||
| return; |
There was a problem hiding this comment.
Guard mention selection against stale index before insert.
On Line 278, filteredMentions[selectedMentionIndex] can be undefined if the list shrinks while the menu is open, which can crash insertMention.
💡 Suggested fix
if (e.key === 'Tab' || (e.key === 'Enter' && !e.shiftKey)) {
e.preventDefault();
e.stopPropagation();
- insertMention(filteredMentions[selectedMentionIndex]);
+ const selected = filteredMentions[selectedMentionIndex] ?? filteredMentions[0];
+ if (selected) {
+ insertMention(selected);
+ }
return;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (showMentions && filteredMentions.length > 0) { | |
| if (e.key === 'ArrowDown') { | |
| e.preventDefault(); | |
| e.stopPropagation(); | |
| setSelectedMentionIndex((prev) => (prev < filteredAgents.length - 1 ? prev + 1 : 0)); | |
| setSelectedMentionIndex((prev) => (prev < filteredMentions.length - 1 ? prev + 1 : 0)); | |
| return; | |
| } | |
| if (e.key === 'ArrowUp') { | |
| e.preventDefault(); | |
| e.stopPropagation(); | |
| setSelectedMentionIndex((prev) => (prev > 0 ? prev - 1 : filteredAgents.length - 1)); | |
| setSelectedMentionIndex((prev) => (prev > 0 ? prev - 1 : filteredMentions.length - 1)); | |
| return; | |
| } | |
| if (e.key === 'Tab' || (e.key === 'Enter' && !e.shiftKey)) { | |
| e.preventDefault(); | |
| e.stopPropagation(); | |
| insertMention(filteredAgents[selectedMentionIndex].mentionName); | |
| insertMention(filteredMentions[selectedMentionIndex]); | |
| return; | |
| if (showMentions && filteredMentions.length > 0) { | |
| if (e.key === 'ArrowDown') { | |
| e.preventDefault(); | |
| e.stopPropagation(); | |
| setSelectedMentionIndex((prev) => (prev < filteredMentions.length - 1 ? prev + 1 : 0)); | |
| return; | |
| } | |
| if (e.key === 'ArrowUp') { | |
| e.preventDefault(); | |
| e.stopPropagation(); | |
| setSelectedMentionIndex((prev) => (prev > 0 ? prev - 1 : filteredMentions.length - 1)); | |
| return; | |
| } | |
| if (e.key === 'Tab' || (e.key === 'Enter' && !e.shiftKey)) { | |
| e.preventDefault(); | |
| e.stopPropagation(); | |
| const selected = filteredMentions[selectedMentionIndex] ?? filteredMentions[0]; | |
| if (selected) { | |
| insertMention(selected); | |
| } | |
| return; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/components/GroupChatInput.tsx` around lines 262 - 279, The
mention insertion can attempt to access filteredMentions[selectedMentionIndex]
after the list changes, causing undefined to be passed into insertMention;
update the keyboard handler where it handles Tab/Enter (the block using
showMentions, filteredMentions, selectedMentionIndex and calling insertMention)
to first check filteredMentions.length > 0 and clamp/normalize
selectedMentionIndex into the valid range (e.g., use
Math.min(selectedMentionIndex, filteredMentions.length - 1) or compute a safe
index) before reading filteredMentions[safeIndex]; if no valid mention exists,
bail out without calling insertMention and optionally reset
selectedMentionIndex.
Comment on lines
1257
to
1291
| useEffect(() => { | ||
| if (isOpen && session) { | ||
| const activeToolType = selectedToolType; | ||
| const isProviderSwitch = activeToolType !== session.toolType; | ||
|
|
||
| // Load agent definition to get configOptions | ||
| window.maestro.agents.detect().then((agents: AgentConfig[]) => { | ||
| const foundAgent = agents.find((a) => a.id === session.toolType); | ||
| const foundAgent = agents.find((a) => a.id === activeToolType); | ||
| setAgent(foundAgent || null); | ||
|
|
||
| // Load models if agent supports model selection | ||
| if (foundAgent?.capabilities?.supportsModelSelection) { | ||
| setLoadingModels(true); | ||
| window.maestro.agents | ||
| .getModels(session.toolType) | ||
| .getModels(activeToolType) | ||
| .then((models) => setAvailableModels(models)) | ||
| .catch((err) => console.error('Failed to load models:', err)) | ||
| .finally(() => setLoadingModels(false)); | ||
| } else { | ||
| setAvailableModels([]); | ||
| } | ||
| }); | ||
| // Load agent config for defaults, but use session-level overrides when available | ||
| // Both model and contextWindow are now per-session | ||
| window.maestro.agents.getConfig(session.toolType).then((globalConfig) => { | ||
| // Use session-level values if set, otherwise use global defaults | ||
| const modelValue = session.customModel ?? globalConfig.model ?? ''; | ||
| const contextWindowValue = session.customContextWindow ?? globalConfig.contextWindow; | ||
| setAgentConfig({ ...globalConfig, model: modelValue, contextWindow: contextWindowValue }); | ||
| window.maestro.agents.getConfig(activeToolType).then((globalConfig) => { | ||
| if (isProviderSwitch) { | ||
| // When provider changed, use global defaults for the new provider | ||
| setAgentConfig(globalConfig); | ||
| } else { | ||
| // Use session-level values if set, otherwise use global defaults | ||
| const modelValue = session.customModel ?? globalConfig.model ?? ''; | ||
| const contextWindowValue = session.customContextWindow ?? globalConfig.contextWindow; | ||
| setAgentConfig({ ...globalConfig, model: modelValue, contextWindow: contextWindowValue }); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Prevent stale async provider loads from overwriting current state.
The provider-loading effect can run multiple overlapping async chains; older responses can resolve last and overwrite agent, availableModels, or agentConfig for the currently selected provider.
💡 Suggested fix (cancel stale effect runs)
useEffect(() => {
- if (isOpen && session) {
+ if (!isOpen || !session) return;
+ let cancelled = false;
+
const activeToolType = selectedToolType;
const isProviderSwitch = activeToolType !== session.toolType;
window.maestro.agents.detect().then((agents: AgentConfig[]) => {
+ if (cancelled) return;
const foundAgent = agents.find((a) => a.id === activeToolType);
setAgent(foundAgent || null);
if (foundAgent?.capabilities?.supportsModelSelection) {
setLoadingModels(true);
window.maestro.agents
.getModels(activeToolType)
- .then((models) => setAvailableModels(models))
+ .then((models) => {
+ if (!cancelled) setAvailableModels(models);
+ })
.catch((err) => console.error('Failed to load models:', err))
- .finally(() => setLoadingModels(false));
+ .finally(() => {
+ if (!cancelled) setLoadingModels(false);
+ });
} else {
setAvailableModels([]);
}
});
window.maestro.agents.getConfig(activeToolType).then((globalConfig) => {
+ if (cancelled) return;
if (isProviderSwitch) {
setAgentConfig(globalConfig);
} else {
const modelValue = session.customModel ?? globalConfig.model ?? '';
const contextWindowValue = session.customContextWindow ?? globalConfig.contextWindow;
setAgentConfig({ ...globalConfig, model: modelValue, contextWindow: contextWindowValue });
}
});
- }
+
+ return () => {
+ cancelled = true;
+ };
}, [isOpen, session, selectedToolType]);Also applies to: 1328-1328
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/components/NewInstanceModal.tsx` around lines 1257 - 1291, The
effect in useEffect that loads agent info and models can have stale async
responses overwrite current state; prevent this by introducing a per-run
cancellation token (e.g., a local runId or an AbortController) inside the effect
and capture it before starting any async call (for window.maestro.agents.detect,
getModels and getConfig), then check the token is still valid before calling
setAgent, setAvailableModels, setAgentConfig (or abort the fetches if the
maestro API supports AbortSignal); ensure you invalidate the token in the effect
cleanup so older promises do not update agent, availableModels, or agentConfig
for a newly selectedToolType or closed modal.
Comment on lines
450
to
457
| const handleArchiveGroupChat = useCallback( | ||
| async (id: string, archived: boolean) => { | ||
| const { activeGroupChatId, setGroupChats } = useGroupChatStore.getState(); | ||
| const updated = await window.maestro.groupChat.archive(id, archived); | ||
| setGroupChats((prev) => prev.map((c) => (c.id === id ? updated : c))); | ||
| if (archived && activeGroupChatId === id) { | ||
| handleCloseGroupChat(); | ||
| } |
There was a problem hiding this comment.
Avoid stale activeGroupChatId across await in archive flow.
Line 452 snapshots activeGroupChatId before the async IPC call. If the user navigates during the await, the stale value can trigger handleCloseGroupChat() on the wrong current UI state.
Proposed fix
const handleArchiveGroupChat = useCallback(
async (id: string, archived: boolean) => {
- const { activeGroupChatId, setGroupChats } = useGroupChatStore.getState();
+ const { setGroupChats } = useGroupChatStore.getState();
const updated = await window.maestro.groupChat.archive(id, archived);
setGroupChats((prev) => prev.map((c) => (c.id === id ? updated : c)));
- if (archived && activeGroupChatId === id) {
+ if (archived && useGroupChatStore.getState().activeGroupChatId === id) {
handleCloseGroupChat();
}
},
[handleCloseGroupChat]
);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/hooks/groupChat/useGroupChatHandlers.ts` around lines 450 - 457,
handleArchiveGroupChat captures activeGroupChatId before the await, causing a
stale check after window.maestro.groupChat.archive completes; to fix, remove the
pre-await snapshot and instead read the current activeGroupChatId from
useGroupChatStore.getState() after the await (i.e., after calling
window.maestro.groupChat.archive and updating setGroupChats) and only then call
handleCloseGroupChat() if the freshly read activeGroupChatId equals id and
archived is true; keep existing references to setGroupChats and
window.maestro.groupChat.archive and preserve the async flow in
handleArchiveGroupChat.
Comment on lines
179
to
182
| // Kill the existing AI process for this session | ||
| window.maestro.process.kill(`${sessionId}-ai`).catch(() => { | ||
| // Process may not exist — that's fine | ||
| }); |
There was a problem hiding this comment.
Do not silently swallow process-kill failures
Line 180 currently suppresses all errors from window.maestro.process.kill(...). Ignore only known “not found” cases; send unexpected failures to Sentry with context.
Suggested fix
- window.maestro.process.kill(`${sessionId}-ai`).catch(() => {
- // Process may not exist — that's fine
- });
+ window.maestro.process.kill(`${sessionId}-ai`).catch((error: unknown) => {
+ const message = error instanceof Error ? error.message.toLowerCase() : '';
+ const isMissingProcess =
+ message.includes('not found') || message.includes('no such process');
+ if (!isMissingProcess) {
+ captureException(error, {
+ extra: {
+ sessionId,
+ operation: 'kill-ai-on-tool-change',
+ },
+ });
+ }
+ });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." and "Use Sentry utilities (captureException, captureMessage) from 'src/utils/sentry' for explicit error reporting with context."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Kill the existing AI process for this session | |
| window.maestro.process.kill(`${sessionId}-ai`).catch(() => { | |
| // Process may not exist — that's fine | |
| }); | |
| // Kill the existing AI process for this session | |
| window.maestro.process.kill(`${sessionId}-ai`).catch((error: unknown) => { | |
| const message = error instanceof Error ? error.message.toLowerCase() : ''; | |
| const isMissingProcess = | |
| message.includes('not found') || message.includes('no such process'); | |
| if (!isMissingProcess) { | |
| captureException(error, { | |
| extra: { | |
| sessionId, | |
| operation: 'kill-ai-on-tool-change', | |
| }, | |
| }); | |
| } | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/hooks/session/useSessionLifecycle.ts` around lines 179 - 182,
Replace the silent catch around window.maestro.process.kill(`${sessionId}-ai`)
in useSessionLifecycle.ts so only “process not found” errors are ignored; import
captureException from 'src/utils/sentry' and in the catch inspect the error
(e.g., error.code or message) and if it’s not a known not-found case call
captureException(error, { extra: { sessionId, action: 'kill-ai-process' } }) and
rethrow (or allow the error to propagate) so unexpected failures are reported to
Sentry rather than being swallowed.
Comment on lines
69
to
73
| // Group chat entry | ||
| if (entry.groupChatId) { | ||
| onNavigateToGroupChat?.(entry.groupChatId); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Reset cycle position for group-chat history navigation too.
The group-chat branch returns before resetting cyclePositionRef, so cycle navigation state can remain stale after back/forward into a group chat.
💡 Suggested fix
if (entry.groupChatId) {
+ cyclePositionRef.current = -1;
onNavigateToGroupChat?.(entry.groupChatId);
return;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Group chat entry | |
| if (entry.groupChatId) { | |
| onNavigateToGroupChat?.(entry.groupChatId); | |
| return; | |
| } | |
| // Group chat entry | |
| if (entry.groupChatId) { | |
| cyclePositionRef.current = -1; | |
| onNavigateToGroupChat?.(entry.groupChatId); | |
| return; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/hooks/session/useSessionNavigation.ts` around lines 69 - 73,
When handling a group chat entry in useSessionNavigation (the branch that checks
entry.groupChatId and calls onNavigateToGroupChat), reset the navigation cycle
state before returning: set cyclePositionRef.current (and any related cycle
state like cycleIndexRef or similar) back to its initial value so cycle
navigation does not remain stale after navigating into a group chat; update the
group-chat branch (the if (entry.groupChatId) { ... }) to reset
cyclePositionRef.current and then call
onNavigateToGroupChat?.(entry.groupChatId) and return.
Comment on lines
138
to
144
| let unsubscribe: (() => void) | undefined; | ||
| if (enabled) { | ||
| // Initial fetch | ||
| fetchStats(); | ||
| // Subscribe to stats updates with stable debounced function | ||
| unsubscribe = window.maestro.stats.onStatsUpdate(debouncedUpdate); | ||
| } |
There was a problem hiding this comment.
loading can stay stuck when enabled is false.
On Line 139, the effect skips fetch/subscription when disabled, but loading remains true from initial state. Consumers can observe a permanent loading state while disabled.
Suggested fix
useEffect(() => {
mountedRef.current = true;
let unsubscribe: (() => void) | undefined;
- if (enabled) {
+ if (!enabled) {
+ setLoading(false);
+ } else {
// Initial fetch
fetchStats();
// Subscribe to stats updates with stable debounced function
unsubscribe = window.maestro.stats.onStatsUpdate(debouncedUpdate);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let unsubscribe: (() => void) | undefined; | |
| if (enabled) { | |
| // Initial fetch | |
| fetchStats(); | |
| // Subscribe to stats updates with stable debounced function | |
| unsubscribe = window.maestro.stats.onStatsUpdate(debouncedUpdate); | |
| } | |
| let unsubscribe: (() => void) | undefined; | |
| if (!enabled) { | |
| setLoading(false); | |
| } else { | |
| // Initial fetch | |
| fetchStats(); | |
| // Subscribe to stats updates with stable debounced function | |
| unsubscribe = window.maestro.stats.onStatsUpdate(debouncedUpdate); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/hooks/stats/useStats.ts` around lines 138 - 144, In useStats,
when enabled is false the effect skips calling fetchStats and subscribing but
leaves the loading state true; update the effect so that when enabled is false
you explicitly set loading to false (e.g., call the same state setter used by
fetchStats) and avoid creating a subscription; ensure the unsubscribe variable
and cleanup logic around window.maestro.stats.onStatsUpdate and debouncedUpdate
remain intact so toggling enabled on/off always clears or sets loading
correctly.
- 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
Resolved 30 source file conflicts. Key resolution decisions: - Adopted main's store-read pattern for group chat state and context warning thresholds (removed prop drilling through useSessionListProps) - Fixed zustand v5 shallow import (useShallow from zustand/react/shallow) - Kept branch additions: context menu positioning, orphaned file preview fix, provider switching in edit modal - Took main's formatting for multi-line function signatures
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/renderer/components/GroupChatPanel.tsx`:
- Line 40: GroupChatPanel declares and destructures the groups?: Group[] prop
but never passes it down, so GroupChatInput (which supports group mentions)
can't use it; update the GroupChatPanel component to forward the groups prop
into the GroupChatInput element (or, if unused, remove the prop and its type
from GroupChatPanel). Specifically, locate the groups destructuring in the
GroupChatPanel component and add groups={groups} to the GroupChatInput JSX (or
delete the groups prop and references if you choose removal) so the group
mention filtering in GroupChatInput can function.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/renderer/App.tsxsrc/renderer/components/GroupChatPanel.tsxsrc/renderer/components/MainPanel.tsxsrc/renderer/components/SessionList.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- src/renderer/components/SessionList.tsx
- src/renderer/components/MainPanel.tsx
| onToggleRightPanel: () => void; | ||
| shortcuts: Record<string, Shortcut>; | ||
| sessions: Session[]; | ||
| groups?: Group[]; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the GroupChatPanel.tsx file and examine the lines in question
find . -name "GroupChatPanel.tsx" -type f
# Get the full content of the file to check lines 14, 40, 91, 114-169
wc -l "$(find . -name 'GroupChatPanel.tsx' -type f | head -1)"Repository: RunMaestro/Maestro
Length of output: 155
🏁 Script executed:
# Show the relevant sections of GroupChatPanel.tsx
FILE=$(find . -name "GroupChatPanel.tsx" -type f | head -1)
echo "=== Imports and line 14 ==="
head -20 "$FILE"
echo ""
echo "=== Around line 40 (interface definition) ==="
sed -n '35,45p' "$FILE"
echo ""
echo "=== Around line 91 (destructuring) ==="
sed -n '85,100p' "$FILE"
echo ""
echo "=== Component return and props passing (lines 114-169) ==="
sed -n '110,175p' "$FILE"Repository: RunMaestro/Maestro
Length of output: 3100
🏁 Script executed:
# Search for any usage of 'groups' variable in GroupChatPanel.tsx
FILE=$(find . -name "GroupChatPanel.tsx" -type f | head -1)
echo "=== All occurrences of 'groups' in GroupChatPanel.tsx ==="
grep -n "groups" "$FILE"Repository: RunMaestro/Maestro
Length of output: 153
🏁 Script executed:
# Check if GroupChatInput, GroupChatHeader, GroupChatMessages accept a 'groups' prop
echo "=== Checking GroupChatInput.tsx ==="
find . -name "GroupChatInput.tsx" -type f -exec grep -l "groups" {} \; || echo "No 'groups' found in GroupChatInput.tsx"
echo "=== Checking GroupChatHeader.tsx ==="
find . -name "GroupChatHeader.tsx" -type f -exec grep -l "groups" {} \; || echo "No 'groups' found in GroupChatHeader.tsx"
echo "=== Checking GroupChatMessages.tsx ==="
find . -name "GroupChatMessages.tsx" -type f -exec grep -l "groups" {} \; || echo "No 'groups' found in GroupChatMessages.tsx"Repository: RunMaestro/Maestro
Length of output: 218
🏁 Script executed:
# Let's look at the type definitions for these child components to see what props they accept
echo "=== Checking GroupChatInput.tsx interface/props ==="
find . -name "GroupChatInput.tsx" -type f -exec sed -n '1,50p' {} \;
echo ""
echo "=== Checking GroupChatHeader.tsx interface/props ==="
find . -name "GroupChatHeader.tsx" -type f -exec sed -n '1,50p' {} \;
echo ""
echo "=== Checking GroupChatMessages.tsx interface/props ==="
find . -name "GroupChatMessages.tsx" -type f -exec sed -n '1,50p' {} \;Repository: RunMaestro/Maestro
Length of output: 4600
groups prop is accepted but never consumed inside the component.
groups is destructured at line 91 but is not passed to GroupChatInput (or any other child component), and is not used in any rendering logic. GroupChatInput imports Group and supports group mentions internally, making it the likely intended consumer—but the prop forwarding is missing.
Either pass groups to GroupChatInput to enable group-aware mention filtering, or remove the prop entirely.
🔧 Example fix — forward to GroupChatInput
<GroupChatInput
theme={theme}
state={state}
onSend={onSendMessage}
participants={groupChat.participants}
sessions={sessions}
+ groups={groups}
groupChatId={groupChat.id}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/components/GroupChatPanel.tsx` at line 40, GroupChatPanel
declares and destructures the groups?: Group[] prop but never passes it down, so
GroupChatInput (which supports group mentions) can't use it; update the
GroupChatPanel component to forward the groups prop into the GroupChatInput
element (or, if unused, remove the prop and its type from GroupChatPanel).
Specifically, locate the groups destructuring in the GroupChatPanel component
and add groups={groups} to the GroupChatInput JSX (or delete the groups prop and
references if you choose removal) so the group mention filtering in
GroupChatInput can function.
…rector'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
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.
Summary
inputMode, so switching to terminal mode without clearingactiveFileTabIdleft the file preview visible with no tab bar — creating an unnavigable dead-end stateauthenticateAfterError(agentStore) and remote mode switching (useRemoteIntegration)Changes
inputMode === 'ai'activeFileTabIdinauthenticateAfterErrorwhen switching to terminalactiveFileTabIdon remote terminal mode switchactiveFileTabIdwheninputMode !== 'ai'on restoreTest plan
Summary by CodeRabbit
Bug Fixes
New Features
Refactor
Tests