refactor: extracted 3 domain-specific hooks from App.tsx#470
Merged
refactor: extracted 3 domain-specific hooks from App.tsx#470
Conversation
Extract all wizard-related handlers into a dedicated hook module: - Slash command discovery effect - Wizard state sync effect (inline context → tab state) - sendWizardMessageWithThinking (thinking chunk routing) - handleHistoryCommand (/history slash command) - handleSkillsCommand (/skills slash command) - handleWizardCommand (/wizard slash command) - handleLaunchWizardTab (wizard in new tab) - handleWizardComplete (wizard → normal session) - handleWizardLetsGo (document generation) - handleToggleWizardShowThinking - handleWizardLaunchSession (onboarding wizard → session) Uses ref pattern for circular deps with useAutoRunHandlers/useInputHandlers. Imports real types (UseInlineWizardReturn, WizardState, HistoryEntryInput, AgentSpawnResult) instead of hand-crafted local types. 58 new tests covering all handlers and edge cases. App.tsx: 6,441 → 5,311 lines (-1,130).
Extract session CRUD operations (create, delete, rename, bookmark, drag-drop, group-move) into dedicated useSessionCrud hook with 52 tests.
Extract process interrupt/stop logic (SIGINT, force-kill fallback, execution queue processing, thinking/tool log cleanup) into dedicated useInterruptHandler hook with 21 tests.
…tern Merge 16 commits from main (features, fixes, polish) into code-refactor branch while preventing Phase 3 regressions: - Restored SessionList direct store subscriptions (Phase 3B) that were reverted by main's prop-centralization approach - Restored MainPanel direct store subscriptions (Phase 3C) similarly - Kept useSessionListProps/useMainPanelProps slim (handlers + computed only) - Applied genuinely new changes: handleUtilityFileTabSelect reuse, GroupChatPanel groups prop, collapsed sidebar ring removal - Updated test for ring-2 removal (CSS fix from main) - Fixed ESLint empty-block in getShellPath.ts - All 21,016 tests pass, TypeScript compiles clean, App.tsx at 4,602 lines
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughMigrates many UI props and setters into centralized Zustand stores, extracts App.tsx logic into three new hooks (useWizardHandlers, useSessionCrud, useInterruptHandler), adds extensive tests for the new hooks, and applies assorted minor fixes, formatting and doc updates across renderer and main code. Changes
Sequence Diagram(s)mermaid UI->>Store: trigger interrupt Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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 SummaryContinued Phase 2 of the App.tsx refactoring by extracting three large domain-specific hooks, reducing Hook Extractions:
Phase 3 Restoration:
Quality Indicators:
Confidence Score: 5/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[App.tsx ~6,600 lines] -->|Phase 2: Extract| B[useWizardHandlers<br/>~1,247 lines]
A -->|Phase 2: Extract| C[useSessionCrud<br/>~472 lines]
A -->|Phase 2: Extract| D[useInterruptHandler<br/>~459 lines]
A -->|Reduced to| E[App.tsx<br/>4,602 lines]
E -->|Phase 3: Props removed| F[SessionList]
E -->|Phase 3: Props removed| G[MainPanel]
F -.->|Direct read| H[(useSessionStore)]
F -.->|Direct read| I[(useUIStore)]
F -.->|Direct read| J[(useSettingsStore)]
F -.->|Direct read| K[(useBatchStore)]
F -.->|Direct read| L[(useGroupChatStore)]
G -.->|Direct read| I
G -.->|Direct read| J
M[useSessionListProps] -.->|Slimmed down<br/>Only handlers| F
N[useMainPanelProps] -.->|Slimmed down<br/>Only handlers| G
B -->|58 tests| O[Test Coverage]
C -->|52 tests| O
D -->|21 tests| O
O -->|131 new tests<br/>21,016 total| P[✓ All Pass]
style A fill:#ffcccc
style E fill:#ccffcc
style B fill:#cce5ff
style C fill:#cce5ff
style D fill:#cce5ff
style F fill:#ffffcc
style G fill:#ffffcc
style P fill:#ccffcc
Last reviewed commit: baedb57 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 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/services/inlineWizardConversation.test.ts`:
- Around line 324-326: Update the mistaken elapsed-time comment next to the
vi.advanceTimersByTimeAsync(600000) call in
src/__tests__/renderer/services/inlineWizardConversation.test.ts (the test that
advances timers) so it correctly reflects the timer math: change the comment to
indicate "40 minutes total, 25+ min since last data" (since 600000 ms advances
from 10 to 40 minutes total, leaving 25 minutes since the last 15-minute mark).
In `@src/renderer/App.tsx`:
- Around line 1593-1618: The deferred processInput() call can fire after
focus/session changes and act on stale targets; fix by capturing the intended
sessionId and tabId when scheduling the timeout, store the timeout id and return
a cleanup that clears it, and before calling processInput() inside the timeout
verify the current activeSession and activeTab still match the captured ids (use
getActiveTab(activeSession) and activeSession?.id comparisons) so you only call
processInput() for the originally intended session/tab; update the useEffect
that uses getActiveTab, setSessions and processInput to implement these checks
and cleanup.
- Around line 2397-2410: The console.log in the App component is printing the
queued payload object (firstItem) which may contain sensitive user content;
update the handler inside the setTimeout that iterates sessionsWithQueuedItems
so it does not log the full executionQueue item—replace the second argument to
console.log in that block with non-sensitive metadata (e.g., session.id,
executionQueue length, firstItem.id or a redacted flag) or remove the object
entirely; ensure no other code paths in the same block reference or stringify
firstItem or executionQueue contents (look for the setTimeout callback,
sessionsWithQueuedItems iteration, and the firstItem variable) and only surface
safe identifiers for debugging.
- Around line 2445-2447: The call to processQueuedItem(session.id, firstItem) is
not handling promise rejections, which can create unhandled rejections after the
item is dequeued and the session is marked busy; wrap the call in an async error
handler (either await inside an async IIFE or attach .catch) and on error: reset
the session busy flag (e.g. call your setSessionBusy/session.setBusy API for
session.id), re-enqueue the item or mark it failed so it is not lost, and log
the error for debugging; use the existing symbols processQueuedItem, session.id
and firstItem to locate the call and ensure the handler performs: try { await
processQueuedItem(...)} catch (err) { reset busy state for session.id; requeue
firstItem or persist failure; log error }.
In `@src/renderer/components/SessionList.tsx`:
- Around line 445-458: The Director’s Notes menu item is being shown because
after switching to getModalActions() the setter setDirectorNotesOpen always
exists; instead gate rendering on the actual feature flag. Update the
SessionList component to check the real Director’s Notes feature flag (e.g., use
the featureFlags or settings hook you have such as isDirectorNotesEnabled or
settingsStore.featureFlags.directorNotes) before rendering the menu entry and
before calling setDirectorNotesOpen; replace the current existence-based
condition that references setDirectorNotesOpen with a boolean feature-flag check
and apply the same change to the other menu items in the 693-719 range that
suffer from this issue.
In `@src/renderer/hooks/agent/useInterruptHandler.ts`:
- Around line 180-209: The no-queue cleanup branch that builds
updatedAiTabsForIdle only resets the active tab and leaves other tabs still
marked 'busy'; update the mapping logic that produces updatedAiTabsForIdle
(using s.aiTabs, canceledLog, activeTabForCancel) so that in both the
canceledLog branch and the else branch you normalize all tabs: for any tab with
state === 'busy' (or the active tab when matched) remove logs with source
'thinking' or 'tool', set state to 'idle' and clear thinkingStartTime; keep the
canceledLog appended only to the activeTabForCancel as currently intended.
- Around line 223-225: The deferred calls to processQueuedItem (invoked inside
setTimeout for queuedItemToProcess and queuedItemAfterKill) can return rejected
promises that become unhandled; update both setTimeout callbacks to call
processQueuedItem(...).catch(err => {/* log or handle error and advance queue
*/}) or use an async IIFE with try/catch to await processQueuedItem and handle
errors explicitly, ensuring errors are logged and the queue progression is
continued; apply this pattern to the block referencing queuedItemToProcess and
the block referencing queuedItemAfterKill so both rejections are handled.
In `@src/renderer/hooks/session/useSessionCrud.ts`:
- Around line 161-164: Update the user-facing toast text to use "agent" instead
of "session": modify the notifyToast call (function notifyToast) that currently
sets title 'Session Creation Failed' and message 'Cannot create duplicate
session' (or uses validation.error) so the title reads something like 'Agent
Creation Failed' and the fallback message uses 'Cannot create duplicate agent';
keep validation.error unchanged if present. Ensure only the user-facing strings
in the notifyToast invocation are changed (leave internal/session variable names
intact).
- Around line 382-404: finishRenamingSession is reading session.agentSessionId
but newly created sessions put provider IDs on aiTabs[*].agentSessionId, so
update the logic to derive the provider session id from aiTabs when
session.agentSessionId is falsy: inside finishRenamingSession after computing
session from updated, look up aTabAgentId = session.agentSessionId ||
session.aiTabs?.find(t => t.active)?.agentSessionId ||
session.aiTabs?.[0]?.agentSessionId and use aTabAgentId (instead of
session.agentSessionId) when deciding to call (window as
any).maestro.claude.updateSessionName or maestro.agentSessions.setSessionName;
keep existing agentId determination via session.toolType || 'claude-code' and
preserve the .catch logging.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
src/__tests__/renderer/components/MainPanel.test.tsxsrc/__tests__/renderer/components/SessionList.test.tsxsrc/__tests__/renderer/hooks/useInterruptHandler.test.tssrc/__tests__/renderer/hooks/useSessionCrud.test.tssrc/__tests__/renderer/hooks/useWizardHandlers.test.tssrc/__tests__/renderer/services/inlineWizardConversation.test.tssrc/main/agents/definitions.tssrc/main/agents/path-prober.tssrc/main/ipc/handlers/git.tssrc/main/runtime/getShellPath.tssrc/renderer/App.tsxsrc/renderer/components/MainPanel.tsxsrc/renderer/components/SessionList.tsxsrc/renderer/components/Settings/IgnorePatternsSection.tsxsrc/renderer/components/SymphonyModal.tsxsrc/renderer/components/UsageDashboard/ChartSkeletons.tsxsrc/renderer/components/UsageDashboard/SummaryCards.tsxsrc/renderer/hooks/agent/index.tssrc/renderer/hooks/agent/useInterruptHandler.tssrc/renderer/hooks/index.tssrc/renderer/hooks/props/useMainPanelProps.tssrc/renderer/hooks/props/useSessionListProps.tssrc/renderer/hooks/session/index.tssrc/renderer/hooks/session/useSessionCrud.tssrc/renderer/hooks/wizard/index.tssrc/renderer/hooks/wizard/useWizardHandlers.tssrc/renderer/utils/tabHelpers.ts
💤 Files with no reviewable changes (1)
- src/renderer/hooks/props/useMainPanelProps.ts
Expand lint-staged from `*.{ts,tsx}` to `*` so Prettier runs on all
staged files. Reformat markdown tables in docs for consistency.
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/releases.md`:
- Line 24: Update the phrasing in the release note string that currently reads
"cross context memory" to use a hyphenated compound modifier "cross-context
memory" for readability; locate the sentence containing "Agents are now
inherently aware of your activity history as seen in the history panel 📜 (this
is built-in cross context memory!)" and replace the unhyphenated phrase with
"cross-context memory".
- Line 135: The release notes text contains incorrect casing "Github Worktree" —
update the string "Github Worktree support was added." to use the official
product casing "GitHub Worktree support was added." so all occurrences (e.g.,
the sentence starting with "Github Worktree support was added.") in
docs/releases.md are corrected to "GitHub".
- Line 200: The release notes use mixed variants "prerelease" and "pre-release"
(e.g., the line "- Enhanced update checker to filter prerelease tags like -rc,
-beta 🚀"); standardize all instances to the chosen form "pre-release" across
docs/releases.md (and any nearby lines) by replacing "prerelease" with
"pre-release" and ensuring consistency in similar phrases like "pre-release
tags" and "pre-release build".
- Around line 110-114: Fix typos and grammar in the v0.12.x release notes:
change "built in" to "built-in" in the sentence about GitHub Spec-Kit, correct
"Wortrees" to "Worktrees" (used in the clause mentioning v0.11.x), and change
"You will received" to "You will receive" in the Context Management Tools
paragraph; keep references to the GitHub Spec-Kit link and the overridden
command "/speckit-implement" intact.
- Around line 35-41: Fix the two typos in the release notes: replace the
mis-capitalized word "FIle" with "File" in the sentence describing launch
options ("Launch from file preview or from the File tree panel"), and correct
the version series string "014.x" to "0.14.x" in the heading "Smaller Changes in
014.x"; update the visible strings so the user-facing docs show "File" and
"0.14.x".
In `@package.json`:
- Around line 316-319: The pre-commit entry using the "*" glob runs "eslint
--fix" against every staged file (the "*" pattern) and will try to lint non-JS
assets; change the hook so that instead of the bare "*" pattern it only targets
lintable extensions and/or invokes ESLint with explicit extensions (e.g. use a
glob like **/*.{js,jsx,ts,tsx} or add --ext .js,.jsx,.ts,.tsx to the "eslint
--fix" invocation). Locate the package.json pre-commit/husky entry that
currently contains "*" and the "eslint --fix" command and update it to restrict
files to supported extensions to avoid running ESLint on images/JSON/CSS/etc.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
docs/autorun-playbooks.mddocs/configuration.mddocs/releases.mdpackage.json
✅ Files skipped from review due to trivial changes (1)
- docs/autorun-playbooks.md
|
|
||
| - Desktop app performance improvements (more to come on this, we want Maestro blazing fast) 🐌 | ||
| - Added local manifest feature for custom playbooks 📖 | ||
| - Agents are now inherently aware of your activity history as seen in the history panel 📜 (this is built-in cross context memory!) |
There was a problem hiding this comment.
Hyphenate compound modifier for readability.
cross context memory reads better as cross-context memory.
🧰 Tools
🪛 LanguageTool
[grammar] ~24-~24: Use a hyphen to join words.
Context: ...history panel 📜 (this is built-in cross context memory!) - Added markdown render...
(QB_NEW_EN_HYPHEN)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/releases.md` at line 24, Update the phrasing in the release note string
that currently reads "cross context memory" to use a hyphenated compound
modifier "cross-context memory" for readability; locate the sentence containing
"Agents are now inherently aware of your activity history as seen in the history
panel 📜 (this is built-in cross context memory!)" and replace the unhyphenated
phrase with "cross-context memory".
Comment on lines
+35
to
+41
| 🗄️ Document Graphs. Launch from file preview or from the FIle tree panel. Explore relationships between Markdown documents that contain links between documents and to URLs. | ||
|
|
||
| 📶 SSH support for agents. Manage a remote agent with feature parity over SSH. Includes support for Git and File tree panels. Manage agents on remote systems or in containers. This even works for Group Chat, which is rad as hell. | ||
|
|
||
| 🧙♂️ Added an in-tab wizard for generating Auto Run Playbooks via `/wizard` or a new button in the Auto Run panel. | ||
|
|
||
| # Smaller Changes in 014.x |
There was a problem hiding this comment.
Fix heading/title typos and version series formatting.
There are two user-visible text issues: FIle should be File (Line 35), and 014.x should be 0.14.x (Line 41).
Suggested patch
-🗄️ Document Graphs. Launch from file preview or from the FIle tree panel.
+🗄️ Document Graphs. Launch from file preview or from the File tree panel.
-# Smaller Changes in 014.x
+# Smaller Changes in 0.14.x📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| 🗄️ Document Graphs. Launch from file preview or from the FIle tree panel. Explore relationships between Markdown documents that contain links between documents and to URLs. | |
| 📶 SSH support for agents. Manage a remote agent with feature parity over SSH. Includes support for Git and File tree panels. Manage agents on remote systems or in containers. This even works for Group Chat, which is rad as hell. | |
| 🧙♂️ Added an in-tab wizard for generating Auto Run Playbooks via `/wizard` or a new button in the Auto Run panel. | |
| # Smaller Changes in 014.x | |
| 🗄️ Document Graphs. Launch from file preview or from the File tree panel. Explore relationships between Markdown documents that contain links between documents and to URLs. | |
| 📶 SSH support for agents. Manage a remote agent with feature parity over SSH. Includes support for Git and File tree panels. Manage agents on remote systems or in containers. This even works for Group Chat, which is rad as hell. | |
| 🧙♂️ Added an in-tab wizard for generating Auto Run Playbooks via `/wizard` or a new button in the Auto Run panel. | |
| # Smaller Changes in 0.14.x |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/releases.md` around lines 35 - 41, Fix the two typos in the release
notes: replace the mis-capitalized word "FIle" with "File" in the sentence
describing launch options ("Launch from file preview or from the File tree
panel"), and correct the version series string "014.x" to "0.14.x" in the
heading "Smaller Changes in 014.x"; update the visible strings so the
user-facing docs show "File" and "0.14.x".
Comment on lines
+110
to
+114
| 🎯 Added [GitHub Spec-Kit](https://github.com/github/spec-kit) commands into Maestro with a built in updater to grab the latest prompts from the repository. We do override `/speckit-implement` (the final step) to create Auto Run docs and guide the user through their execution, which thanks to Wortrees from v0.11.x allows us to run in parallel! | ||
|
|
||
| ## Context Management Tools | ||
|
|
||
| 📖 Added context management options from tab right-click menu. You can now compress, merge, and transfer contexts between agents. You will received (configurable) warnings at 60% and 80% context consumption with a hint to compact. |
There was a problem hiding this comment.
Correct spelling/grammar in the v0.12.x section.
built in should be hyphenated, Wortrees appears to be a typo, and You will received is grammatically incorrect.
Suggested patch
-🎯 Added [GitHub Spec-Kit](https://github.com/github/spec-kit) commands into Maestro with a built in updater ... thanks to Wortrees from v0.11.x ...
+🎯 Added [GitHub Spec-Kit](https://github.com/github/spec-kit) commands into Maestro with a built-in updater ... thanks to Worktrees from v0.11.x ...
-📖 Added context management options from tab right-click menu. ... You will received (configurable) warnings ...
+📖 Added context management options from tab right-click menu. ... You will receive (configurable) warnings ...📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| 🎯 Added [GitHub Spec-Kit](https://github.com/github/spec-kit) commands into Maestro with a built in updater to grab the latest prompts from the repository. We do override `/speckit-implement` (the final step) to create Auto Run docs and guide the user through their execution, which thanks to Wortrees from v0.11.x allows us to run in parallel! | |
| ## Context Management Tools | |
| 📖 Added context management options from tab right-click menu. You can now compress, merge, and transfer contexts between agents. You will received (configurable) warnings at 60% and 80% context consumption with a hint to compact. | |
| 🎯 Added [GitHub Spec-Kit](https://github.com/github/spec-kit) commands into Maestro with a built-in updater to grab the latest prompts from the repository. We do override `/speckit-implement` (the final step) to create Auto Run docs and guide the user through their execution, which thanks to Worktrees from v0.11.x allows us to run in parallel! | |
| ## Context Management Tools | |
| 📖 Added context management options from tab right-click menu. You can now compress, merge, and transfer contexts between agents. You will receive (configurable) warnings at 60% and 80% context consumption with a hint to compact. |
🧰 Tools
🪛 LanguageTool
[grammar] ~110-~110: Use a hyphen to join words.
Context: ...-kit) commands into Maestro with a built in updater to grab the latest prompts fr...
(QB_NEW_EN_HYPHEN)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/releases.md` around lines 110 - 114, Fix typos and grammar in the
v0.12.x release notes: change "built in" to "built-in" in the sentence about
GitHub Spec-Kit, correct "Wortrees" to "Worktrees" (used in the clause
mentioning v0.11.x), and change "You will received" to "You will receive" in the
Context Management Tools paragraph; keep references to the GitHub Spec-Kit link
and the overridden command "/speckit-implement" intact.
| - @ file mentions now include documents from your Auto Run folder (which may not live in your agent working directory) 🗄️ | ||
| - The wizard is now capable of detecting and continuing on past started projects 🧙 | ||
| 🌳 Github Worktree support was added. Any agent bound to a Git repository has the option to enable worktrees, each of which show up as a sub-agent with their own write-lock and Auto Run capability. Now you can truly develop in parallel on the same project and issue PRs when you're ready, all from within Maestro. Huge improvement, major thanks to @petersilberman. |
There was a problem hiding this comment.
Use official product casing for “GitHub.”
Line 135 uses Github; this should be GitHub for consistency and branding correctness.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~135-~135: The official name of this software platform is spelled with a capital “H”.
Context: ...11.0** | Released December 22, 2025 🌳 Github Worktree support was added. Any agent b...
(GITHUB)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/releases.md` at line 135, The release notes text contains incorrect
casing "Github Worktree" — update the string "Github Worktree support was
added." to use the official product casing "GitHub Worktree support was added."
so all occurrences (e.g., the sentence starting with "Github Worktree support
was added.") in docs/releases.md are corrected to "GitHub".
| - Implemented fuzzy file search in quick actions for instant navigation 🔍 | ||
| - Added "clear" command support to clean terminal shell logs 🧹 | ||
| - Simplified search highlighting by integrating into markdown pipeline ✨ | ||
| - Enhanced update checker to filter prerelease tags like -rc, -beta 🚀 |
There was a problem hiding this comment.
Normalize “pre-release” wording across the doc.
Use one variant consistently (pre-release vs prerelease) in release notes terminology.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~200-~200: Do not mix variants of the same word (‘prerelease’ and ‘pre-release’) within a single text.
Context: ...e ✨ - Enhanced update checker to filter prerelease tags like -rc, -beta 🚀 - Fixed RPM pac...
(EN_WORD_COHERENCY)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/releases.md` at line 200, The release notes use mixed variants
"prerelease" and "pre-release" (e.g., the line "- Enhanced update checker to
filter prerelease tags like -rc, -beta 🚀"); standardize all instances to the
chosen form "pre-release" across docs/releases.md (and any nearby lines) by
replacing "prerelease" with "pre-release" and ensuring consistency in similar
phrases like "pre-release tags" and "pre-release build".
Split lint-staged config so prettier runs on all files while eslint only targets JS/TS files, avoiding "File ignored" warnings on non-script files.
- Fix stale processInput() in deferred auto-send (capture target IDs, add cleanup, verify before firing) - Redact sensitive queue item content from console.log - Handle processQueuedItem rejection in App.tsx startup queue processing - Gate Director's Notes menu on encoreFeatures flag instead of always-truthy setter existence - Reset all busy tabs on interrupt (not just active tab) in canceledLog branch - Handle processQueuedItem rejections in both useInterruptHandler setTimeout calls - Use "agent" instead of "session" in user-facing toast text per CLAUDE.md vernacular - Derive provider session ID from aiTabs in finishRenamingSession when session-level agentSessionId is absent - Fix incorrect elapsed-time comment in inlineWizardConversation test
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/__tests__/renderer/components/SessionList.test.tsx (1)
1167-1180:⚠️ Potential issue | 🟡 MinorAvoid replacing
encoreFeatureswith a partial object in tests.This overwrites sibling feature flags and can make tests order-dependent. Merge into existing
encoreFeaturesinstead.Suggested fix
- useSettingsStore.setState({ encoreFeatures: { directorNotes: true } }); + useSettingsStore.setState((s) => ({ + encoreFeatures: { ...s.encoreFeatures, directorNotes: true }, + }));- useSettingsStore.setState({ encoreFeatures: { directorNotes: true } }); + useSettingsStore.setState((s) => ({ + encoreFeatures: { ...s.encoreFeatures, directorNotes: true }, + }));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/renderer/components/SessionList.test.tsx` around lines 1167 - 1180, The test is overwriting the entire encoreFeatures object which can drop other flags and make tests order-dependent; instead, read the current encoreFeatures from useSettingsStore.getState(), merge in directorNotes: true and call useSettingsStore.setState with the merged encoreFeatures so only that flag is changed (update the two places in the test that call useSettingsStore.setState and reference encoreFeatures/directorNotes and leave createDefaultProps and SessionList unchanged).
♻️ Duplicate comments (2)
src/renderer/hooks/agent/useInterruptHandler.ts (1)
343-372:⚠️ Potential issue | 🟠 MajorNo-queue kill/error cleanup still leaves non-active busy tabs stale.
These branches only normalize the active tab. Other tabs marked
busycan survive while session state is set toidle, leaving contradictory state.Suggested fix pattern (apply in both branches)
- aiTabs: updatedSession.aiTabs.map((t) => { - if (t.id === tab.id) { + aiTabs: updatedSession.aiTabs.map((t) => { + const shouldReset = t.state === 'busy' || t.id === tab.id; + if (shouldReset) { const logsWithoutThinkingOrTools = t.logs.filter( (log) => log.source !== 'thinking' && log.source !== 'tool' ); return { ...t, state: 'idle' as const, thinkingStartTime: undefined, logs: logsWithoutThinkingOrTools, }; } return t; }),Also applies to: 408-437
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/hooks/agent/useInterruptHandler.ts` around lines 343 - 372, The branch that sets session state to 'idle' only normalizes the active tab and leaves other aiTabs with state 'busy' stale; update the logic in useInterruptHandler (both the branch shown and the similar branch at 408-437) so that when returning updatedSession with state: 'idle' and busySource/thinkingStartTime cleared you also map over updatedSession.aiTabs and for every tab with state 'busy' (or any non-idle state) set state: 'idle', thinkingStartTime: undefined, and remove logs whose source is 'thinking' or 'tool' (same filter used for the active tab); use getActiveTab and tab.id only to detect the active tab when needed but apply normalization across all aiTabs to avoid contradictory states.src/renderer/App.tsx (1)
2460-2480:⚠️ Potential issue | 🟠 MajorQueue replay can still drop a queued item on processing failure.
At Line [2432] the item is dequeued before execution, and at Line [2461] failure only resets busy state. The failed
firstItemis not re-queued or persisted as failed, so startup replay can silently lose input.Proposed fix
- processQueuedItem(session.id, firstItem).catch((err) => { + processQueuedItem(session.id, firstItem).catch((err) => { console.error(`[App] Failed to process queued item for session ${session.id}:`, err); - // Reset session busy state so the queue isn't permanently stuck + // Reset session busy state and restore failed item to queue setSessions((prev) => prev.map((s) => { if (s.id !== session.id) return s; return { ...s, state: 'idle', busySource: undefined, thinkingStartTime: undefined, + executionQueue: [firstItem, ...s.executionQueue], aiTabs: s.aiTabs.map((tab) => - tab.state === 'busy' + tab.id === firstItem.tabId ? { ...tab, state: 'idle' as const, thinkingStartTime: undefined } : tab ), }; }) ); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/App.tsx` around lines 2460 - 2480, The catch block for processQueuedItem currently only resets session busy state and drops firstItem; update it to re-queue or persist the failed firstItem instead of discarding it: in the catch for processQueuedItem(session.id, firstItem) either call the existing enqueue function (or use setSessions to insert firstItem back onto that session's queue) so it will be replayed on startup, or mark the item as persisted failed (with an explicit failed flag) so it isn't silently lost; keep resetting busy state but ensure the re-queue/persist logic references session.id and firstItem and runs before finishing the catch.
🧹 Nitpick comments (2)
src/__tests__/renderer/hooks/useSessionCrud.test.ts (1)
88-89: Restorewindow.maestroafter tests to avoid global leakage.Line [88] sets a global mock, but teardown only runs
cleanup(). This can bleed state into other suites running in the same environment.Proposed fix
-import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { describe, it, expect, vi, beforeEach, afterEach, afterAll } from 'vitest'; @@ -(window as any).maestro = mockMaestro; +const originalMaestro = (window as any).maestro; +(window as any).maestro = mockMaestro; @@ afterEach(() => { cleanup(); }); + +afterAll(() => { + (window as any).maestro = originalMaestro; +});Also applies to: 190-192
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/renderer/hooks/useSessionCrud.test.ts` around lines 88 - 89, Tests set a global mock via (window as any).maestro = mockMaestro but never restore it, leaking state; capture the original value (e.g., const originalMaestro = (window as any).maestro) before assigning and restore it in the test teardown (afterEach or finally) after calling cleanup(), and apply the same pattern for the other places where mockMaestro is assigned (the other test blocks referencing window.maestro) so the global is always returned to its prior value even if tests fail.src/__tests__/renderer/hooks/useWizardHandlers.test.ts (1)
1730-1760: Extract repeated wizard-state fixture builders.These large repeated
wizardContext.stateliterals are costly to maintain and easy to drift. AcreateWizardContextState(overrides)helper will shrink tests and make intent clearer.Also applies to: 1799-1829, 1857-1887, 1942-1972, 1999-2029, 2049-2079, 2098-2128, 2150-2183
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/renderer/hooks/useWizardHandlers.test.ts` around lines 1730 - 1760, Extract the repeated wizardContext.state literal into a reusable factory: implement a createWizardContextState(overrides?: Partial<WizardState>) helper that returns the full default state used in tests (matching fields like currentStep, isOpen, selectedAgent, availableAgents, agentName, directoryPath, isGitRepo, detectedAgentPath, directoryError, hasExistingAutoRunDocs, existingDocsCount, existingDocsChoice, conversationHistory, confidenceLevel, isReadyToProceed, isConversationLoading, conversationError, generatedDocuments, currentDocumentIndex, isGeneratingDocuments, generationError, editedPhase1Content, wantsTour, isComplete, createdSessionId) and merge any overrides; then replace the inline literals in the test cases (where wizardContext: { state: { ... } as any, completeWizard: vi.fn(), clearResumeState: vi.fn() } ) with wizardContext: { state: createWizardContextState({...overrides}), completeWizard: vi.fn(), clearResumeState: vi.fn() } to reduce duplication across the specified ranges.
🤖 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/hooks/useWizardHandlers.test.ts`:
- Around line 587-631: Update the test "clears thinking content and tool
executions before sending" to assert the state changes instead of only checking
sendMessage; after calling result.current.sendWizardMessageWithThinking('Test'),
read the updated session/tab from useSessionStore (or from the session object
returned by createMockSession) and add explicit expect assertions that the
tab.wizardState.thinkingContent is '' and that tab.wizardState.toolExecutions is
reset (e.g., empty array or the expected cleared value). Keep the existing
expect(sendMessage).toHaveBeenCalled() but add these two assertions referencing
sendWizardMessageWithThinking, thinkingContent, and toolExecutions to close the
assertion gap.
- Around line 333-336: Tests in useWizardHandlers.test rely on real-time sleeps
like "await new Promise((r) => setTimeout(r, X))" inside act blocks which causes
flakiness; replace each instance with Vitest fake timers by calling
vi.useFakeTimers() at the start of the test, trigger timer-driven behavior with
vi.advanceTimersByTimeAsync(X) inside the same act wrapper (or await act(() =>
vi.advanceTimersByTimeAsync(X))) to simulate the hook's 100ms/300ms/500ms delays
deterministically, then call vi.useRealTimers() (or
vi.resetAllMocks()/vi.clearAllTimers()) at the end to restore timers; update all
occurrences noted (around the act blocks in useWizardHandlers.test where
setTimeout-based sleeps appear) and ensure assertions run after advancing
timers.
In `@src/renderer/App.tsx`:
- Around line 2418-2485: The effect that schedules the startup replay using
setTimeout needs to cancel that timer on unmount: capture the timer ID when
calling setTimeout inside the effect (where sessionsLoaded and sessions are
dependencies) and return a cleanup function from the effect that calls
clearTimeout(timerId) to prevent the delayed callback from running after
teardown; update the effect surrounding setTimeout (the block that calls
processQueuedItem for each session and references sessionsLoaded/sessions) to
store the timeout id in a local variable and clear it in the returned cleanup.
In `@src/renderer/hooks/agent/useInterruptHandler.ts`:
- Around line 69-73: The cancelPendingSynopsis(...) call should not share the
interrupt failure catch: first await cancelPendingSynopsis(activeSession.id)
inside its own try/catch and handle/log errors locally (do not rethrow), then in
a separate try/catch await (window as
any).maestro.process.interrupt(targetSessionId) so only interrupt errors flow
into the force-kill fallback; apply the same split-error-handling pattern for
the other occurrence around the lines referencing cancelPendingSynopsis and
maestro.process.interrupt (the block including targetSessionId and
activeSession.id).
In `@src/renderer/hooks/session/useSessionCrud.ts`:
- Around line 145-151: The call to maestro.agents.get(agentId) is performed
outside the try/catch in createNewSession so a rejection there will escape local
handling; move or duplicate the agent lookup into the existing try block (or
wrap it with its own try/catch) so any rejection from (window as
any).maestro.agents.get(agentId) is caught, log the error and return early
(keeping the existing agent-null check and console.error for Agent not found:
${agentId}) to prevent unhandled rejections in createNewSession.
---
Outside diff comments:
In `@src/__tests__/renderer/components/SessionList.test.tsx`:
- Around line 1167-1180: The test is overwriting the entire encoreFeatures
object which can drop other flags and make tests order-dependent; instead, read
the current encoreFeatures from useSettingsStore.getState(), merge in
directorNotes: true and call useSettingsStore.setState with the merged
encoreFeatures so only that flag is changed (update the two places in the test
that call useSettingsStore.setState and reference encoreFeatures/directorNotes
and leave createDefaultProps and SessionList unchanged).
---
Duplicate comments:
In `@src/renderer/App.tsx`:
- Around line 2460-2480: The catch block for processQueuedItem currently only
resets session busy state and drops firstItem; update it to re-queue or persist
the failed firstItem instead of discarding it: in the catch for
processQueuedItem(session.id, firstItem) either call the existing enqueue
function (or use setSessions to insert firstItem back onto that session's queue)
so it will be replayed on startup, or mark the item as persisted failed (with an
explicit failed flag) so it isn't silently lost; keep resetting busy state but
ensure the re-queue/persist logic references session.id and firstItem and runs
before finishing the catch.
In `@src/renderer/hooks/agent/useInterruptHandler.ts`:
- Around line 343-372: The branch that sets session state to 'idle' only
normalizes the active tab and leaves other aiTabs with state 'busy' stale;
update the logic in useInterruptHandler (both the branch shown and the similar
branch at 408-437) so that when returning updatedSession with state: 'idle' and
busySource/thinkingStartTime cleared you also map over updatedSession.aiTabs and
for every tab with state 'busy' (or any non-idle state) set state: 'idle',
thinkingStartTime: undefined, and remove logs whose source is 'thinking' or
'tool' (same filter used for the active tab); use getActiveTab and tab.id only
to detect the active tab when needed but apply normalization across all aiTabs
to avoid contradictory states.
---
Nitpick comments:
In `@src/__tests__/renderer/hooks/useSessionCrud.test.ts`:
- Around line 88-89: Tests set a global mock via (window as any).maestro =
mockMaestro but never restore it, leaking state; capture the original value
(e.g., const originalMaestro = (window as any).maestro) before assigning and
restore it in the test teardown (afterEach or finally) after calling cleanup(),
and apply the same pattern for the other places where mockMaestro is assigned
(the other test blocks referencing window.maestro) so the global is always
returned to its prior value even if tests fail.
In `@src/__tests__/renderer/hooks/useWizardHandlers.test.ts`:
- Around line 1730-1760: Extract the repeated wizardContext.state literal into a
reusable factory: implement a createWizardContextState(overrides?:
Partial<WizardState>) helper that returns the full default state used in tests
(matching fields like currentStep, isOpen, selectedAgent, availableAgents,
agentName, directoryPath, isGitRepo, detectedAgentPath, directoryError,
hasExistingAutoRunDocs, existingDocsCount, existingDocsChoice,
conversationHistory, confidenceLevel, isReadyToProceed, isConversationLoading,
conversationError, generatedDocuments, currentDocumentIndex,
isGeneratingDocuments, generationError, editedPhase1Content, wantsTour,
isComplete, createdSessionId) and merge any overrides; then replace the inline
literals in the test cases (where wizardContext: { state: { ... } as any,
completeWizard: vi.fn(), clearResumeState: vi.fn() } ) with wizardContext: {
state: createWizardContextState({...overrides}), completeWizard: vi.fn(),
clearResumeState: vi.fn() } to reduce duplication across the specified ranges.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/__tests__/renderer/components/SessionList.test.tsxsrc/__tests__/renderer/components/TransferErrorModal.test.tsxsrc/__tests__/renderer/hooks/useSessionCrud.test.tssrc/__tests__/renderer/hooks/useWizardHandlers.test.tssrc/__tests__/renderer/services/inlineWizardConversation.test.tssrc/renderer/App.tsxsrc/renderer/components/SessionList.tsxsrc/renderer/components/TransferErrorModal.tsxsrc/renderer/hooks/agent/useInterruptHandler.tssrc/renderer/hooks/session/useSessionCrud.tssrc/renderer/hooks/wizard/useWizardHandlers.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/tests/renderer/services/inlineWizardConversation.test.ts
- src/renderer/hooks/wizard/useWizardHandlers.ts
Comment on lines
+333
to
+336
| // Wait for async effects | ||
| await act(async () => { | ||
| await new Promise((r) => setTimeout(r, 50)); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and get basic stats
wc -l src/__tests__/renderer/hooks/useWizardHandlers.test.tsRepository: RunMaestro/Maestro
Length of output: 121
🏁 Script executed:
# Check the specific lines mentioned in the review (333-336)
sed -n '333,336p' src/__tests__/renderer/hooks/useWizardHandlers.test.ts | cat -nRepository: RunMaestro/Maestro
Length of output: 201
🏁 Script executed:
# Verify all instances of the sleep pattern across the file
rg -n "new Promise\(\(r\) => setTimeout\(r,\s*\d+\)\)" src/__tests__/renderer/hooks/useWizardHandlers.test.tsRepository: RunMaestro/Maestro
Length of output: 596
🏁 Script executed:
# Also check if there are other setTimeout patterns that might be relevant
rg -n "setTimeout\(" src/__tests__/renderer/hooks/useWizardHandlers.test.ts | head -20Repository: RunMaestro/Maestro
Length of output: 596
🏁 Script executed:
# Check imports and setup of the test file
head -50 src/__tests__/renderer/hooks/useWizardHandlers.test.ts | cat -nRepository: RunMaestro/Maestro
Length of output: 2261
🏁 Script executed:
# Check beforeEach/afterEach setup
rg -A 20 "beforeEach|afterEach" src/__tests__/renderer/hooks/useWizardHandlers.test.ts | head -60Repository: RunMaestro/Maestro
Length of output: 2038
🏁 Script executed:
# Find and check the useWizardHandlers hook implementation
fd -t f "useWizardHandlers" src --exclude="*.test.*" --exclude="__tests__"Repository: RunMaestro/Maestro
Length of output: 108
🏁 Script executed:
# Search for where useWizardHandlers is defined
rg -l "export.*useWizardHandlers|function useWizardHandlers" src --exclude-dir=__tests__Repository: RunMaestro/Maestro
Length of output: 477
🏁 Script executed:
# Check the useWizardHandlers hook implementation
head -100 src/renderer/hooks/wizard/useWizardHandlers.ts | cat -nRepository: RunMaestro/Maestro
Length of output: 4951
🏁 Script executed:
# Search for setTimeout usage in the hook
rg -n "setTimeout|useEffect|useCallback" src/renderer/hooks/wizard/useWizardHandlers.ts | head -20Repository: RunMaestro/Maestro
Length of output: 806
🏁 Script executed:
# Check if there are any timers in the hook that would conflict with fake timers
rg -B 2 -A 5 "setTimeout" src/renderer/hooks/wizard/useWizardHandlers.ts | head -40Repository: RunMaestro/Maestro
Length of output: 892
🏁 Script executed:
# Check if other test files use vi.useFakeTimers in this repo
fd -t f "\.test\.ts" src --type f | head -20Repository: RunMaestro/Maestro
Length of output: 1018
🏁 Script executed:
# Search for existing vi.useFakeTimers usage in test files
rg -l "useFakeTimers|vi.useFakeTimers" src/__tests__ | head -5Repository: RunMaestro/Maestro
Length of output: 301
🏁 Script executed:
# Check one of those files to see how fake timers are used with React Testing Library
rg -B 5 -A 10 "useFakeTimers" src/__tests__ | head -50Repository: RunMaestro/Maestro
Length of output: 3466
🏁 Script executed:
# Check the indentation in the test file (tabs vs spaces)
head -50 src/__tests__/renderer/hooks/useWizardHandlers.test.ts | cat -A | head -30Repository: RunMaestro/Maestro
Length of output: 1360
🏁 Script executed:
# Get a count of all the sleep patterns in the file
rg -c "new Promise\(\(r\) => setTimeout\(r," src/__tests__/renderer/hooks/useWizardHandlers.test.tsRepository: RunMaestro/Maestro
Length of output: 64
🏁 Script executed:
# Verify all line numbers where these patterns appear to get accurate count
rg -n "await new Promise\(\(r\) => setTimeout\(r," src/__tests__/renderer/hooks/useWizardHandlers.test.tsRepository: RunMaestro/Maestro
Length of output: 596
🏁 Script executed:
# Check one of the test contexts to understand what's being tested
sed -n '330,375p' src/__tests__/renderer/hooks/useWizardHandlers.test.ts | cat -nRepository: RunMaestro/Maestro
Length of output: 1933
Replace sleep-based waits with deterministic timer control.
This file contains 10 instances of await new Promise((r) => setTimeout(r, X)) pattern used to wait for async effects. Combined with the hook's internal timers (100ms, 300ms, 500ms+), these real-time delays can cause test flakiness in CI and slow down the test suite. Use Vitest fake timers (vi.useFakeTimers + vi.advanceTimersByTimeAsync) instead, which is already established elsewhere in the codebase.
Proposed refactor (deterministic timers)
describe('useWizardHandlers', () => {
beforeEach(() => {
vi.clearAllMocks();
+ vi.useFakeTimers();
idCounter = 0;
...
});
afterEach(() => {
+ vi.runOnlyPendingTimers();
+ vi.useRealTimers();
cleanup();
});- await act(async () => {
- await new Promise((r) => setTimeout(r, 50));
- });
+ await act(async () => {
+ await vi.advanceTimersByTimeAsync(50);
+ });Also applies to: 365, 379, 399, 458, 506, 546, 1334, 1840, 1897
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/__tests__/renderer/hooks/useWizardHandlers.test.ts` around lines 333 -
336, Tests in useWizardHandlers.test rely on real-time sleeps like "await new
Promise((r) => setTimeout(r, X))" inside act blocks which causes flakiness;
replace each instance with Vitest fake timers by calling vi.useFakeTimers() at
the start of the test, trigger timer-driven behavior with
vi.advanceTimersByTimeAsync(X) inside the same act wrapper (or await act(() =>
vi.advanceTimersByTimeAsync(X))) to simulate the hook's 100ms/300ms/500ms delays
deterministically, then call vi.useRealTimers() (or
vi.resetAllMocks()/vi.clearAllTimers()) at the end to restore timers; update all
occurrences noted (around the act blocks in useWizardHandlers.test where
setTimeout-based sleeps appear) and ensure assertions run after advancing
timers.
- useWizardHandlers.test: fix assertion gap by mocking getStateForTab so sync effect preserves wizardState, enabling clearing verification - App.tsx: add cleanup for startup replay setTimeout; re-queue failed items in processQueuedItem catch to prevent queue data loss - useInterruptHandler: isolate cancelPendingSynopsis in own try/catch; normalize force-kill branches to reset ALL busy tabs, not just active - useSessionCrud: move agents.get inside try/catch for proper error handling - SessionList.test: spread existing encoreFeatures before overriding
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
src/__tests__/renderer/components/SessionList.test.tsx (1)
2916-2917: Minor duplication in spy setup.The
vi.spyOn(useSettingsStore.getState(), 'setLeftSidebarWidth')pattern is repeated in multiple tests (lines 1406, 2898, 2916). Consider extracting to a helper function if this pattern is used frequently.♻️ Optional refactor to reduce duplication
// Helper at the top of describe block const spyOnSettingsAction = <K extends keyof ReturnType<typeof useSettingsStore.getState>>( action: K ) => vi.spyOn(useSettingsStore.getState(), action); // Usage in tests const setLeftSidebarWidthState = spyOnSettingsAction('setLeftSidebarWidth');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/renderer/components/SessionList.test.tsx` around lines 2916 - 2917, Tests repeat vi.spyOn(useSettingsStore.getState(), 'setLeftSidebarWidth'); extract a small helper to centralize this pattern: add a function (e.g., spyOnSettingsAction) near the top of the describe block that accepts the action name and returns vi.spyOn(useSettingsStore.getState(), action), then replace direct calls with spyOnSettingsAction('setLeftSidebarWidth') where setLeftSidebarWidth is currently spied and similarly use it for other repeated spies; update tests that call createDefaultProps({}) to use the returned spy as before.src/renderer/hooks/session/useSessionCrud.ts (2)
417-423: toggleBookmark bypasses the localsetSessionsreference.This function directly calls
useSessionStore.getState().setSessions()instead of using thesetSessionsfrom the hook's getState() call at line 107. While functionally equivalent, it's inconsistent with other handlers in this hook.♻️ Suggested consistency fix
const toggleBookmark = useCallback((sessionId: string) => { - useSessionStore - .getState() - .setSessions((prev) => - prev.map((s) => (s.id === sessionId ? { ...s, bookmarked: !s.bookmarked } : s)) - ); + setSessions((prev) => + prev.map((s) => (s.id === sessionId ? { ...s, bookmarked: !s.bookmarked } : s)) + ); }, []);Note: You'd need to add
setSessionsto the dependency array.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/hooks/session/useSessionCrud.ts` around lines 417 - 423, The toggleBookmark handler currently calls useSessionStore.getState().setSessions(...) directly instead of using the local setSessions reference obtained earlier via getState(); update toggleBookmark to call the local setSessions function (the one returned by your hook's getState() call) so it matches other handlers, and add setSessions to the useCallback dependency array; reference the toggleBookmark function and the store's setSessions symbol when making this change.
309-326: Sequential process termination may slow group deletion for large groups.The
for...ofloop withawaitprocesses each session serially. For groups with many agents, this could cause noticeable delay.Consider parallelizing the cleanup operations:
♻️ Suggested parallel cleanup
- for (const session of groupSessions) { - try { - await (window as any).maestro.process.kill(`${session.id}-ai`); - } catch (error) { - console.error('Failed to kill AI process:', error); - } - try { - await (window as any).maestro.process.kill(`${session.id}-terminal`); - } catch (error) { - console.error('Failed to kill terminal process:', error); - } - try { - await (window as any).maestro.playbooks.deleteAll(session.id); - } catch (error) { - console.error('Failed to delete playbooks:', error); - } - } + await Promise.all( + groupSessions.map(async (session) => { + await Promise.allSettled([ + (window as any).maestro.process.kill(`${session.id}-ai`), + (window as any).maestro.process.kill(`${session.id}-terminal`), + (window as any).maestro.playbooks.deleteAll(session.id), + ]); + }) + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/hooks/session/useSessionCrud.ts` around lines 309 - 326, The current for...of loop over groupSessions performs each cleanup sequentially; replace it with parallelized promises so session cleanup runs concurrently: for each session (from groupSessions) create a Promise that runs the three ops ((window as any).maestro.process.kill(`${session.id}-ai`), maestro.process.kill(`${session.id}-terminal`), and (window as any).maestro.playbooks.deleteAll(session.id)) in parallel (e.g., Promise.all or Promise.allSettled per session) and then await Promise.all or Promise.allSettled of those per-session promises to perform all session cleanups concurrently while preserving error logging/handling.src/renderer/hooks/agent/useInterruptHandler.ts (1)
104-178: Consider extracting shared tab-state-update logic.The interrupt path (lines 104-178) and kill path (lines 254-344) share nearly identical logic for resetting busy tabs, handling queue items, and adding log entries. Extracting this into a helper function would reduce duplication and make future maintenance easier.
♻️ Example helper extraction
// Helper to build updated aiTabs when processing queue after interrupt/kill function buildTabsForQueueProcessing( aiTabs: AITab[], targetTab: AITab, nextItem: QueuedItem, canceledLog: LogEntry | null ): AITab[] { let updatedTabs = aiTabs.map((tab) => { if (tab.id === targetTab.id) { return { ...tab, state: 'busy' as const, thinkingStartTime: Date.now() }; } if (tab.state === 'busy') { const cleanedLogs = tab.logs.filter( (log) => log.source !== 'thinking' && log.source !== 'tool' ); return { ...tab, state: 'idle' as const, thinkingStartTime: undefined, logs: canceledLog ? [...cleanedLogs, canceledLog] : cleanedLogs, }; } return tab; }); if (nextItem.type === 'message' && nextItem.text) { const logEntry: LogEntry = { id: generateId(), timestamp: Date.now(), source: 'user', text: nextItem.text, images: nextItem.images, }; updatedTabs = updatedTabs.map((tab) => tab.id === targetTab.id ? { ...tab, logs: [...tab.logs, logEntry] } : tab ); } return updatedTabs; }Also applies to: 254-344
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/hooks/agent/useInterruptHandler.ts` around lines 104 - 178, The interrupt and kill branches in useInterruptHandler share the same tab-reset, queue-consume and user-log-append logic; extract that into a helper (e.g., buildTabsForQueueProcessing(aiTabs: AITab[], targetTab: AITab, nextItem: QueuedItem, canceledLog: LogEntry | null): AITab[]) that performs: (1) map busy tabs to idle and append canceledLog (filtering out 'thinking'/'tool' logs), (2) set targetTab to busy with thinkingStartTime, and (3) if nextItem.type === 'message' create a user LogEntry (use generateId(), Date.now(), nextItem.text/images) and append it to targetTab.logs; then call this helper from both the interrupt branch and the kill branch to replace the duplicated mapping and log-building code (keeping other state updates like executionQueue, state, busySource, thinkingStartTime, currentCycleTokens/Bytes intact).
🤖 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/agent/useInterruptHandler.ts`:
- Around line 114-124: When targetTab is missing in useInterruptHandler.ts the
code returns while leaving other sessions marked busy; update the branch that
handles "!targetTab" to also iterate the sessions and reset any stale busy
sessions (e.g., where s.state === 'busy' and s.busySource === 'ai' or
s.targetTabId matches the missing tab) back to idle: set state to 'idle', clear
busySource/target-related fields, zero currentCycleTokens/currentCycleBytes and
update executionQueue/thinkingStartTime as appropriate before returning the new
sessions array; adjust the transformation that currently returns the single
session object so it returns the full, corrected sessions list instead.
- Around line 286-296: When targetTab is missing in the kill recovery branch
inside useInterruptHandler (the block returning updatedSession when !targetTab),
reset any busy-related fields to make the tab idle rather than leaving it
'busy'; update the returned updatedSession to set state to 'idle' (SessionState
idle), clear busySource, clear thinkingStartTime, set currentCycleTokens and
currentCycleBytes to 0, and ensure executionQueue reflects remainingQueue or is
emptied as in the earlier recovery path (mirror the reset behavior implemented
around lines 114-124) so busy tabs are properly reset when targetTab isn't
found.
---
Nitpick comments:
In `@src/__tests__/renderer/components/SessionList.test.tsx`:
- Around line 2916-2917: Tests repeat vi.spyOn(useSettingsStore.getState(),
'setLeftSidebarWidth'); extract a small helper to centralize this pattern: add a
function (e.g., spyOnSettingsAction) near the top of the describe block that
accepts the action name and returns vi.spyOn(useSettingsStore.getState(),
action), then replace direct calls with
spyOnSettingsAction('setLeftSidebarWidth') where setLeftSidebarWidth is
currently spied and similarly use it for other repeated spies; update tests that
call createDefaultProps({}) to use the returned spy as before.
In `@src/renderer/hooks/agent/useInterruptHandler.ts`:
- Around line 104-178: The interrupt and kill branches in useInterruptHandler
share the same tab-reset, queue-consume and user-log-append logic; extract that
into a helper (e.g., buildTabsForQueueProcessing(aiTabs: AITab[], targetTab:
AITab, nextItem: QueuedItem, canceledLog: LogEntry | null): AITab[]) that
performs: (1) map busy tabs to idle and append canceledLog (filtering out
'thinking'/'tool' logs), (2) set targetTab to busy with thinkingStartTime, and
(3) if nextItem.type === 'message' create a user LogEntry (use generateId(),
Date.now(), nextItem.text/images) and append it to targetTab.logs; then call
this helper from both the interrupt branch and the kill branch to replace the
duplicated mapping and log-building code (keeping other state updates like
executionQueue, state, busySource, thinkingStartTime, currentCycleTokens/Bytes
intact).
In `@src/renderer/hooks/session/useSessionCrud.ts`:
- Around line 417-423: The toggleBookmark handler currently calls
useSessionStore.getState().setSessions(...) directly instead of using the local
setSessions reference obtained earlier via getState(); update toggleBookmark to
call the local setSessions function (the one returned by your hook's getState()
call) so it matches other handlers, and add setSessions to the useCallback
dependency array; reference the toggleBookmark function and the store's
setSessions symbol when making this change.
- Around line 309-326: The current for...of loop over groupSessions performs
each cleanup sequentially; replace it with parallelized promises so session
cleanup runs concurrently: for each session (from groupSessions) create a
Promise that runs the three ops ((window as
any).maestro.process.kill(`${session.id}-ai`),
maestro.process.kill(`${session.id}-terminal`), and (window as
any).maestro.playbooks.deleteAll(session.id)) in parallel (e.g., Promise.all or
Promise.allSettled per session) and then await Promise.all or Promise.allSettled
of those per-session promises to perform all session cleanups concurrently while
preserving error logging/handling.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/__tests__/renderer/components/SessionList.test.tsxsrc/__tests__/renderer/hooks/useWizardHandlers.test.tssrc/renderer/App.tsxsrc/renderer/hooks/agent/useInterruptHandler.tssrc/renderer/hooks/session/useSessionCrud.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/tests/renderer/hooks/useWizardHandlers.test.ts
Comment on lines
+114
to
+124
| if (!targetTab) { | ||
| return { | ||
| ...s, | ||
| state: 'busy' as SessionState, | ||
| busySource: 'ai', | ||
| executionQueue: remainingQueue, | ||
| thinkingStartTime: Date.now(), | ||
| currentCycleTokens: 0, | ||
| currentCycleBytes: 0, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Reset busy tabs when targetTab is not found.
When targetTab is not found, the code returns without resetting other busy tabs to idle. This leaves tabs in a potentially inconsistent state (session is busy but originating tab still shows busy without the new target).
🔧 Suggested fix
if (!targetTab) {
+ const resetAiTabs = s.aiTabs.map((tab) => {
+ if (tab.state === 'busy') {
+ const logsWithoutThinkingOrTools = tab.logs.filter(
+ (log) => log.source !== 'thinking' && log.source !== 'tool'
+ );
+ return {
+ ...tab,
+ state: 'idle' as const,
+ thinkingStartTime: undefined,
+ logs: canceledLog
+ ? [...logsWithoutThinkingOrTools, canceledLog]
+ : logsWithoutThinkingOrTools,
+ };
+ }
+ return tab;
+ });
return {
...s,
state: 'busy' as SessionState,
busySource: 'ai',
+ aiTabs: resetAiTabs,
executionQueue: remainingQueue,
thinkingStartTime: Date.now(),
currentCycleTokens: 0,
currentCycleBytes: 0,
};
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/hooks/agent/useInterruptHandler.ts` around lines 114 - 124, When
targetTab is missing in useInterruptHandler.ts the code returns while leaving
other sessions marked busy; update the branch that handles "!targetTab" to also
iterate the sessions and reset any stale busy sessions (e.g., where s.state ===
'busy' and s.busySource === 'ai' or s.targetTabId matches the missing tab) back
to idle: set state to 'idle', clear busySource/target-related fields, zero
currentCycleTokens/currentCycleBytes and update executionQueue/thinkingStartTime
as appropriate before returning the new sessions array; adjust the
transformation that currently returns the single session object so it returns
the full, corrected sessions list instead.
Comment on lines
+286
to
+296
| if (!targetTab) { | ||
| return { | ||
| ...updatedSession, | ||
| state: 'busy' as SessionState, | ||
| busySource: 'ai', | ||
| executionQueue: remainingQueue, | ||
| thinkingStartTime: Date.now(), | ||
| currentCycleTokens: 0, | ||
| currentCycleBytes: 0, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Same issue: Reset busy tabs when targetTab is not found in kill path.
Mirrors the same issue from lines 114-124. When targetTab is not found during kill recovery, busy tabs should be reset to idle.
🔧 Suggested fix
if (!targetTab) {
+ const resetAiTabs = updatedSession.aiTabs.map((tab) => {
+ if (tab.state === 'busy') {
+ const logsWithoutThinkingOrTools = tab.logs.filter(
+ (log) => log.source !== 'thinking' && log.source !== 'tool'
+ );
+ return {
+ ...tab,
+ state: 'idle' as const,
+ thinkingStartTime: undefined,
+ logs: logsWithoutThinkingOrTools,
+ };
+ }
+ return tab;
+ });
return {
...updatedSession,
state: 'busy' as SessionState,
busySource: 'ai',
+ aiTabs: resetAiTabs,
executionQueue: remainingQueue,
thinkingStartTime: Date.now(),
currentCycleTokens: 0,
currentCycleBytes: 0,
};
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/hooks/agent/useInterruptHandler.ts` around lines 286 - 296, When
targetTab is missing in the kill recovery branch inside useInterruptHandler (the
block returning updatedSession when !targetTab), reset any busy-related fields
to make the tab idle rather than leaving it 'busy'; update the returned
updatedSession to set state to 'idle' (SessionState idle), clear busySource,
clear thinkingStartTime, set currentCycleTokens and currentCycleBytes to 0, and
ensure executionQueue reflects remainingQueue or is emptied as in the earlier
recovery path (mirror the reset behavior implemented around lines 114-124) so
busy tabs are properly reset when targetTab isn't found.
This was referenced Feb 27, 2026
openasocket
added a commit
to openasocket/Maestro
that referenced
this pull request
Feb 28, 2026
…eground/errorForeground tokens onto main's refactored version Reset TerminalOutput.tsx to main's latest version (which includes hook extraction refactors from PR RunMaestro#470), then re-applied the 4 theme token replacements from fix/theme-integration: 1. Search highlight (JSX): theme.mode ternary → theme.colors.warningForeground 2. Search highlight (HTML string): same pattern in innerHTML mark tag 3. STDERR badge: hardcoded '#fff' → theme.colors.errorForeground 4. Delete confirm button: hardcoded '#fff' → theme.colors.errorForeground Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This was referenced Feb 28, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Continue the App.tsx refactoring effort (Phase 2) by extracting three large domain hooks, reducing App.tsx from ~6,600 lines to ~4,600 lines. Also preserves Phase 3 store-subscription patterns in SessionList and MainPanel against upstream regressions.
Hook Extractions
useWizardHandlers(~1,250 lines, 58 tests) — Onboarding wizard, inline wizard, tour system, slash command discovery, and session launch logicuseSessionCrud(~470 lines, 52 tests) — Session/group CRUD operations (create, delete, rename, duplicate, drag-and-drop, bookmark, worktree management)useInterruptHandler(~460 lines, 21 tests) — Agent interrupt logic with provider-specific stop mechanisms, batch/group-chat interrupt coordinationPhase 3 Preservation
Upstream polish work (#460) reverted Phase 3B/3C by moving store subscriptions back into prop-assembly hooks. This PR restores the intended pattern:
useSessionStore,useUIStore,useSettingsStore,useBatchStore,useGroupChatStore, andgetModalActions()— not via propsuseUIStoreanduseSettingsStore— not via propsuseSessionListPropsanduseMainPanelPropsremain slim, passing only computed values and domain handlersStats
Test plan
npx tsc --noEmitpasses with zero errorsnpm run test— 21,016 tests pass, 0 failuresnpm run lint:eslint— zero errorsnpx prettier --check "src/**/*.{ts,tsx}"— all files formattedSummary by CodeRabbit
New Features
Bug Fixes
Tests
Chores