Merged
Conversation
📝 WalkthroughWalkthroughPR adds null-safety guards for file reading across multiple components, implements paginated GitHub issue fetching with cache validation via repoSlugs tracking, updates test mocks and assertions for shortcuts and console.warn calls, adds keyboard accessibility to GroupChatHeader, and refactors various formatting and state synchronization changes. Changes
Sequence DiagramsequenceDiagram
participant Client as useSymphony Hook
participant IPC as symphony.ts Handler
participant GitHub as GitHub API
participant Cache as Settings Store
Client->>IPC: getIssueCounts(repos)
rect rgba(100, 150, 200, 0.5)
note right of IPC: Cache Validation
IPC->>IPC: Normalize & sort repos
IPC->>Cache: Check cached repoSlugs
alt Slugs match && cache fresh
IPC-->>Client: Return cached counts
end
end
rect rgba(150, 100, 200, 0.5)
note right of IPC: Paginated Fetch (1-10 pages)
IPC->>GitHub: Fetch issues page 1<br/>(per_page=100)
GitHub-->>IPC: Page 1 results
IPC->>IPC: Aggregate into counts map
loop While page has 100 items
IPC->>GitHub: Fetch next page
GitHub-->>IPC: Next page results
IPC->>IPC: Aggregate counts
end
end
rect rgba(200, 150, 100, 0.5)
note right of Cache: Cache Update
IPC->>Cache: Store counts, fetchedAt,<br/>repoSlugs
end
IPC-->>Client: Return fresh counts
Client->>Client: Update state
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
src/renderer/components/GroupChatHeader.tsx (1)
48-55:⚠️ Potential issue | 🟡 MinorAdd keyboard accessibility to the clickable heading.
The
h1element hasonClickbut lacks keyboard support. Users navigating with keyboards cannot activate the rename action via this element. AddtabIndex={0}and anonKeyDownhandler for Enter/Space.♿ Proposed fix for keyboard accessibility
<h1 className="text-lg font-semibold cursor-pointer hover:opacity-80" style={{ color: theme.colors.textMain }} onClick={onRename} + onKeyDown={(e) => { + if (e.key === 'Enter' || e.key === ' ') { + e.preventDefault(); + onRename(); + } + }} + tabIndex={0} title="Click to rename" + role="button" > Group Chat: {name} </h1>As per coding guidelines: "Add tabIndex attribute and focus event handlers when implementing components that need keyboard focus".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/GroupChatHeader.tsx` around lines 48 - 55, The clickable heading in GroupChatHeader (the h1 that renders "Group Chat: {name}" and uses onRename) lacks keyboard support; add tabIndex={0} so it is focusable and implement an onKeyDown handler that calls onRename when Enter or Space is pressed (checking event.key === 'Enter' || event.key === ' ' / 'Spacebar' as needed) and optionally preventDefault for Space to avoid page scrolling; consider adding role="button" and an accessible title/aria-label if not already present.src/renderer/hooks/batch/useWorktreeManager.ts (1)
358-365:⚠️ Potential issue | 🟡 MinorPropagate
targetBranchin the exception path too.
targetBranchis returned for handled outcomes but omitted when execution falls intocatch, so callers lose branch context on thrown failures.💡 Suggested fix
const createPR = useCallback( async (options: CreatePROptions): Promise<PRCreationResult> => { const { worktreePath, mainRepoCwd, worktree, documents, totalCompletedTasks } = options; + let baseBranch: string | undefined = worktree.prTargetBranch; console.log('[WorktreeManager] Creating PR from worktree branch', worktree.branchName); try { // Use the user-selected target branch, or fall back to default branch detection - let baseBranch = worktree.prTargetBranch; if (!baseBranch) { const defaultBranchResult = await window.maestro.git.getDefaultBranch(mainRepoCwd); baseBranch = defaultBranchResult.success && defaultBranchResult.branch ? defaultBranchResult.branch : 'main'; } @@ } catch (error) { console.error('[WorktreeManager] Error creating PR:', error); return { success: false, error: error instanceof Error ? error.message : 'Unknown error', + targetBranch: baseBranch, }; } }, [generatePRTitle, generatePRBody] );Also applies to: 407-412
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/hooks/batch/useWorktreeManager.ts` around lines 358 - 365, The code computes baseBranch from worktree.prTargetBranch (falling back to window.maestro.git.getDefaultBranch(mainRepoCwd) or 'main') but only returns/attaches targetBranch for handled outcomes; when an exception is thrown the branch context is lost—update the catch/throw paths in useWorktreeManager (the blocks around baseBranch / worktree.prTargetBranch and the similar section at lines ~407-412) to include the resolved baseBranch/targetBranch in the thrown error or the error payload so callers always receive targetBranch information.src/renderer/hooks/batch/useBatchProcessor.ts (1)
1-2014:⚠️ Potential issue | 🟡 MinorCI blocker: Prettier check is failing for this file.
The pipeline reports formatting failure here; please run formatter on this file before merge.
As per coding guidelines
src/**/*.{ts,tsx,js,jsx,json}: Use tabs for indentation, not spaces, throughout the codebase.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/hooks/batch/useBatchProcessor.ts` around lines 1 - 2014, Prettier/formatting fails due to spaces used for indentation; reformat this file to use tabs per project rules. Run the project's formatter (prettier or npm/yarn format script) on the file and ensure tabs are used (not spaces) across top-level functions/hooks like useBatchProcessor, startBatchRun, updateBatchStateAndBroadcast, pauseBatchOnError, and other exported functions; fix any lingering lint/Prettier issues reported and re-run CI to verify the Prettier check passes before merging.src/renderer/hooks/batch/useAutoRunHandlers.ts (1)
114-127:⚠️ Potential issue | 🟡 MinorClear the dedup marker when worktree setup throws.
If
window.maestro.git.worktreeSetup(...)throws at Line 119, the marker set at Line 116 is never cleared. That can cause transient false dedup behavior until TTL expiry.Proposed fix
// Step 2: Create worktree on disk - const result = await window.maestro.git.worktreeSetup( - parentSession.cwd, - worktreePath, - branchName, - sshRemoteId - ); + let result; + try { + result = await window.maestro.git.worktreeSetup( + parentSession.cwd, + worktreePath, + branchName, + sshRemoteId + ); + } catch (error) { + clearRecentlyCreatedWorktreePath(worktreePath); + throw error; + } if (!result.success) { clearRecentlyCreatedWorktreePath(worktreePath); notifyToast({🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/hooks/batch/useAutoRunHandlers.ts` around lines 114 - 127, The marker set by markWorktreePathAsRecentlyCreated(worktreePath) isn't cleared if window.maestro.git.worktreeSetup(...) throws; wrap the worktreeSetup call in a try/catch (or try/finally) so that clearRecentlyCreatedWorktreePath(worktreePath) is always invoked on error (and/or in finally) before rethrowing or returning an error result, ensuring the dedup marker is removed even when worktreeSetup throws.src/renderer/App.tsx (1)
1-3244:⚠️ Potential issue | 🟡 MinorPrettier check is failing for this file in CI.
Formatting must be normalized before merge (
npx prettier --write src/renderer/App.tsx), otherwise the current pipeline failure remains.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/App.tsx` around lines 1 - 3244, Prettier formatting in src/renderer/App.tsx is failing CI; run the formatter and commit the changes. Run npx prettier --write src/renderer/App.tsx (or your repo's prettier command), verify the file (MaestroConsoleInner / MaestroConsole) is reformatted, review any changes, then stage and commit the updated file so the CI Prettier check passes.src/renderer/components/DocumentGraph/DocumentGraphView.tsx (1)
1-2142:⚠️ Potential issue | 🟡 MinorPrettier check is failing for this file in CI.
Please run
npx prettier --write src/renderer/components/DocumentGraph/DocumentGraphView.tsxto clear the current pipeline failure.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/DocumentGraph/DocumentGraphView.tsx` around lines 1 - 2142, Prettier formatting is failing for DocumentGraphView.tsx; run the formatter and commit the updated file to fix CI. Specifically, run `npx prettier --write src/renderer/components/DocumentGraph/DocumentGraphView.tsx`, review the changes in the DocumentGraphView component (and helper buildFileTreeFromPaths) to ensure no logic was altered, then stage and commit the formatted file so the pipeline passes.
🧹 Nitpick comments (7)
src/renderer/components/FilePreview.tsx (1)
2438-2438: Consider user feedback for large file loading.When loading full content of very large files, syntax highlighting may cause a brief UI freeze. This is an acceptable trade-off for a user-initiated action, but you could optionally:
- Add a loading spinner while content renders
- For files significantly larger than 100KB (e.g., >500KB), show a confirmation or warning
This is low priority since the user explicitly requests full content.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/FilePreview.tsx` at line 2438, The onClick should guard large-file loads and surface progress: update the FilePreview component's onClick handler (where setShowFullContent is called) to an async handler that (1) checks the file size (use the existing file.size or contentLength) and for sizes > 500*1024 shows a user confirmation/warning before proceeding, (2) sets a local loading state (e.g., isLoadingFullContent) to true before triggering the full-content render (setShowFullContent(true)), and (3) clear the loading state after the render completes; optionally use requestIdleCallback or setTimeout to defer heavy syntax-highlighting work so the spinner can display. Ensure you add and use isLoadingFullContent state and render a spinner in FilePreview when true, and reuse the existing setShowFullContent symbol so behavior remains consistent.src/__tests__/renderer/components/GroupChatHeader.test.tsx (1)
43-52: Consider resetting mocks between tests.The mock functions in
defaultPropsare defined once at module level. While current tests likely pass, shared mocks can cause flaky tests if call counts accumulate. Adding abeforeEachto clear mocks improves test isolation.🧪 Proposed fix to reset mocks
const defaultProps = { theme: mockTheme, name: 'Test Chat', participantCount: 3, onRename: vi.fn(), onShowInfo: vi.fn(), rightPanelOpen: false, onToggleRightPanel: vi.fn(), shortcuts: mockShortcuts, }; describe('GroupChatHeader', () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + it('renders group chat name and participant count', () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/renderer/components/GroupChatHeader.test.tsx` around lines 43 - 52, Add test isolation by resetting and reinitializing the module-level mocks before each test: add a beforeEach that calls vi.clearAllMocks() (or vi.resetAllMocks()) and reassigns defaultProps with fresh handler mocks (onRename, onShowInfo, onToggleRightPanel) so call counts don’t accumulate between tests; reference the existing defaultProps object and the mock handler names to locate where to recreate the vi.fn() instances.src/renderer/stores/settingsStore.ts (1)
714-719: Deduplicate allowed layout literals to avoid drift.The same
['mindmap', 'radial', 'force']list is duplicated in setter and loader. Centralizing it avoids subtle future mismatches.♻️ Proposed refactor
export type DocumentGraphLayoutType = 'mindmap' | 'radial' | 'force'; +const DOCUMENT_GRAPH_LAYOUT_TYPES = ['mindmap', 'radial', 'force'] as const; ... setDocumentGraphLayoutType: (value) => { - const valid: DocumentGraphLayoutType[] = ['mindmap', 'radial', 'force']; - const layoutType = valid.includes(value) ? value : 'mindmap'; + const layoutType = DOCUMENT_GRAPH_LAYOUT_TYPES.includes(value) ? value : 'mindmap'; set({ documentGraphLayoutType: layoutType }); window.maestro.settings.set('documentGraphLayoutType', layoutType); }, ... if (allSettings['documentGraphLayoutType'] !== undefined) { const lt = allSettings['documentGraphLayoutType'] as string; - if (['mindmap', 'radial', 'force'].includes(lt)) { + if (DOCUMENT_GRAPH_LAYOUT_TYPES.includes(lt as DocumentGraphLayoutType)) { patch.documentGraphLayoutType = lt as DocumentGraphLayoutType; } }Also applies to: 1622-1627
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/stores/settingsStore.ts` around lines 714 - 719, The allowed layout literals are duplicated; extract the array ['mindmap','radial','force'] into a single constant (e.g., ALLOWED_DOCUMENT_GRAPH_LAYOUTS: DocumentGraphLayoutType[]) defined near the top of the module and use that constant in setDocumentGraphLayoutType (replace the local valid variable) and the corresponding loader code (the other occurrence around lines 1622-1627) so both places use ALLOWED_DOCUMENT_GRAPH_LAYOUTS.includes(value) and default to 'mindmap' if invalid.src/__tests__/renderer/utils/fileExplorer.test.ts (1)
460-495: Assert warning emission in the new dedup tests.You already spy on
console.warn; adding explicit assertions will lock in the expected observability behavior for duplicate collapse paths.Suggested test tightening
const consoleSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}); const result = await loadFileTree('/project'); + expect(consoleSpy).toHaveBeenCalled(); consoleSpy.mockRestore(); @@ const consoleSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}); const result = await loadFileTree('/project'); + expect(consoleSpy).toHaveBeenCalled(); consoleSpy.mockRestore();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/renderer/utils/fileExplorer.test.ts` around lines 460 - 495, The new dedup tests spy on console.warn but don't assert it was called; update both tests for duplicated entries ("deduplicates entries returned by readDir" and "deduplicates entries in nested directories") to assert the warning was emitted: after calling loadFileTree('/project') assert that the consoleSpy (the vi.spyOn(console, 'warn') mock) was called (e.g., expect(consoleSpy).toHaveBeenCalled() or toHaveBeenCalledTimes(1)) and then restore the spy as already done, keeping references to loadFileTree and consoleSpy so the assertions target the exact spy used in each test.src/renderer/hooks/symphony/useSymphony.ts (1)
198-225: Clear staleissueCountswhen fetching fails or repo list becomes empty.Current flow keeps old counts on failure/empty repo set, which can leave stale badges visible. Resetting counts in those paths avoids misleading UI state.
Proposed refactor
const fetchIssueCounts = useCallback(async (repos: RegisteredRepository[]) => { - if (repos.length === 0) return; + if (repos.length === 0) { + setIssueCounts(null); + return; + } setIsLoadingIssueCounts(true); try { const slugs = repos.map((r) => r.slug); const response = await window.maestro.symphony.getIssueCounts(slugs); if (response.counts) { setIssueCounts(response.counts); } } catch (err) { console.error('Failed to fetch issue counts:', err); + setIssueCounts(null); } finally { setIsLoadingIssueCounts(false); } }, []);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/hooks/symphony/useSymphony.ts` around lines 198 - 225, The fetchIssueCounts flow can leave stale issueCounts displayed when the fetch fails or when the repo list becomes empty; update the fetchIssueCounts function and its related effect to clear state: inside fetchIssueCounts ensure that when repos.length === 0 you call setIssueCounts({}) (or an initial empty value) before returning, and in the catch block call setIssueCounts({}) as well as setIsLoadingIssueCounts(false); additionally, in the useEffect that watches repositories, if repositories.length === 0 call setIssueCounts({}) to proactively clear counts when the repo list is emptied (refer to fetchIssueCounts, setIssueCounts, setIsLoadingIssueCounts, and the repositories effect).src/renderer/components/DocumentGraph/mindMapLayouts.ts (2)
108-135: Consider cache eviction strategy fornodeHeightCache.The
nodeHeightCacheMap at module scope will grow unbounded over time. While the cache keys are based on text length and limit (not the actual text), in long-running Electron apps with varyingpreviewCharLimitvalues, this could accumulate entries.Consider adding a size limit or using an LRU cache if memory becomes a concern. However, given the key structure (
${truncatedLength}:${previewCharLimit}), the practical number of unique keys is bounded by reasonable text lengths and limit values, so this is likely acceptable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/DocumentGraph/mindMapLayouts.ts` around lines 108 - 135, The module-level nodeHeightCache Map can grow unbounded; modify it to enforce a bounded size (e.g., MAX_CACHE_ENTRIES) or swap it for an LRU cache implementation and evict the oldest entries when capacity is exceeded. Update the nodeHeightCache initialization (replace Map with an LRU structure or a wrapped Map) and change the calculateNodeHeight flow that calls nodeHeightCache.set(cacheKey, height) to perform eviction when size > MAX_CACHE_ENTRIES (or use the LRU's set method). You can implement a simple LRU using insertion order on Map or import a small lru-cache; keep nodeHeightCache and calculateNodeHeight names so callers remain unchanged.
47-58: Consider exportingLayoutFunctiontype for extensibility.The
LayoutFunctiontype (line 48) is currently internal. If you plan to allow custom layout algorithms in the future, exporting this type would enable type-safe implementations. For now, this is a minor consideration.💡 Optional: Export the type
-/** Common layout function signature */ -type LayoutFunction = ( +/** Common layout function signature */ +export type LayoutFunction = (🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/DocumentGraph/mindMapLayouts.ts` around lines 47 - 58, The LayoutFunction type is currently internal; export it so external modules can implement custom layouts with type safety. Modify the declaration of LayoutFunction in mindMapLayouts.ts to be exported (export type LayoutFunction = ...) and ensure any related symbols used by its signature — MindMapNode, MindMapLink, LayoutResult — are exported or at least accessible where consumers will implement layouts; update exports if necessary so consumers can import LayoutFunction and its dependent types.
🤖 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/auto-scroll.test.tsx`:
- Line 126: Prettier is failing because the inline logs object is too long;
reformat the logs array entry in auto-scroll.test.tsx by breaking the object
into multiple lines (e.g., put id, text, timestamp, and source on their own
lines) or simply run the formatter to rewrite the file; target the logs
variable/array in the test (the object with id 'default-log') and run npx
prettier --write src/__tests__/renderer/components/auto-scroll.test.tsx to apply
the fix.
In `@src/__tests__/renderer/components/GroupChatHeader.test.tsx`:
- Around line 39-41: The mock object mockShortcuts in GroupChatHeader.test.tsx
does not conform to the Shortcut interface (it uses description and lacks id and
label) and the "as Shortcut" cast hides the mismatch; fix by replacing the
mocked entry (toggleRightPanel) with a valid Shortcut shape including id, label,
and keys (and any other required fields from src/renderer/types/index.ts) and
remove the unnecessary type cast so TypeScript enforces correctness; update any
test assertions that referenced description to use label or the new fields as
appropriate.
In `@src/__tests__/renderer/components/TerminalOutput.test.tsx`:
- Around line 1785-1901: Prettier formatting failed for the test file containing
the describe block "tool log detail extraction" and tests referencing
TerminalOutput and LogEntry (tests: "renders TodoWrite tool with task
summary...", "renders TodoWrite with first task when none in progress", "renders
Bash tool with command detail", "renders tool with no extractable detail
gracefully"); fix by running Prettier to auto-format the test file (e.g., npx
prettier --write <this test file>) and re-run CI to ensure the formatting error
is resolved.
In `@src/__tests__/renderer/hooks/useBatchProcessor.test.ts`:
- Around line 5682-5986: The CI failure is a Prettier formatting error in the
test file containing the useBatchProcessor tests; run Prettier (e.g. npx
prettier --write .) to reformat the file and commit the changes. Locate the
tests that reference useBatchProcessor and startBatchRun and assertions using
mockWorktreeSetup, mockNotifyToast, and mockOnAddHistoryEntry, reformat that
file with Prettier, then re-run tests to ensure formatting is fixed before
pushing.
- Around line 27-30: The test currently declares mockNotifyToast after the
vi.mock factory which can trigger a TDZ ReferenceError; hoist the mock by
creating it with vi.hoisted() and use that hoisted mock in the vi.mock factory
for the notificationStore notifyToast export (i.e., replace the top-level const
mockNotifyToast with a vi.hoisted() mock and reference that hoisted mock inside
the vi.mock factory that provides notifyToast).
In `@src/main/ipc/handlers/filesystem.ts`:
- Around line 170-176: The filesystem IPC handler currently returns null for
ENOENT in its catch block (the change in src/main/ipc/handlers/filesystem.ts),
but several renderer callers (MarkdownRenderer.tsx, FilePreview.tsx,
AutoRun.tsx) assume a non-null string and call .startsWith('data:') leading to
runtime errors, and global.d.ts still types the IPC as Promise<string>; fix by
either (A) adding null/undefined guards in each caller before calling
.startsWith (e.g., check value !== null && value.startsWith(...)) and update
tests, or (B) change the handler to accept an explicit option (default false) to
preserve previous behavior and only return null when callers request it, and
update global.d.ts to Promise<string | null> when opting into null returns; pick
one approach and make consistent updates to the handler, the callers
(MarkdownRenderer, FilePreview, AutoRun), and the type declaration to avoid
type/ runtime mismatches.
In `@src/main/ipc/handlers/symphony.ts`:
- Around line 1230-1239: The cache check currently returns cache.issueCounts
whenever it's fresh, but it must also validate that the cached data was fetched
for the same requested repoSlugs; update the logic around cache.issueCounts (and
the similar block at 1247-1254) to compare the requested repoSlugs parameter
with whatever identifier is stored with the cache (e.g., a
cache.issueCounts.repoSlugs or fetchedFor field) using an equality or
set-equality check before returning; if the stored slugs don’t match, treat the
cache as invalid (fall through to refresh), otherwise continue using
isCacheValid(cache.issueCounts.fetchedAt, ISSUE_COUNTS_CACHE_TTL_MS) as now and
return the cached counts when both TTL and slug match pass.
- Around line 527-559: The search currently fetches only one page (per_page=100)
and misses results beyond page 1; modify the logic around the URL/query fetch
(where query, GITHUB_API_BASE and repoSlugs are used) to paginate using the page
parameter: loop pages starting at 1, request
`${GITHUB_API_BASE}/search/issues?q=${query}&per_page=100&page=${page}`, parse
each page's data.items and increment counts[slug] (initialize counts from
repoSlugs as before), stop when a page returns fewer than per_page items or when
the cumulative results would exceed GitHub's 1000-result Search API cap (i.e.,
stop after page 10), and keep the same error handling (throw SymphonyError on
non-ok responses) and logging via logger.info when done.
In `@src/renderer/components/DocumentGraph/DocumentGraphView.tsx`:
- Around line 220-221: The local state layoutType (created with useState and
updated via setLayoutType) is only initialized from the defaultLayoutType prop
and will go out of sync if defaultLayoutType changes; add a useEffect that
watches defaultLayoutType and calls setLayoutType(defaultLayoutType) to keep the
internal state in sync (also apply the same pattern to the other instance
initialized around the same area, e.g., the related state created near lines
202-204).
In `@src/renderer/components/DocumentGraph/mindMapLayouts.ts`:
- Around line 1-12: The file fails Prettier formatting; run the formatter and
save the fixed file so the comment block and surrounding code conform to project
formatting rules. Specifically, run `npx prettier --write` (or your editor's
Prettier integration) on src/renderer/components/DocumentGraph/mindMapLayouts.ts
to reformat the header and any related exports like calculateLayout and the
LayoutResult types, then stage the changes and push the updated file.
In `@src/renderer/components/FilePreview.tsx`:
- Around line 2431-2441: Prettier formatting failed for the FilePreview
component; run the formatter (e.g., npx prettier --write) on the file containing
the FilePreview component and reformat the JSX around the Load full file button
and the setShowFullContent handler to match project Prettier rules so CI passes;
then stage the formatted file and push the commit.
In `@src/renderer/components/SymphonyModal.tsx`:
- Around line 2048-2052: Prettier formatting has drifted in SymphonyModal.tsx
causing CI to fail; run the formatter and commit the changes by running `npx
prettier --write src/renderer/components/SymphonyModal.tsx` (or your project's
formatting command) to reformat the file, then stage and push the updated file
so the span containing filteredRepositories, isLoadingIssueCounts and Loader2 is
properly formatted to satisfy Prettier.
In `@src/renderer/hooks/batch/useBatchProcessor.ts`:
- Around line 796-802: The toast call to notifyToast in useBatchProcessor.ts
uses the wrong metadata key: it passes sessionId instead of the expected
agentSessionId, so session-linked logging is lost; update the notifyToast
invocation that constructs the auto-run toast (the block calling notifyToast
with title 'Auto Run Started') to replace the sessionId property with
agentSessionId: pass agentSessionId: sessionId (or rename the variable if
appropriate) so the toast metadata matches notifyToast's contract and restores
session attribution.
---
Outside diff comments:
In `@src/renderer/App.tsx`:
- Around line 1-3244: Prettier formatting in src/renderer/App.tsx is failing CI;
run the formatter and commit the changes. Run npx prettier --write
src/renderer/App.tsx (or your repo's prettier command), verify the file
(MaestroConsoleInner / MaestroConsole) is reformatted, review any changes, then
stage and commit the updated file so the CI Prettier check passes.
In `@src/renderer/components/DocumentGraph/DocumentGraphView.tsx`:
- Around line 1-2142: Prettier formatting is failing for DocumentGraphView.tsx;
run the formatter and commit the updated file to fix CI. Specifically, run `npx
prettier --write src/renderer/components/DocumentGraph/DocumentGraphView.tsx`,
review the changes in the DocumentGraphView component (and helper
buildFileTreeFromPaths) to ensure no logic was altered, then stage and commit
the formatted file so the pipeline passes.
In `@src/renderer/components/GroupChatHeader.tsx`:
- Around line 48-55: The clickable heading in GroupChatHeader (the h1 that
renders "Group Chat: {name}" and uses onRename) lacks keyboard support; add
tabIndex={0} so it is focusable and implement an onKeyDown handler that calls
onRename when Enter or Space is pressed (checking event.key === 'Enter' ||
event.key === ' ' / 'Spacebar' as needed) and optionally preventDefault for
Space to avoid page scrolling; consider adding role="button" and an accessible
title/aria-label if not already present.
In `@src/renderer/hooks/batch/useAutoRunHandlers.ts`:
- Around line 114-127: The marker set by
markWorktreePathAsRecentlyCreated(worktreePath) isn't cleared if
window.maestro.git.worktreeSetup(...) throws; wrap the worktreeSetup call in a
try/catch (or try/finally) so that
clearRecentlyCreatedWorktreePath(worktreePath) is always invoked on error
(and/or in finally) before rethrowing or returning an error result, ensuring the
dedup marker is removed even when worktreeSetup throws.
In `@src/renderer/hooks/batch/useBatchProcessor.ts`:
- Around line 1-2014: Prettier/formatting fails due to spaces used for
indentation; reformat this file to use tabs per project rules. Run the project's
formatter (prettier or npm/yarn format script) on the file and ensure tabs are
used (not spaces) across top-level functions/hooks like useBatchProcessor,
startBatchRun, updateBatchStateAndBroadcast, pauseBatchOnError, and other
exported functions; fix any lingering lint/Prettier issues reported and re-run
CI to verify the Prettier check passes before merging.
In `@src/renderer/hooks/batch/useWorktreeManager.ts`:
- Around line 358-365: The code computes baseBranch from worktree.prTargetBranch
(falling back to window.maestro.git.getDefaultBranch(mainRepoCwd) or 'main') but
only returns/attaches targetBranch for handled outcomes; when an exception is
thrown the branch context is lost—update the catch/throw paths in
useWorktreeManager (the blocks around baseBranch / worktree.prTargetBranch and
the similar section at lines ~407-412) to include the resolved
baseBranch/targetBranch in the thrown error or the error payload so callers
always receive targetBranch information.
---
Nitpick comments:
In `@src/__tests__/renderer/components/GroupChatHeader.test.tsx`:
- Around line 43-52: Add test isolation by resetting and reinitializing the
module-level mocks before each test: add a beforeEach that calls
vi.clearAllMocks() (or vi.resetAllMocks()) and reassigns defaultProps with fresh
handler mocks (onRename, onShowInfo, onToggleRightPanel) so call counts don’t
accumulate between tests; reference the existing defaultProps object and the
mock handler names to locate where to recreate the vi.fn() instances.
In `@src/__tests__/renderer/utils/fileExplorer.test.ts`:
- Around line 460-495: The new dedup tests spy on console.warn but don't assert
it was called; update both tests for duplicated entries ("deduplicates entries
returned by readDir" and "deduplicates entries in nested directories") to assert
the warning was emitted: after calling loadFileTree('/project') assert that the
consoleSpy (the vi.spyOn(console, 'warn') mock) was called (e.g.,
expect(consoleSpy).toHaveBeenCalled() or toHaveBeenCalledTimes(1)) and then
restore the spy as already done, keeping references to loadFileTree and
consoleSpy so the assertions target the exact spy used in each test.
In `@src/renderer/components/DocumentGraph/mindMapLayouts.ts`:
- Around line 108-135: The module-level nodeHeightCache Map can grow unbounded;
modify it to enforce a bounded size (e.g., MAX_CACHE_ENTRIES) or swap it for an
LRU cache implementation and evict the oldest entries when capacity is exceeded.
Update the nodeHeightCache initialization (replace Map with an LRU structure or
a wrapped Map) and change the calculateNodeHeight flow that calls
nodeHeightCache.set(cacheKey, height) to perform eviction when size >
MAX_CACHE_ENTRIES (or use the LRU's set method). You can implement a simple LRU
using insertion order on Map or import a small lru-cache; keep nodeHeightCache
and calculateNodeHeight names so callers remain unchanged.
- Around line 47-58: The LayoutFunction type is currently internal; export it so
external modules can implement custom layouts with type safety. Modify the
declaration of LayoutFunction in mindMapLayouts.ts to be exported (export type
LayoutFunction = ...) and ensure any related symbols used by its signature —
MindMapNode, MindMapLink, LayoutResult — are exported or at least accessible
where consumers will implement layouts; update exports if necessary so consumers
can import LayoutFunction and its dependent types.
In `@src/renderer/components/FilePreview.tsx`:
- Line 2438: The onClick should guard large-file loads and surface progress:
update the FilePreview component's onClick handler (where setShowFullContent is
called) to an async handler that (1) checks the file size (use the existing
file.size or contentLength) and for sizes > 500*1024 shows a user
confirmation/warning before proceeding, (2) sets a local loading state (e.g.,
isLoadingFullContent) to true before triggering the full-content render
(setShowFullContent(true)), and (3) clear the loading state after the render
completes; optionally use requestIdleCallback or setTimeout to defer heavy
syntax-highlighting work so the spinner can display. Ensure you add and use
isLoadingFullContent state and render a spinner in FilePreview when true, and
reuse the existing setShowFullContent symbol so behavior remains consistent.
In `@src/renderer/hooks/symphony/useSymphony.ts`:
- Around line 198-225: The fetchIssueCounts flow can leave stale issueCounts
displayed when the fetch fails or when the repo list becomes empty; update the
fetchIssueCounts function and its related effect to clear state: inside
fetchIssueCounts ensure that when repos.length === 0 you call setIssueCounts({})
(or an initial empty value) before returning, and in the catch block call
setIssueCounts({}) as well as setIsLoadingIssueCounts(false); additionally, in
the useEffect that watches repositories, if repositories.length === 0 call
setIssueCounts({}) to proactively clear counts when the repo list is emptied
(refer to fetchIssueCounts, setIssueCounts, setIsLoadingIssueCounts, and the
repositories effect).
In `@src/renderer/stores/settingsStore.ts`:
- Around line 714-719: The allowed layout literals are duplicated; extract the
array ['mindmap','radial','force'] into a single constant (e.g.,
ALLOWED_DOCUMENT_GRAPH_LAYOUTS: DocumentGraphLayoutType[]) defined near the top
of the module and use that constant in setDocumentGraphLayoutType (replace the
local valid variable) and the corresponding loader code (the other occurrence
around lines 1622-1627) so both places use
ALLOWED_DOCUMENT_GRAPH_LAYOUTS.includes(value) and default to 'mindmap' if
invalid.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (52)
src/__tests__/main/ipc/handlers/symphony.test.tssrc/__tests__/renderer/components/DocumentGraph/DocumentGraphView.test.tsxsrc/__tests__/renderer/components/DocumentGraph/mindMapLayouts.test.tssrc/__tests__/renderer/components/FilePreview.test.tsxsrc/__tests__/renderer/components/GroupChatHeader.test.tsxsrc/__tests__/renderer/components/RightPanel.test.tsxsrc/__tests__/renderer/components/SymphonyModal.test.tsxsrc/__tests__/renderer/components/TerminalOutput.test.tsxsrc/__tests__/renderer/components/auto-scroll.test.tsxsrc/__tests__/renderer/hooks/batch/useAutoRunHandlers.worktree.test.tssrc/__tests__/renderer/hooks/useBatchProcessor.test.tssrc/__tests__/renderer/hooks/useCycleSession.test.tssrc/__tests__/renderer/hooks/useSettings.test.tssrc/__tests__/renderer/stores/settingsStore.test.tssrc/__tests__/renderer/utils/fileExplorer.test.tssrc/__tests__/renderer/utils/platformUtils.test.tssrc/__tests__/renderer/utils/worktreeDedup.test.tssrc/__tests__/shared/symphony-constants.test.tssrc/__tests__/shared/symphony-types.test.tssrc/main/agents/path-prober.tssrc/main/ipc/handlers/filesystem.tssrc/main/ipc/handlers/symphony.tssrc/main/preload/fs.tssrc/main/preload/symphony.tssrc/renderer/App.tsxsrc/renderer/components/DocumentGraph/DocumentGraphView.tsxsrc/renderer/components/DocumentGraph/MindMap.tsxsrc/renderer/components/DocumentGraph/mindMapLayouts.tssrc/renderer/components/FileExplorerPanel.tsxsrc/renderer/components/FilePreview.tsxsrc/renderer/components/GroupChatHeader.tsxsrc/renderer/components/GroupChatPanel.tsxsrc/renderer/components/RightPanel.tsxsrc/renderer/components/SessionList.tsxsrc/renderer/components/SettingsModal.tsxsrc/renderer/components/SymphonyModal.tsxsrc/renderer/components/TerminalOutput.tsxsrc/renderer/global.d.tssrc/renderer/hooks/batch/useAutoRunHandlers.tssrc/renderer/hooks/batch/useBatchProcessor.tssrc/renderer/hooks/batch/useWorktreeManager.tssrc/renderer/hooks/session/useCycleSession.tssrc/renderer/hooks/settings/useSettings.tssrc/renderer/hooks/symphony/useSymphony.tssrc/renderer/hooks/worktree/useWorktreeHandlers.tssrc/renderer/stores/settingsStore.tssrc/renderer/types/index.tssrc/renderer/utils/fileExplorer.tssrc/renderer/utils/platformUtils.tssrc/renderer/utils/worktreeDedup.tssrc/shared/symphony-constants.tssrc/shared/symphony-types.ts
💤 Files with no reviewable changes (2)
- src/renderer/components/GroupChatPanel.tsx
- src/main/agents/path-prober.ts
Comment on lines
796
to
802
| notifyToast({ | ||
| type: 'info', | ||
| title: 'Auto Run Started', | ||
| message: `${initialTotalTasks} ${initialTotalTasks === 1 ? 'task' : 'tasks'} across ${documents.length} ${documents.length === 1 ? 'document' : 'documents'}`, | ||
| project: session.name, | ||
| sessionId, | ||
| }); |
There was a problem hiding this comment.
Toast metadata key mismatch prevents session attribution.
notifyToast consumes agentSessionId metadata, but this call passes sessionId. The toast still renders, but session-linked logging/notification context is lost.
🔧 Proposed fix
notifyToast({
type: 'info',
title: 'Auto Run Started',
message: `${initialTotalTasks} ${initialTotalTasks === 1 ? 'task' : 'tasks'} across ${documents.length} ${documents.length === 1 ? 'document' : 'documents'}`,
project: session.name,
- sessionId,
+ agentSessionId: sessionId,
});📝 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
| notifyToast({ | |
| type: 'info', | |
| title: 'Auto Run Started', | |
| message: `${initialTotalTasks} ${initialTotalTasks === 1 ? 'task' : 'tasks'} across ${documents.length} ${documents.length === 1 ? 'document' : 'documents'}`, | |
| project: session.name, | |
| sessionId, | |
| }); | |
| notifyToast({ | |
| type: 'info', | |
| title: 'Auto Run Started', | |
| message: `${initialTotalTasks} ${initialTotalTasks === 1 ? 'task' : 'tasks'} across ${documents.length} ${documents.length === 1 ? 'document' : 'documents'}`, | |
| project: session.name, | |
| agentSessionId: sessionId, | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/hooks/batch/useBatchProcessor.ts` around lines 796 - 802, The
toast call to notifyToast in useBatchProcessor.ts uses the wrong metadata key:
it passes sessionId instead of the expected agentSessionId, so session-linked
logging is lost; update the notifyToast invocation that constructs the auto-run
toast (the block calling notifyToast with title 'Auto Run Started') to replace
the sessionId property with agentSessionId: pass agentSessionId: sessionId (or
rename the variable if appropriate) so the toast metadata matches notifyToast's
contract and restores session attribution.
Greptile SummaryThis is a large polish/RC release (52 files) that bundles several features, fixes, and refactors across the Maestro codebase. The most significant changes are:
One logic bug found: In Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant UI as SymphonyModal
participant Hook as useSymphony
participant IPC as symphony:getIssueCounts
participant GH as GitHub Search API
participant Cache as SymphonyCache
UI->>Hook: repositories loaded
Hook->>IPC: getIssueCounts(repoSlugs)
IPC->>Cache: readCache(app)
alt Cache valid (< 30 min)
Cache-->>IPC: issueCounts.data
IPC-->>Hook: { counts, fromCache: true }
else Cache stale or missing
IPC->>GH: GET /search/issues?q=label:runmaestro.ai+state:open+repo:A+repo:B&per_page=100
GH-->>IPC: { total_count, items[repository_url] }
IPC->>IPC: count items per slug (case-sensitive ⚠️)
IPC->>Cache: writeCache(newCache)
IPC-->>Hook: { counts, fromCache: false }
end
Hook-->>UI: issueCounts, isLoadingIssueCounts
note over UI: RepositoryTile shows count badge
note over UI: opacity 0.45 when count === 0
Last reviewed commit: ad9d63a |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Additional Comments (3)
The // Normalize casing when matching slugs
const slugLower = slug.toLowerCase();
const matchingSlug = repoSlugs.find((s) => s.toLowerCase() === slugLower);
if (matchingSlug !== undefined) {
counts[matchingSlug]++;
}Alternatively, normalize
The GitHub Search API returns at most Consider paginating (following
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time! |
pedramamini
added a commit
that referenced
this pull request
Mar 1, 2026
… a11y, formatting - readFile ENOENT null return: update global.d.ts type to string | null, add null guards across 12 call sites (MarkdownRenderer, FilePreview, AutoRun, DocumentEditor, DocumentGenerationView, ImageDiffViewer, DocumentGraphView, DebugWizardModal, useAutoRunImageHandling, useFileExplorerEffects, useTabHandlers, useAppHandlers) - Symphony issue counts: add pagination (up to 1,000 results), validate cache against requested repoSlugs, clear stale counts on failure/empty - Worktree fixes: clear dedup marker on worktreeSetup throw, propagate targetBranch in PR creation catch path - GroupChatHeader: add keyboard a11y (tabIndex, role, onKeyDown) - Settings: extract DOCUMENT_GRAPH_LAYOUT_TYPES constant, dedup validation - DocumentGraphView: sync layoutType state with defaultLayoutType prop - Tests: fix vi.hoisted() TDZ, proper Shortcut mock shape, clearAllMocks, assert console.warn in dedup tests, update symphony cache type test - Prettier: format all CI-flagged files
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
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.
- 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
…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
Cache no longer keys on lookbackDays — once generated, notes persist until explicit Regenerate click. During regeneration, old notes stay visible and scrollable with stats bar, timestamp, and Save/Copy buttons remaining enabled. Progress bar only shows on first-ever generation. Renamed "Refresh" to "Regenerate" for clearer intent.
Add ~37 missing props to useMainPanelProps interface, useMemo return, and dependency array. Remove state props from useRightPanelProps call in App.tsx that the refactored hook no longer accepts (RightPanel now reads state directly from Zustand stores).
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/renderer/hooks/batch/useBatchProcessor.ts (1)
792-799:⚠️ Potential issue | 🟡 MinorUse
agentSessionIdin toast metadata (still unresolved).At Line 798,
notifyToastis still passedsessionIdinstead ofagentSessionId, which can break session-linked attribution for notifications. This appears to be the same issue previously reported; please also update the related assertion insrc/__tests__/renderer/hooks/useBatchProcessor.test.ts(toast expectation block around Lines 5799-5804).🔧 Proposed fix
notifyToast({ type: 'info', title: 'Auto Run Started', message: `${initialTotalTasks} ${initialTotalTasks === 1 ? 'task' : 'tasks'} across ${documents.length} ${documents.length === 1 ? 'document' : 'documents'}`, project: session.name, - sessionId, + agentSessionId: sessionId, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/hooks/batch/useBatchProcessor.ts` around lines 792 - 799, The toast metadata is using sessionId instead of agentSessionId; update the notifyToast call in useBatchProcessor (the call to notifyToast that sets project/sessionId) to pass agentSessionId as the sessionId field, and then update the corresponding assertion in useBatchProcessor.test.ts (the toast expectation block) to assert agentSessionId is present in the toast metadata instead of sessionId.
🧹 Nitpick comments (1)
src/renderer/components/DocumentGraph/DocumentGraphView.tsx (1)
1357-1414: Consider addingaria-expandedand keyboard navigation for accessibility.The layout dropdown button lacks
aria-expandedandaria-haspopupattributes, and the dropdown items don't support arrow-key navigation. While functional, these additions would improve keyboard accessibility.♿ Suggested accessibility improvements
<button onClick={() => setShowLayoutDropdown(!showLayoutDropdown)} className="flex items-center gap-1.5 px-3 py-1.5 rounded text-sm transition-colors" + aria-expanded={showLayoutDropdown} + aria-haspopup="listbox" style={{ backgroundColor: `${theme.colors.accent}10`, 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/DocumentGraph/DocumentGraphView.tsx` around lines 1357 - 1414, The layout dropdown lacks accessibility attributes and keyboard support: update the toggle button (where setShowLayoutDropdown and showLayoutDropdown are used) to include aria-haspopup="menu" and aria-expanded tied to showLayoutDropdown, and add an onKeyDown handler on the button to open/close on Enter/Space and move focus into the list; mark the dropdown container with role="menu" and each mapped button (the items that call handleLayoutTypeChange and read layoutType/LAYOUT_LABELS) with role="menuitem" and make them keyboard-focusable (manage tabIndex or programmatic focus), then implement arrow-up/arrow-down handling to move focus between items and Enter/Space to select so keyboard-only users can navigate the layout selector.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/ipc/handlers/symphony.ts`:
- Around line 1279-1286: The fallback currently returns cache.issueCounts.data
without ensuring it corresponds to the requested repo set; update the
catch/fallback to compare the requested repoSlugs (the input parameter or local
variable representing the requested repos) with cache.issueCounts.repoSlugs
(and/or a normalized sorted representation) before returning counts, and only
return { counts: cache.issueCounts.data, fromCache: true, cacheAge } when they
match; if they don’t match, avoid returning the cached counts (e.g., throw the
error or return an explicit miss) so callers don’t receive counts for the wrong
repo set.
- Around line 525-536: The query currently builds repoQualifiers by joining
repoSlugs with '+' which creates an implicit AND; update repoQualifiers to build
an OR-grouped expression like `(repo:owner1/repo1 OR repo:owner2/repo2 ...)`
(use repoSlugs to create each `repo:${slug}` and join with ' OR '), then build
the full query string using SYMPHONY_ISSUE_LABEL and state:open, wrap the entire
query in parentheses where appropriate, and pass the entire assembled query
through encodeURIComponent when constructing `query`/the URL so the Search API
sees the grouped OR expression correctly; adjust the code around the
`repoQualifiers`, `query`, and URL construction to use this encoded grouped OR
form.
In `@src/renderer/components/ImageDiffViewer.tsx`:
- Around line 58-60: The current truthy guard around setNewImage after calling
window.maestro.fs.readFile can leave a stale "after" image when read returns
null; update the logic in the try/catch surrounding
window.maestro.fs.readFile(fullPath, sshRemoteId) so that when content is falsy
(null/undefined) you explicitly clear the "after" image state (call
setNewImage(null) or equivalent) and also ensure the catch block likewise clears
setNewImage and/or sets an error state; update references to setNewImage and the
read call in ImageDiffViewer.tsx to implement this behavior so a failed or empty
read never leaves the previous image displayed.
---
Duplicate comments:
In `@src/renderer/hooks/batch/useBatchProcessor.ts`:
- Around line 792-799: The toast metadata is using sessionId instead of
agentSessionId; update the notifyToast call in useBatchProcessor (the call to
notifyToast that sets project/sessionId) to pass agentSessionId as the sessionId
field, and then update the corresponding assertion in useBatchProcessor.test.ts
(the toast expectation block) to assert agentSessionId is present in the toast
metadata instead of sessionId.
---
Nitpick comments:
In `@src/renderer/components/DocumentGraph/DocumentGraphView.tsx`:
- Around line 1357-1414: The layout dropdown lacks accessibility attributes and
keyboard support: update the toggle button (where setShowLayoutDropdown and
showLayoutDropdown are used) to include aria-haspopup="menu" and aria-expanded
tied to showLayoutDropdown, and add an onKeyDown handler on the button to
open/close on Enter/Space and move focus into the list; mark the dropdown
container with role="menu" and each mapped button (the items that call
handleLayoutTypeChange and read layoutType/LAYOUT_LABELS) with role="menuitem"
and make them keyboard-focusable (manage tabIndex or programmatic focus), then
implement arrow-up/arrow-down handling to move focus between items and
Enter/Space to select so keyboard-only users can navigate the layout selector.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (31)
src/__tests__/renderer/components/DocumentGraph/mindMapLayouts.test.tssrc/__tests__/renderer/components/GroupChatHeader.test.tsxsrc/__tests__/renderer/components/TerminalOutput.test.tsxsrc/__tests__/renderer/components/auto-scroll.test.tsxsrc/__tests__/renderer/hooks/useBatchProcessor.test.tssrc/__tests__/renderer/utils/fileExplorer.test.tssrc/__tests__/shared/symphony-types.test.tssrc/main/ipc/handlers/symphony.tssrc/renderer/App.tsxsrc/renderer/components/AutoRun.tsxsrc/renderer/components/DebugWizardModal.tsxsrc/renderer/components/DocumentGraph/DocumentGraphView.tsxsrc/renderer/components/DocumentGraph/mindMapLayouts.tssrc/renderer/components/FilePreview.tsxsrc/renderer/components/GroupChatHeader.tsxsrc/renderer/components/ImageDiffViewer.tsxsrc/renderer/components/InlineWizard/DocumentGenerationView.tsxsrc/renderer/components/MarkdownRenderer.tsxsrc/renderer/components/SymphonyModal.tsxsrc/renderer/components/Wizard/shared/DocumentEditor.tsxsrc/renderer/global.d.tssrc/renderer/hooks/batch/useAutoRunHandlers.tssrc/renderer/hooks/batch/useAutoRunImageHandling.tssrc/renderer/hooks/batch/useBatchProcessor.tssrc/renderer/hooks/batch/useWorktreeManager.tssrc/renderer/hooks/git/useFileExplorerEffects.tssrc/renderer/hooks/symphony/useSymphony.tssrc/renderer/hooks/tabs/useTabHandlers.tssrc/renderer/hooks/ui/useAppHandlers.tssrc/renderer/stores/settingsStore.tssrc/shared/symphony-types.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- src/tests/renderer/components/auto-scroll.test.tsx
- src/tests/shared/symphony-types.test.ts
- src/tests/renderer/utils/fileExplorer.test.ts
- src/tests/renderer/components/GroupChatHeader.test.tsx
- src/renderer/hooks/batch/useWorktreeManager.ts
- src/renderer/hooks/symphony/useSymphony.ts
- src/renderer/hooks/batch/useAutoRunHandlers.ts
Comment on lines
525
to
536
| const repoQualifiers = repoSlugs.map((s) => `repo:${s}`).join('+'); | ||
| const query = `label:${encodeURIComponent(SYMPHONY_ISSUE_LABEL)}+state:open+${repoQualifiers}`; | ||
| // Initialize all slugs to 0, then count from paginated results | ||
| const counts: Record<string, number> = {}; | ||
| for (const slug of repoSlugs) { | ||
| counts[slug] = 0; | ||
| } | ||
|
|
||
| let page = 1; | ||
| while (true) { | ||
| const url = `${GITHUB_API_BASE}/search/issues?q=${query}&per_page=100&page=${page}`; | ||
| const response = await fetch(url, { |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
GitHub Search API multiple repositories repo qualifier syntax AND OR
💡 Result:
For the GitHub Search API (e.g., GET /search/code?q=...), the q string uses GitHub’s search syntax:
- Whitespace =
ANDfor code search (e.g.,foo barmeansfoo AND bar).AND,OR, andNOTare supported, and you can group with parentheses. [1] - A single item can’t be in two repos at once, so to search multiple repositories you must combine
repo:qualifiers withOR(optionally grouped with parentheses). [1]
Examples:
- Two repos:
q=searchterm (repo:owner1/repo1 OR repo:owner2/repo2)[1]
- Many repos:
q=searchterm (repo:a/r1 OR repo:a/r2 OR repo:b/r3)[1]
- Alternative (broader) scopes:
q=searchterm org:my-orgorq=searchterm user:my-user[1]
Sources:
[1] Understanding GitHub Code Search syntax (GitHub Docs) https://archive.vn/2026.01.11-051612/https%3A/docs.github.com/en/search-github/github-code-search/understanding-github-code-search-syntax
Use explicit OR grouping for multi-repository Search API queries.
At Line 525–526, concatenating multiple repo: qualifiers with + creates an implicit AND condition. GitHub's Search API treats whitespace (which + decodes to) as AND, making it impossible for a single issue to satisfy multiple repo: constraints simultaneously. This results in zero matches instead of the intended multi-repository query.
Multiple repositories must be combined with explicit OR and grouped with parentheses. Additionally, the entire query string should be URL-encoded as a single unit.
Suggested fix
- const repoQualifiers = repoSlugs.map((s) => `repo:${s}`).join('+');
- const query = `label:${encodeURIComponent(SYMPHONY_ISSUE_LABEL)}+state:open+${repoQualifiers}`;
+ const repoExpression = repoSlugs.map((s) => `repo:${s}`).join(' OR ');
+ const query = encodeURIComponent(
+ `label:${SYMPHONY_ISSUE_LABEL} state:open (${repoExpression})`
+ );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/ipc/handlers/symphony.ts` around lines 525 - 536, The query
currently builds repoQualifiers by joining repoSlugs with '+' which creates an
implicit AND; update repoQualifiers to build an OR-grouped expression like
`(repo:owner1/repo1 OR repo:owner2/repo2 ...)` (use repoSlugs to create each
`repo:${slug}` and join with ' OR '), then build the full query string using
SYMPHONY_ISSUE_LABEL and state:open, wrap the entire query in parentheses where
appropriate, and pass the entire assembled query through encodeURIComponent when
constructing `query`/the URL so the Search API sees the grouped OR expression
correctly; adjust the code around the `repoQualifiers`, `query`, and URL
construction to use this encoded grouped OR form.
Comment on lines
1279
to
1286
| // Fallback to expired cache if available | ||
| if (cache?.issueCounts?.data) { | ||
| const cacheAge = Date.now() - cache.issueCounts.fetchedAt; | ||
| return { | ||
| counts: cache.issueCounts.data, | ||
| fromCache: true, | ||
| cacheAge, | ||
| }; |
There was a problem hiding this comment.
Fallback path can return counts for the wrong repo set.
If fresh fetch fails, the catch block returns cache.issueCounts.data without validating that cached repoSlugs matches the current request. That can surface incorrect counts.
💡 Suggested fix
- // Fallback to expired cache if available
- if (cache?.issueCounts?.data) {
+ // Fallback to expired cache only if slug set still matches request
+ const fallbackSlugsMatch =
+ cache?.issueCounts?.repoSlugs &&
+ requestedSlugs.length === cache.issueCounts.repoSlugs.length &&
+ requestedSlugs.every((s) => cache.issueCounts!.repoSlugs.includes(s));
+ if (cache?.issueCounts?.data && fallbackSlugsMatch) {
const cacheAge = Date.now() - cache.issueCounts.fetchedAt;
return {
counts: cache.issueCounts.data,
fromCache: true,
cacheAge,
};
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/ipc/handlers/symphony.ts` around lines 1279 - 1286, The fallback
currently returns cache.issueCounts.data without ensuring it corresponds to the
requested repo set; update the catch/fallback to compare the requested repoSlugs
(the input parameter or local variable representing the requested repos) with
cache.issueCounts.repoSlugs (and/or a normalized sorted representation) before
returning counts, and only return { counts: cache.issueCounts.data, fromCache:
true, cacheAge } when they match; if they don’t match, avoid returning the
cached counts (e.g., throw the error or return an explicit miss) so callers
don’t receive counts for the wrong repo set.
Comment on lines
58
to
60
| const content = await window.maestro.fs.readFile(fullPath, sshRemoteId); | ||
| setNewImage(content); | ||
| if (content) setNewImage(content); | ||
| } catch (err) { |
There was a problem hiding this comment.
Truthy guard can leave stale “After” image displayed.
At Line 59, skipping setNewImage when content is null keeps the previous image in state. This can show the wrong file after a failed read.
💡 Suggested fix
- const content = await window.maestro.fs.readFile(fullPath, sshRemoteId);
- if (content) setNewImage(content);
+ const content = await window.maestro.fs.readFile(fullPath, sshRemoteId);
+ if (content === null) {
+ setNewImage(null);
+ } else {
+ setNewImage(content);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const content = await window.maestro.fs.readFile(fullPath, sshRemoteId); | |
| setNewImage(content); | |
| if (content) setNewImage(content); | |
| } catch (err) { | |
| const content = await window.maestro.fs.readFile(fullPath, sshRemoteId); | |
| if (content === null) { | |
| setNewImage(null); | |
| } else { | |
| setNewImage(content); | |
| } | |
| } catch (err) { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/components/ImageDiffViewer.tsx` around lines 58 - 60, The
current truthy guard around setNewImage after calling window.maestro.fs.readFile
can leave a stale "after" image when read returns null; update the logic in the
try/catch surrounding window.maestro.fs.readFile(fullPath, sshRemoteId) so that
when content is falsy (null/undefined) you explicitly clear the "after" image
state (call setNewImage(null) or equivalent) and also ensure the catch block
likewise clears setNewImage and/or sets an error state; update references to
setNewImage and the read call in ImageDiffViewer.tsx to implement this behavior
so a failed or empty read never leaves the previous image displayed.
…l, Force-Directed) Adds a dropdown in the Document Graph header to switch between three layout algorithms. Layout choice persists per-agent (via Session) with global default fallback (via settingsStore). - Extract layout code from MindMap.tsx into mindMapLayouts.ts - Add Radial layout (concentric rings) and Force-Directed layout (d3-force) - Add layout type dropdown UI with layer stack Escape handling - Wire persistence through App.tsx (session + global default) - Add tests for all three algorithms, settingsStore, and props
…provements - Fix ESLint errors: empty catch block comment, unused eslint-disable directive, unused variable prefixing in App.tsx and DirectorNotesModal - Hide auto-scroll button when tab has no content (filteredLogs.length > 0 guard) - Fix worktree auto-run: skip setupWorktree when worktreeTarget already created the worktree, preventing "belongs to different repository" false positive - Add "Auto Run Started" toast notification with task/document counts - Record PR creation results in agent history panel - Update auto-scroll and batch processor tests for new behavior
Uses a single GitHub Search API call to batch-fetch open issue counts for all registered repos (instead of 2 calls per repo). Tiles now show "View #N Issues" when issues exist, "No Issues" (ghosted at 45% opacity) when none, and the original "View Issues" while counts are loading. - New IPC handler: symphony:getIssueCounts (30-min cache TTL) - New fetchIssueCounts() using /search/issues with multiple repo: qualifiers - Counts fetched automatically when registry loads - Updated RepositoryTile, useSymphony hook, preload bridge, global.d.ts
… a11y, formatting - readFile ENOENT null return: update global.d.ts type to string | null, add null guards across 12 call sites (MarkdownRenderer, FilePreview, AutoRun, DocumentEditor, DocumentGenerationView, ImageDiffViewer, DocumentGraphView, DebugWizardModal, useAutoRunImageHandling, useFileExplorerEffects, useTabHandlers, useAppHandlers) - Symphony issue counts: add pagination (up to 1,000 results), validate cache against requested repoSlugs, clear stale counts on failure/empty - Worktree fixes: clear dedup marker on worktreeSetup throw, propagate targetBranch in PR creation catch path - GroupChatHeader: add keyboard a11y (tabIndex, role, onKeyDown) - Settings: extract DOCUMENT_GRAPH_LAYOUT_TYPES constant, dedup validation - DocumentGraphView: sync layoutType state with defaultLayoutType prop - Tests: fix vi.hoisted() TDZ, proper Shortcut mock shape, clearAllMocks, assert console.warn in dedup tests, update symphony cache type test - Prettier: format all CI-flagged files
pedramamini
force-pushed
the
0.15.0-RC-polish-round-two
branch
from
435aba8 to
293180c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/renderer/hooks/batch/useWorktreeManager.ts (1)
356-365:⚠️ Potential issue | 🟡 Minor
targetBranchcan still be undefined on early default-branch lookup failureAt Line 360, if
getDefaultBranch()throws, control jumps to the outercatchbefore Line 361-364 assigns a fallback, so Line 412 returnstargetBranch: undefined. That weakens the new “always propagate target branch” behavior.💡 Proposed fix
- let baseBranch: string | undefined = worktree.prTargetBranch; + let baseBranch: string = worktree.prTargetBranch ?? 'main'; try { // Use the user-selected target branch, or fall back to default branch detection - if (!baseBranch) { - const defaultBranchResult = await window.maestro.git.getDefaultBranch(mainRepoCwd); - baseBranch = - defaultBranchResult.success && defaultBranchResult.branch - ? defaultBranchResult.branch - : 'main'; + if (!worktree.prTargetBranch) { + try { + const defaultBranchResult = await window.maestro.git.getDefaultBranch(mainRepoCwd); + if (defaultBranchResult.success && defaultBranchResult.branch) { + baseBranch = defaultBranchResult.branch; + } + } catch (err) { + // Keep default 'main' when branch detection fails + captureException(err, { extra: { mainRepoCwd, operation: 'git.getDefaultBranch' } }); + } }Also applies to: 407-413
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/hooks/batch/useWorktreeManager.ts` around lines 356 - 365, The code can leave baseBranch/targetBranch undefined if window.maestro.git.getDefaultBranch throws; wrap the default-branch lookup in its own try/catch and ensure you assign a fallback (e.g., 'main') on error so baseBranch is always defined afterwards. Specifically, update the block using baseBranch and window.maestro.git.getDefaultBranch in useWorktreeManager (the local variable baseBranch and the awaited getDefaultBranch call) to catch errors from getDefaultBranch and set baseBranch = 'main' (or the desired default) in the catch. Apply the same pattern to the other similar lookup (the block around the other getDefaultBranch usage referenced at lines ~407-413) so targetBranch cannot be left undefined.
♻️ Duplicate comments (1)
src/main/ipc/handlers/symphony.ts (1)
1279-1286:⚠️ Potential issue | 🟠 MajorFallback can still return counts for the wrong slug set.
At Line [1280], the expired-cache fallback does not validate
repoSlugsagainstrequestedSlugs, so mismatched counts can still be returned after fetch failures.Suggested fix
} catch (error) { logger.warn('Failed to fetch issue counts from GitHub Search API', LOG_CONTEXT, { error, }); - // Fallback to expired cache if available - if (cache?.issueCounts?.data) { + // Fallback to expired cache only when slug set matches request + const fallbackSlugsMatch = + cache?.issueCounts?.repoSlugs && + requestedSlugs.length === cache.issueCounts.repoSlugs.length && + requestedSlugs.every((s) => cache.issueCounts!.repoSlugs.includes(s)); + if (cache?.issueCounts?.data && fallbackSlugsMatch) { const cacheAge = Date.now() - cache.issueCounts.fetchedAt; return { counts: cache.issueCounts.data, fromCache: true, cacheAge, }; } throw error; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/ipc/handlers/symphony.ts` around lines 1279 - 1286, The expired-cache fallback is returning counts without verifying they match the requested repo slugs, so update the fallback in the handler (where cache.issueCounts is used) to compare the cached repoSlugs with the current requestedSlugs (or perform set equality) before returning counts; only return the cached counts (with cacheAge) when the cached issueCounts.repoSlugs exactly match requestedSlugs, otherwise treat as no valid cache and propagate the fetch failure path. Ensure you reference cache.issueCounts, cache.issueCounts.fetchedAt, repoSlugs and requestedSlugs when adding the check.
🤖 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/hooks/symphony/useSymphony.ts`:
- Around line 199-202: The empty-repo reset (setIssueCounts(null)) is never
reached because the effect early-returns when the guard if (repositories.length
> 0) prevents calling fetchIssueCounts([]); update the effect that references
repositories and fetchIssueCounts so it explicitly handles the empty case: when
repositories.length === 0 call setIssueCounts(null) (or call
fetchIssueCounts([]) if you prefer consistent flow) before the early-return,
otherwise proceed to call fetchIssueCounts(repositories); adjust the guard
around repositories so that fetchIssueCounts and setIssueCounts are always
invoked for the empty-repo path (affecting the effect containing repositories,
fetchIssueCounts, and setIssueCounts).
---
Outside diff comments:
In `@src/renderer/hooks/batch/useWorktreeManager.ts`:
- Around line 356-365: The code can leave baseBranch/targetBranch undefined if
window.maestro.git.getDefaultBranch throws; wrap the default-branch lookup in
its own try/catch and ensure you assign a fallback (e.g., 'main') on error so
baseBranch is always defined afterwards. Specifically, update the block using
baseBranch and window.maestro.git.getDefaultBranch in useWorktreeManager (the
local variable baseBranch and the awaited getDefaultBranch call) to catch errors
from getDefaultBranch and set baseBranch = 'main' (or the desired default) in
the catch. Apply the same pattern to the other similar lookup (the block around
the other getDefaultBranch usage referenced at lines ~407-413) so targetBranch
cannot be left undefined.
---
Duplicate comments:
In `@src/main/ipc/handlers/symphony.ts`:
- Around line 1279-1286: The expired-cache fallback is returning counts without
verifying they match the requested repo slugs, so update the fallback in the
handler (where cache.issueCounts is used) to compare the cached repoSlugs with
the current requestedSlugs (or perform set equality) before returning counts;
only return the cached counts (with cacheAge) when the cached
issueCounts.repoSlugs exactly match requestedSlugs, otherwise treat as no valid
cache and propagate the fetch failure path. Ensure you reference
cache.issueCounts, cache.issueCounts.fetchedAt, repoSlugs and requestedSlugs
when adding the check.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (31)
src/__tests__/renderer/components/DocumentGraph/mindMapLayouts.test.tssrc/__tests__/renderer/components/GroupChatHeader.test.tsxsrc/__tests__/renderer/components/TerminalOutput.test.tsxsrc/__tests__/renderer/components/auto-scroll.test.tsxsrc/__tests__/renderer/hooks/useBatchProcessor.test.tssrc/__tests__/renderer/utils/fileExplorer.test.tssrc/__tests__/shared/symphony-types.test.tssrc/main/ipc/handlers/symphony.tssrc/renderer/App.tsxsrc/renderer/components/AutoRun.tsxsrc/renderer/components/DebugWizardModal.tsxsrc/renderer/components/DocumentGraph/DocumentGraphView.tsxsrc/renderer/components/DocumentGraph/mindMapLayouts.tssrc/renderer/components/FilePreview.tsxsrc/renderer/components/GroupChatHeader.tsxsrc/renderer/components/ImageDiffViewer.tsxsrc/renderer/components/InlineWizard/DocumentGenerationView.tsxsrc/renderer/components/MarkdownRenderer.tsxsrc/renderer/components/SymphonyModal.tsxsrc/renderer/components/Wizard/shared/DocumentEditor.tsxsrc/renderer/global.d.tssrc/renderer/hooks/batch/useAutoRunHandlers.tssrc/renderer/hooks/batch/useAutoRunImageHandling.tssrc/renderer/hooks/batch/useBatchProcessor.tssrc/renderer/hooks/batch/useWorktreeManager.tssrc/renderer/hooks/git/useFileExplorerEffects.tssrc/renderer/hooks/symphony/useSymphony.tssrc/renderer/hooks/tabs/useTabHandlers.tssrc/renderer/hooks/ui/useAppHandlers.tssrc/renderer/stores/settingsStore.tssrc/shared/symphony-types.ts
✅ Files skipped from review due to trivial changes (2)
- src/tests/renderer/components/TerminalOutput.test.tsx
- src/renderer/components/SymphonyModal.tsx
🚧 Files skipped from review as they are similar to previous changes (18)
- src/renderer/components/GroupChatHeader.tsx
- src/tests/renderer/hooks/useBatchProcessor.test.ts
- src/renderer/components/FilePreview.tsx
- src/renderer/components/AutoRun.tsx
- src/tests/renderer/components/auto-scroll.test.tsx
- src/renderer/global.d.ts
- src/renderer/hooks/batch/useAutoRunHandlers.ts
- src/renderer/hooks/batch/useBatchProcessor.ts
- src/renderer/stores/settingsStore.ts
- src/tests/renderer/components/DocumentGraph/mindMapLayouts.test.ts
- src/renderer/hooks/git/useFileExplorerEffects.ts
- src/tests/renderer/utils/fileExplorer.test.ts
- src/tests/shared/symphony-types.test.ts
- src/renderer/components/DocumentGraph/mindMapLayouts.ts
- src/renderer/hooks/tabs/useTabHandlers.ts
- src/shared/symphony-types.ts
- src/renderer/components/DocumentGraph/DocumentGraphView.tsx
- src/renderer/components/InlineWizard/DocumentGenerationView.tsx
Comment on lines
+199
to
+202
| if (repos.length === 0) { | ||
| setIssueCounts(null); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Empty-repo reset path is not reached from the current effect flow.
At Line [225], the guard if (repositories.length > 0) prevents calling fetchIssueCounts([]), so the new clear-at-empty logic at Line [199] won’t run and stale counts can persist when repositories become empty.
Suggested fix
const fetchIssueCounts = useCallback(async (repos: RegisteredRepository[]) => {
if (repos.length === 0) {
setIssueCounts(null);
+ setIsLoadingIssueCounts(false);
return;
}
setIsLoadingIssueCounts(true);
@@
// Fetch issue counts once repositories are loaded
useEffect(() => {
- if (repositories.length > 0) {
- fetchIssueCounts(repositories);
- }
+ fetchIssueCounts(repositories);
}, [repositories, fetchIssueCounts]);Also applies to: 225-228
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/hooks/symphony/useSymphony.ts` around lines 199 - 202, The
empty-repo reset (setIssueCounts(null)) is never reached because the effect
early-returns when the guard if (repositories.length > 0) prevents calling
fetchIssueCounts([]); update the effect that references repositories and
fetchIssueCounts so it explicitly handles the empty case: when
repositories.length === 0 call setIssueCounts(null) (or call
fetchIssueCounts([]) if you prefer consistent flow) before the early-return,
otherwise proceed to call fetchIssueCounts(repositories); adjust the guard
around repositories so that fetchIssueCounts and setIssueCounts are always
invoked for the empty-repo path (affecting the effect containing repositories,
fetchIssueCounts, and setIssueCounts).
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 by CodeRabbit
Release Notes
New Features
Bug Fixes
Refactors