feat: Disk-based prompt system with user customization UI#541
Open
peterjmorgan wants to merge 25 commits intoRunMaestro:mainfrom
Open
feat: Disk-based prompt system with user customization UI#541peterjmorgan wants to merge 25 commits intoRunMaestro:mainfrom
peterjmorgan wants to merge 25 commits intoRunMaestro:mainfrom
Conversation
Creates the core PromptManager that loads all 19 system prompts from disk at startup, with support for user customizations stored in a separate JSON file. Follows the same architecture pattern as SpecKit/OpenSpec managers. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…tforms Adds electron-builder extraResources entries for mac, win, and linux to copy src/prompts/*.md files to prompts/core/ in the packaged app, enabling the disk-based prompt manager to load prompts from Resources at runtime. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Exposes window.maestro.prompts namespace with get, getAll, getAllIds, save, and reset methods so the renderer can access, edit, and reset system prompts with immediate in-memory effect. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Calls initializePrompts() in app.whenReady() after stats DB init but before setupIpcHandlers() and window creation. Includes error handling that shows a dialog and quits if prompts fail to load. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace compiled prompt imports with getPrompt() in group-chat-router.ts and group-chat-agent.ts. Add CLI-specific async getCliPrompt() loader in batch-processor.ts since CLI doesn't use Electron's startup sequence. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace direct imports from compiled prompts module with IPC-based loading via window.maestro.prompts.get() in useInputProcessing.ts, settingsStore.ts, and agentStore.ts. Prompts are cached in module-level variables and loaded once at app startup via useAppInitialization. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ding Replace direct imports from prompts module with IPC-based loading via window.maestro.prompts.get() in contextGroomer, contextSummarizer, wizardPrompts, and phaseGenerator. Each file now uses module-level caching with async loaders and synchronous getter functions. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Create promptInit.ts that loads all renderer prompts (input processing, context groomer, context summarizer, wizard, phase generator, settings store) via a single idempotent init function. Replaces individual loader calls in useAppInitialization with the centralized initializeRendererPrompts(). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Prompts are now loaded from disk at runtime, so the compile-time prompt generation step is no longer needed. Removed from all build/dev scripts, CI workflows, and Windows dev tooling. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…rate remaining imports Replace src/prompts/index.ts re-exports from generated/prompts with a PROMPT_IDS constant map providing type-safe prompt ID strings. This completes the migration away from compile-time prompt generation. - Add tab-naming and director-notes to CORE_PROMPTS in prompt-manager.ts - Migrate 3 main-process files to use getPrompt() from prompt-manager (group-chat-moderator, tabNaming, director-notes) - Migrate 6 renderer files to disk-based IPC loading via cached prompts (inlineWizardConversation, inlineWizardDocumentGeneration, batchUtils, useWizardHandlers, useAgentListeners, useMergeTransferHandlers) - Register 3 new prompt loaders in promptInit.ts - Update 6 test files with appropriate mocks for new loading pattern Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ts migration The generated prompts build system has been fully replaced by disk-based loading. The src/generated/ directory is no longer produced, so its gitignore entry is dead weight. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…g core system prompts Creates MaestroPromptsTab component following SpecKit panel patterns with category grouping, inline editing, save/reset functionality, and modified indicators. Integrates into RightPanel as a fourth tab. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Adds goToMaestroPrompts shortcut following the existing right panel tab shortcut pattern (goToAutoRun = Cmd+Shift+1). Registered in shortcuts.ts, useMainKeyboardHandler.ts, and QuickActionsModal.tsx. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Tests cover initialization, caching, user customization overlay, save/reset flows, error handling, and re-initialization guard. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… docs Add two new entries to the Key Files table for prompt customization and new prompt creation. Add a Prompt Customization section explaining the Maestro Prompts UI, how it works, and reset behavior. Create a user-facing docs page (docs/maestro-prompts.md) with full usage guide. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Update lazy-load comment that still referenced removed generated/prompts path to describe the actual reason for lazy-loading. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughCore prompts moved from compile-time bundle to runtime disk loading: added a prompt-manager, preload IPC surface and main IPC handlers, initialized prompts at startup (fatal on init failure), added renderer loaders and orchestration, a Maestro Prompts UI tab (edit/save/reset), removed the prompt generator and build:prompts step, and updated many consumers and tests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Main as Main
participant PM as prompt-manager
participant FS as FileSystem
participant IPC as Main IPC
participant Pre as Preload
participant Renderer as Renderer
Main->>PM: initializePrompts()
PM->>FS: read bundled prompts (resources/prompts/core or src/prompts)
FS-->>PM: .md contents
PM->>FS: read userData/core-prompts-customizations.json (if exists)
FS-->>PM: customizations
PM->>PM: merge -> in-memory cache
PM-->>Main: initialized
Main->>Renderer: open window
Renderer->>Pre: access window.maestro.prompts
Renderer->>IPC: invoke prompts:getAll / prompts:get
IPC->>PM: getAllPrompts() / getPrompt(id)
PM-->>IPC: prompt data
IPC-->>Renderer: responses
Renderer->>Renderer: render MaestroPromptsTab
Renderer->>IPC: prompts:save(id, content)
IPC->>PM: savePrompt(id, content)
PM->>FS: write customizations JSON
FS-->>PM: saved
PM-->>IPC: success
IPC-->>Renderer: saved -> update UI
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR replaces the build-time Key changes:
Issues found:
Confidence Score: 3/5
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
226
to
233
| // Select the base prompt based on mode | ||
| let basePrompt: string; | ||
| if (mode === 'iterate') { | ||
| basePrompt = wizardInlineIteratePrompt; | ||
| basePrompt = cachedWizardInlineIteratePrompt; | ||
| } else { | ||
| // 'new' mode uses the new plan prompt | ||
| basePrompt = wizardInlineNewPrompt; | ||
| basePrompt = cachedWizardInlineNewPrompt; | ||
| } |
There was a problem hiding this comment.
Silent empty prompt when load fails
If loadInlineWizardConversationPrompts() fails (or the IPC call returns success: false), both cachedWizardInlineIteratePrompt and cachedWizardInlineNewPrompt remain empty strings. Unlike other services in this PR (e.g., contextGroomer.ts, contextSummarizer.ts, wizardPrompts.ts, phaseGenerator.ts) which throw a descriptive error when the cached value is empty, generateInlineWizardPrompt uses these cached prompts directly, silently sending an empty system prompt to the AI.
Add guard functions to match the pattern used elsewhere:
function getWizardInlineIteratePrompt(): string {
if (!cachedWizardInlineIteratePrompt) {
throw new Error('Inline wizard iterate prompt not loaded');
}
return cachedWizardInlineIteratePrompt;
}
function getWizardInlineNewPrompt(): string {
if (!cachedWizardInlineNewPrompt) {
throw new Error('Inline wizard new prompt not loaded');
}
return cachedWizardInlineNewPrompt;
}Then replace the direct cache references at lines 229 and 232.
Comment on lines
+387
to
+388
| const basePrompt = | ||
| mode === 'iterate' ? wizardInlineIterateGenerationPrompt : wizardDocumentGenerationPrompt; | ||
| mode === 'iterate' ? cachedWizardInlineIterateGenerationPrompt : cachedWizardDocumentGenerationPrompt; |
There was a problem hiding this comment.
Silent empty prompt when load fails
Same issue as inlineWizardConversation.ts — if loadInlineWizardDocGenPrompts() failed, both cachedWizardDocumentGenerationPrompt and cachedWizardInlineIterateGenerationPrompt are empty strings, and generateDocumentPrompt silently uses empty prompts as basePrompt. Add guard functions (as done in other wizard services) to throw a clear error if either cache is empty when accessed.
Comment on lines
+99
to
+111
| const handleSelectPrompt = useCallback( | ||
| (prompt: CorePromptEntry) => { | ||
| if (hasUnsavedChanges) { | ||
| // Discard unsaved changes on switch | ||
| } | ||
| setSelectedPromptId(prompt.id); | ||
| setEditedContent(prompt.content); | ||
| setHasUnsavedChanges(false); | ||
| setSuccessMessage(null); | ||
| setError(null); | ||
| }, | ||
| [hasUnsavedChanges] | ||
| ); |
There was a problem hiding this comment.
Unsaved changes silently discarded on prompt switch
The if (hasUnsavedChanges) block is empty — a user who switches to another prompt after editing will lose their changes silently without any warning. Add a confirmation dialog:
const handleSelectPrompt = useCallback(
(prompt: CorePromptEntry) => {
if (hasUnsavedChanges) {
const confirm = window.confirm(
'You have unsaved changes. Switch anyway and discard them?'
);
if (!confirm) return;
}
setSelectedPromptId(prompt.id);
setEditedContent(prompt.content);
setHasUnsavedChanges(false);
setSuccessMessage(null);
setError(null);
},
[hasUnsavedChanges]
);
Comment on lines
+40
to
+43
| // Default batch processing prompt (exported for use by BatchRunnerModal and playbook management) | ||
| export const DEFAULT_BATCH_PROMPT = autorunDefaultPrompt; | ||
| // Updated via loadBatchPrompts() at startup; live ES module binding ensures importers see the value. | ||
| // eslint-disable-next-line import/no-mutable-exports | ||
| export let DEFAULT_BATCH_PROMPT = ''; |
There was a problem hiding this comment.
Mutable exported let relies on fragile live-binding semantics
DEFAULT_BATCH_PROMPT is exported as a mutable let and modified inside loadBatchPrompts(). The pattern assumes ES module live bindings, but this doesn't hold in CommonJS bundles or depending on Vite's output format. Any consumer that imports this value before loadBatchPrompts() runs will see the initial empty string.
Export a getter function instead, consistent with the pattern used elsewhere in this PR:
export function getDefaultBatchPrompt(): string {
return cachedAutorunDefaultPrompt;
}Remove the mutable export and eslint-disable comment, then update callers.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/ipc/handlers/director-notes.ts (1)
233-306:⚠️ Potential issue | 🟠 MajorPreserve the
lookbackDays <= 0"all time" behavior here.
getUnifiedHistoryalready treats0as no cutoff, but this path turns0intoDate.now(). An "all time" synopsis would therefore exclude historical entries and describe the window incorrectly.Suggested fix
- const cutoffTime = Date.now() - options.lookbackDays * 24 * 60 * 60 * 1000; + const cutoffTime = + options.lookbackDays > 0 + ? Date.now() - options.lookbackDays * 24 * 60 * 60 * 1000 + : 0; @@ - const cutoffDate = new Date(cutoffTime).toLocaleDateString('en-US', { + const cutoffDate = new Date(cutoffTime || Date.now()).toLocaleDateString('en-US', { month: 'short', day: 'numeric', year: 'numeric', }); @@ const prompt = [ getPrompt('director-notes'), '', '---', '', '## Session History Files', '', - `Lookback period: ${options.lookbackDays} days (${cutoffDate} – ${nowDate})`, - `Timestamp cutoff: ${cutoffTime} (only consider entries with timestamp >= this value)`, + options.lookbackDays > 0 + ? `Lookback period: ${options.lookbackDays} days (${cutoffDate} – ${nowDate})` + : 'Lookback period: all time', + options.lookbackDays > 0 + ? `Timestamp cutoff: ${cutoffTime} (only consider entries with timestamp >= this value)` + : 'Timestamp cutoff: none', `${agentCount} agents had ${entryCount} qualifying entries.`, '', manifestLines, ].join('\n');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/ipc/handlers/director-notes.ts` around lines 233 - 306, The code currently computes cutoffTime as Date.now() - options.lookbackDays... which converts options.lookbackDays === 0 into a recent cutoff; preserve the "all time" behavior by treating non-positive lookbackDays as no cutoff: set cutoffTime to 0 (or null) when options.lookbackDays <= 0, adjust the entry filtering in the loop that uses historyManager.getEntries(sessionId) to count all entries when cutoffTime is falsy, and change the prompt strings (the cutoffDate / lookback description and the "Timestamp cutoff" line) to show "all time" or "no cutoff" when options.lookbackDays <= 0; update references to cutoffTime, cutoffDate, and the prompt assembly so sessionManifest, agentCount, and entryCount reflect the full history in that case.src/main/group-chat/group-chat-router.ts (1)
830-838:⚠️ Potential issue | 🟡 MinorUse function replacers for template interpolation.
These replacements now inject user/session text like
message,historyContext, and paths. With string replacements, sequences like$&,$1, and$$are interpreted specially byString.prototype.replace, so shell snippets and regex examples can be corrupted before the participant sees them.Safer interpolation pattern
- .replace(/\{\{MESSAGE\}\}/g, message) + .replace(/\{\{MESSAGE\}\}/g, () => message)Apply the same callback form to the other placeholders in both chains.
Also applies to: 1374-1385
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/group-chat/group-chat-router.ts` around lines 830 - 838, The current chain building participantPrompt uses String.prototype.replace with replacement strings for placeholders (in the getPrompt('group-chat-participant-request') flow) which can corrupt user/session content containing $&, $1, $$, etc.; change each .replace(...) that supplies a replacement string for placeholders like PARTICIPANT_NAME, GROUP_CHAT_NAME, HISTORY_CONTEXT, MESSAGE, GROUP_CHAT_FOLDER, READ_ONLY_LABEL, READ_ONLY_NOTE and READ_ONLY_INSTRUCTION to use the callback replacer form (e.g., (_, ) => value) so the replacement treats the value literally; apply the same change to the other similar replacement chain referenced around lines 1374-1385 to ensure message, historyContext and path content are not interpreted by the replace engine.
🧹 Nitpick comments (4)
src/__tests__/renderer/stores/agentStore.test.ts (1)
116-129: Keep the prompt-ID contract intact in this mock.Mocking
PROMPT_IDSas{}means any code path that accidentally starts using a prompt ID now receivesundefined, and this suite can still stay green because the getter mocks mask it. Re-exporting the real IDs, or at least stubbing the specific IDs used here, would keep these tests catching prompt-resolution regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/renderer/stores/agentStore.test.ts` around lines 116 - 129, The PROMPT_IDS mock currently returns an empty object which hides prompt-ID regressions; update the mock of the prompts module (the PROMPT_IDS export) to re-export the real PROMPT_IDS or to explicitly stub the specific IDs used in tests (e.g., the IDs consumed by getMaestroSystemPrompt/getImageOnlyPrompt) so code paths receive valid IDs; ensure the vi.mock that also overrides getMaestroSystemPrompt and getImageOnlyPrompt still spreads the actual module and only replaces those getters while leaving PROMPT_IDS populated.src/renderer/components/Wizard/services/wizardPrompts.ts (1)
44-47: Import statement should be at the top of the file.The import on line 44 appears after the function definitions (lines 9-43). Move this import to join the other import on line 8 for proper module organization.
import { getRandomInitialQuestion } from './fillerPhrases'; +import { + substituteTemplateVariables, + type TemplateContext, +} from '../../../utils/templateVariables'; + // Module-level prompt cache let cachedWizardSystemPrompt: string = '';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/Wizard/services/wizardPrompts.ts` around lines 44 - 47, The import for substituteTemplateVariables and TemplateContext is placed after function definitions; move that import so it sits with the other top-of-file imports (i.e., join the existing import block near the top) to keep module imports organized and before any functions or logic (look for substituteTemplateVariables and TemplateContext to locate the misplaced import).src/renderer/hooks/batch/batchUtils.ts (1)
36-38: Inconsistent error handling: getter returns empty string instead of throwing.Unlike
getWizardSystemPrompt()in wizardPrompts.ts which throws when the prompt isn't loaded, this getter silently returns an empty string. This inconsistency could lead to subtle bugs where the synopsis prompt is used empty without any indication of a loading failure.♻️ Proposed fix for consistency
export function getAutorunSynopsisPrompt(): string { + if (!cachedAutorunSynopsisPrompt) { + console.warn('[BatchUtils] Autorun synopsis prompt not loaded, returning empty string'); + } return cachedAutorunSynopsisPrompt; }Or, for strict consistency with wizardPrompts.ts:
export function getAutorunSynopsisPrompt(): string { + if (!cachedAutorunSynopsisPrompt) { + throw new Error('Autorun synopsis prompt not loaded'); + } return cachedAutorunSynopsisPrompt; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/hooks/batch/batchUtils.ts` around lines 36 - 38, The getter getAutorunSynopsisPrompt currently returns an empty string when the prompt isn't loaded; change it to mirror getWizardSystemPrompt by throwing a clear error if cachedAutorunSynopsisPrompt is falsy (e.g., empty string or undefined) so callers can't silently proceed with an empty prompt; update the function to check cachedAutorunSynopsisPrompt and throw a descriptive Error (e.g., "Autorun synopsis prompt not loaded") instead of returning "".src/prompts/index.ts (1)
15-56: Keep prompt IDs in one shared definition.This map duplicates the literals already hardcoded in
src/main/prompt-manager.ts. Any drift compiles cleanly but becomes a runtimeUnknown prompt IDfailure. Consider moving the shared prompt definitions into a neutral module and deriving bothPROMPT_IDSandCORE_PROMPTSfrom it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/prompts/index.ts` around lines 15 - 56, PROMPT_IDS is duplicated vs the literals in prompt-manager.ts causing possible runtime "Unknown prompt ID" drift; extract the single source of truth (e.g., an array or const object of prompt keys) into a neutral module and import it from both src/prompts/index.ts and src/main/prompt-manager.ts, then derive PROMPT_IDS (the const) and CORE_PROMPTS (the constant used in prompt-manager.ts) from that exported source so both modules share the same definitions (refer to PROMPT_IDS and CORE_PROMPTS to locate where to replace the hardcoded literals).
🤖 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/maestro-prompts.md`:
- Around line 45-47: Update the wording that currently uses the literal example
path `Resources/prompts/core/` so it doesn't imply a universal packaged layout;
change that line to refer to "the app's bundled resources prompts directory" or
prefix the path with "example:" (e.g., "example: Resources/prompts/core/") and
add a short note that resource locations may vary by platform and that this is
an example path; ensure the other lines about
`userData/core-prompts-customizations.json` and the fallback behavior remain
unchanged.
In `@src/__tests__/main/group-chat/group-chat-moderator.test.ts`:
- Around line 47-52: The getPrompt mock currently returns an empty string for
unknown IDs which can hide typos; modify the vi.fn mock for getPrompt (used in
the group-chat-moderator tests) so that instead of returning '' on unrecognized
id it throws an Error (including the unknown id in the message) to fail fast
when a test calls getPrompt with an unexpected prompt id like
'group-chat-moderator-system' / 'group-chat-moderator-synthesis' /
'group-chat-participant' / 'group-chat-participant-request'.
In `@src/__tests__/main/ipc/handlers/director-notes.test.ts`:
- Around line 55-60: The mock implementation of getPrompt in the vi.mock block
should fail fast for unknown prompt IDs instead of returning an empty string;
update the mocked getPrompt (used in
src/__tests__/main/ipc/handlers/director-notes.test.ts) so it returns the
expected string for 'director-notes' and throws an Error (or similar) for any
other id to surface typos or contract changes immediately rather than silently
passing with ''. This change is in the vi.mock call and the getPrompt mock
function.
In `@src/__tests__/main/ipc/handlers/tabNaming.test.ts`:
- Around line 36-41: The mock for getPrompt in the test currently returns '' for
unexpected ids which hides typos; update the mock in tabNaming.test.ts (the
vi.mock for '../../../../main/prompt-manager' and its getPrompt implementation)
so that when id !== 'tab-naming' it throws an error (or uses vi.fn(() => { throw
new Error(...) })) instead of returning an empty string, ensuring any lookup
typos cause the test to fail and surface incorrect prompt IDs.
In `@src/cli/services/batch-processor.ts`:
- Around line 27-55: Wrap the generator body of runPlaybook in a try/finally so
that unregisterCliActivity(session.id) is always called even if getCliPrompt (or
any prompt/file lookup) throws; i.e., locate the runPlaybook function (the
generator that registers CLI activity), move the existing body into a try block
and put unregisterCliActivity(session.id) into the finally block to guarantee
the busy state is cleared on errors such as unknown prompt IDs or missing files.
In `@src/main/index.ts`:
- Line 80: initializePrompts is called but the IPC handlers for prompts are
never wired — ensure registerPromptsHandlers() is invoked during startup before
the renderer can call window.maestro.prompts.*; either add
registerPromptsHandlers() inside setupIpcHandlers() or call
registerPromptsHandlers() immediately after initializePrompts() and before
creating/restoring the BrowserWindow so get/getAll/getAllIds/save/reset channels
are registered.
In `@src/main/prompt-manager.ts`:
- Around line 216-229: savePrompt and resetPrompt perform an unguarded
read-modify-write on the same core-prompts-customizations.json which allows
overlapping IPC requests to clobber each other; introduce a single in-process
serialization mechanism (e.g., a module-scoped mutex or promise-based write
queue named something like promptsWriteLock or promptsWriteQueue) and wrap the
entire read/modify/save sequence (calls to loadUserCustomizations and
saveUserCustomizations inside savePrompt and resetPrompt) in
acquire/await/release semantics so only one mutation runs at a time, ensuring
all updates to the customizations file are serialized.
- Around line 118-124: The loadUserCustomizations function currently swallows
all errors; change it to only return null when the file is missing (error.code
=== 'ENOENT'), and rethrow any other fs errors or JSON.parse errors so they
surface to startup; after successfully reading and parsing the file
(JSON.parse), validate/normalize the parsed object into a proper StoredData
shape (fill missing fields with defaults and coerce types) before returning it
to avoid callers mutating a partially-invalid structure; refer to
loadUserCustomizations, getCustomizationsPath, and the StoredData type when
locating where to add the ENOENT check, JSON parse error handling, and
normalization step.
In `@src/renderer/components/MaestroPromptsTab.tsx`:
- Around line 99-110: handleSelectPrompt currently overwrites editedContent when
hasUnsavedChanges is true, causing silent loss of edits; update
handleSelectPrompt to either (A) show a confirmation/save discard modal when
hasUnsavedChanges is true and only call setSelectedPromptId, setEditedContent,
setHasUnsavedChanges, setSuccessMessage, setError after the user confirms, or
(B) implement a per-prompt draft map (e.g., drafts: Record<string,string>) and
on selection save the current editedContent into drafts[currentPromptId] and
load drafts[prompt.id] into setEditedContent so switching never loses the draft;
locate handleSelectPrompt and the related state setters (hasUnsavedChanges,
setEditedContent, setSelectedPromptId) and apply one of these fixes.
In `@src/renderer/components/RightPanel.tsx`:
- Around line 523-527: The MaestroPromptsTab is being unmounted when
activeRightTab changes, dropping its local state
(editedContent/hasUnsavedChanges); instead always mount <MaestroPromptsTab> and
toggle its visibility so its internal state persists: render the component
unconditionally in RightPanel (leave the <MaestroPromptsTab theme={theme} /> JSX
in place) and wrap it in a container whose className or style uses
activeRightTab === 'maestro-prompts' ? 'visible' : 'hidden' (or similar CSS like
display:none) to hide/show the panel without unmounting; keep the existing
data-tour and className attributes but switch the conditional rendering logic
from removing the node to only changing visibility so editedContent and
hasUnsavedChanges remain intact across tab switches.
In `@src/renderer/components/Wizard/services/phaseGenerator.ts`:
- Around line 15-23: The function loadPhaseGeneratorPrompts currently sets
phaseGeneratorPromptsLoaded = true even when
window.maestro.prompts.get('wizard-document-generation') fails or returns no
content, preventing retries; update loadPhaseGeneratorPrompts to only set
phaseGeneratorPromptsLoaded = true after a successful fetch (result.success &&
result.content), add error handling/logging when get() rejects or returns empty
content (use try/catch around window.maestro.prompts.get and log the error or
rethrow), and ensure cachedWizardDocumentGenerationPrompt is only assigned on
success so callers can detect absence and retry.
In `@src/renderer/components/Wizard/services/wizardPrompts.ts`:
- Around line 14-29: loadWizardPrompts currently marks wizardPromptsLoaded =
true even when prompt fetches fail, causing misleading "not loaded" errors
later; update loadWizardPrompts to check systemResult.success and
continuationResult.success and if either is false, log a warning (e.g.,
console.warn with the result objects) and throw a descriptive Error (e.g.,
"Failed to load wizard prompts") instead of setting wizardPromptsLoaded to true;
only set cachedWizardSystemPrompt, cachedWizardSystemContinuationPrompt and
wizardPromptsLoaded = true when the corresponding results are successful;
reference loadWizardPrompts, wizardPromptsLoaded, cachedWizardSystemPrompt,
cachedWizardSystemContinuationPrompt and getWizardSystemPrompt when making the
change.
In `@src/renderer/hooks/batch/batchUtils.ts`:
- Around line 15-31: The loadBatchPrompts function silently proceeds when prompt
fetches fail; update it to detect and log failures from
window.maestro.prompts.get calls (for 'autorun-default' and 'autorun-synopsis')
so missing content is visible — e.g., when defaultResult.success is false or
defaultResult.content is falsy, call the logger with a clear message referencing
'autorun-default' and include the returned error/status; do the same for
synopsisResult for 'autorun-synopsis' and ensure you still only set
cachedAutorunDefaultPrompt, DEFAULT_BATCH_PROMPT, and
cachedAutorunSynopsisPrompt when content exists; finally, set
batchPromptsLoaded=true after logging so state is consistent but failures are
recorded (use the same function name loadBatchPrompts and the result variables
defaultResult/synopsisResult in your messages).
In `@src/renderer/hooks/input/useInputProcessing.ts`:
- Around line 24-39: loadInputProcessingPrompts currently hydrates
cachedImageOnlyPrompt/cachedMaestroSystemPrompt once and sets
inputProcessingPromptsLoaded, which prevents subsequent updates and treats
empty-string prompts as missing; change it so prompt caching does not
permanently freeze: remove or bypass the one-shot lock
(inputProcessingPromptsLoaded) or add an explicit invalidate function, and
ensure loadInputProcessingPrompts (and callers like
getImageOnlyPrompt/getMaestroSystemPrompt) will re-fetch when invalidated or on
use; also set cachedImageOnlyPrompt/cachedMaestroSystemPrompt based on
result.success (not truthiness of result.content) so empty-string customizations
are preserved, and only mark the cache as loaded after handling success flags
appropriately.
In `@src/renderer/services/contextGroomer.ts`:
- Around line 24-58: Currently the cache uses empty-string sentinels and a
single write-once flag which makes a saved-blank prompt indistinguishable from
“not loaded” and prevents refreshes; change cachedContextGroomingPrompt and
cachedContextTransferPrompt from '' to nullable (string | null) and replace the
single contextGroomerPromptsLoaded boolean with per-prompt loaded flags (e.g.
groomingLoaded, transferLoaded) or a small map object; in
loadContextGroomerPrompts always assign the fetched content to the corresponding
cache even when it's an empty string and set that prompt’s loaded flag (do not
short-circuit future calls), and update
getContextGroomingPrompt/getContextTransfer to check the corresponding loaded
flag (not truthiness of the string) and return the cached value (which may be
''), throwing only if that specific prompt isn’t loaded so callers can retry.
In `@src/renderer/services/contextSummarizer.ts`:
- Around line 42-50: The loadContextSummarizerPrompts function currently sets
contextSummarizerPromptsLoaded = true even when
window.maestro.prompts.get('context-summarize') fails or returns no content;
change it so that you only set contextSummarizerPromptsLoaded to true after
successfully assigning cachedContextSummarizePrompt from result.content, and if
result.success is false or result.content is empty throw an error (or propagate)
so callers like getContextSummarizePrompt can retry or handle the failure;
update loadContextSummarizerPrompts to validate result.success && result.content
before caching and marking loaded, and throw a descriptive error otherwise.
In `@src/renderer/services/inlineWizardConversation.ts`:
- Around line 27-41: loadInlineWizardConversationPrompts currently sets
inlineWizardConversationPromptsLoaded = true even if one or both IPC reads fail,
allowing generateInlineWizardPrompt to run with empty prompts; change the
function (loadInlineWizardConversationPrompts) to require both iterateResult and
newResult to be successful and have content before setting
inlineWizardConversationPromptsLoaded and assigning
cachedWizardInlineIteratePrompt / cachedWizardInlineNewPrompt; if either result
is missing or unsuccessful, do not flip the loaded flag and instead throw a
descriptive error (or return a failed promise) so startup can fail fast and the
loader remains retryable; also ensure generateInlineWizardPrompt checks those
cachedWizardInlineIteratePrompt and cachedWizardInlineNewPrompt values rather
than relying solely on inlineWizardConversationPromptsLoaded.
In `@src/renderer/services/inlineWizardDocumentGeneration.ts`:
- Around line 26-40: The loader loadInlineWizardDocGenPrompts currently sets
inlineWizardDocGenPromptsLoaded even if one of the prompt IPC calls failed,
which lets generateDocumentPrompt run with empty
cachedWizardDocumentGenerationPrompt or
cachedWizardInlineIterateGenerationPrompt; change the logic so you only set
inlineWizardDocGenPromptsLoaded = true when both docGenResult.success &&
docGenResult.content AND iterateGenResult.success && iterateGenResult.content
are true, otherwise throw or return an error so startup/reporting can handle it
(alternatively explicitly log and rethrow the failure) — update checks around
cachedWizardDocumentGenerationPrompt and
cachedWizardInlineIterateGenerationPrompt in loadInlineWizardDocGenPrompts and
ensure generateDocumentPrompt does not proceed when the cache flags or cached
values are missing.
In `@src/renderer/services/promptInit.ts`:
- Around line 28-49: The in-flight initPromise is left as a rejected promise on
failure causing permanent module poison; in the catch block of the async IIFE
that assigns initPromise, reset that in-flight reference (e.g., set initPromise
back to null/undefined) before rethrowing the error so subsequent calls to
initializeRendererPrompts() can retry; locate the catch that currently logs via
console.error('[PromptInit] Failed to load renderer prompts:', error) and clear
initPromise there, then rethrow the error.
In `@src/renderer/stores/settingsStore.ts`:
- Around line 48-57: The code marks commitPromptLoaded true and overwrites
DEFAULT_AI_COMMANDS[0].prompt even when
window.maestro.prompts.get('commit-command') fails or returns empty; change
loadSettingsStorePrompts so it only sets commitPromptLoaded = true (or throws)
after a non-empty result.content is returned, and only assign
cachedCommitCommandPrompt and DEFAULT_AI_COMMANDS[0].prompt when result.success
&& result.content is truthy; if you prefer fail-fast, throw an error when the
prompt fetch fails so startup surfaces the problem. Apply the identical fix to
the other similar prompt-loading block that currently flips commitPromptLoaded
and overwrites the default prompt on failure.
---
Outside diff comments:
In `@src/main/group-chat/group-chat-router.ts`:
- Around line 830-838: The current chain building participantPrompt uses
String.prototype.replace with replacement strings for placeholders (in the
getPrompt('group-chat-participant-request') flow) which can corrupt user/session
content containing $&, $1, $$, etc.; change each .replace(...) that supplies a
replacement string for placeholders like PARTICIPANT_NAME, GROUP_CHAT_NAME,
HISTORY_CONTEXT, MESSAGE, GROUP_CHAT_FOLDER, READ_ONLY_LABEL, READ_ONLY_NOTE and
READ_ONLY_INSTRUCTION to use the callback replacer form (e.g., (_, ) => value)
so the replacement treats the value literally; apply the same change to the
other similar replacement chain referenced around lines 1374-1385 to ensure
message, historyContext and path content are not interpreted by the replace
engine.
In `@src/main/ipc/handlers/director-notes.ts`:
- Around line 233-306: The code currently computes cutoffTime as Date.now() -
options.lookbackDays... which converts options.lookbackDays === 0 into a recent
cutoff; preserve the "all time" behavior by treating non-positive lookbackDays
as no cutoff: set cutoffTime to 0 (or null) when options.lookbackDays <= 0,
adjust the entry filtering in the loop that uses
historyManager.getEntries(sessionId) to count all entries when cutoffTime is
falsy, and change the prompt strings (the cutoffDate / lookback description and
the "Timestamp cutoff" line) to show "all time" or "no cutoff" when
options.lookbackDays <= 0; update references to cutoffTime, cutoffDate, and the
prompt assembly so sessionManifest, agentCount, and entryCount reflect the full
history in that case.
---
Nitpick comments:
In `@src/__tests__/renderer/stores/agentStore.test.ts`:
- Around line 116-129: The PROMPT_IDS mock currently returns an empty object
which hides prompt-ID regressions; update the mock of the prompts module (the
PROMPT_IDS export) to re-export the real PROMPT_IDS or to explicitly stub the
specific IDs used in tests (e.g., the IDs consumed by
getMaestroSystemPrompt/getImageOnlyPrompt) so code paths receive valid IDs;
ensure the vi.mock that also overrides getMaestroSystemPrompt and
getImageOnlyPrompt still spreads the actual module and only replaces those
getters while leaving PROMPT_IDS populated.
In `@src/prompts/index.ts`:
- Around line 15-56: PROMPT_IDS is duplicated vs the literals in
prompt-manager.ts causing possible runtime "Unknown prompt ID" drift; extract
the single source of truth (e.g., an array or const object of prompt keys) into
a neutral module and import it from both src/prompts/index.ts and
src/main/prompt-manager.ts, then derive PROMPT_IDS (the const) and CORE_PROMPTS
(the constant used in prompt-manager.ts) from that exported source so both
modules share the same definitions (refer to PROMPT_IDS and CORE_PROMPTS to
locate where to replace the hardcoded literals).
In `@src/renderer/components/Wizard/services/wizardPrompts.ts`:
- Around line 44-47: The import for substituteTemplateVariables and
TemplateContext is placed after function definitions; move that import so it
sits with the other top-of-file imports (i.e., join the existing import block
near the top) to keep module imports organized and before any functions or logic
(look for substituteTemplateVariables and TemplateContext to locate the
misplaced import).
In `@src/renderer/hooks/batch/batchUtils.ts`:
- Around line 36-38: The getter getAutorunSynopsisPrompt currently returns an
empty string when the prompt isn't loaded; change it to mirror
getWizardSystemPrompt by throwing a clear error if cachedAutorunSynopsisPrompt
is falsy (e.g., empty string or undefined) so callers can't silently proceed
with an empty prompt; update the function to check cachedAutorunSynopsisPrompt
and throw a descriptive Error (e.g., "Autorun synopsis prompt not loaded")
instead of returning "".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 566ca78a-f15b-4efa-8703-b082e8f16e40
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (57)
.github/workflows/ci.yml.gitignoreBUILDING_WINDOWS.mdCLAUDE.mddocs/docs.jsondocs/maestro-prompts.mdpackage.jsonscripts/generate-prompts.mjsscripts/start-dev.ps1src/__tests__/main/group-chat/group-chat-moderator.test.tssrc/__tests__/main/ipc/handlers/director-notes.test.tssrc/__tests__/main/ipc/handlers/prompts.test.tssrc/__tests__/main/ipc/handlers/tabNaming.test.tssrc/__tests__/main/preload/prompts.test.tssrc/__tests__/main/prompt-manager.test.tssrc/__tests__/renderer/components/BatchRunnerModal.test.tsxsrc/__tests__/renderer/components/MaestroPromptsTab.test.tsxsrc/__tests__/renderer/services/inlineWizardDocumentGeneration.test.tssrc/__tests__/renderer/stores/agentStore.test.tssrc/cli/index.tssrc/cli/services/batch-processor.tssrc/main/group-chat/group-chat-agent.tssrc/main/group-chat/group-chat-moderator.tssrc/main/group-chat/group-chat-router.tssrc/main/index.tssrc/main/ipc/handlers/director-notes.tssrc/main/ipc/handlers/index.tssrc/main/ipc/handlers/prompts.tssrc/main/ipc/handlers/tabNaming.tssrc/main/preload/index.tssrc/main/preload/prompts.tssrc/main/prompt-manager.tssrc/prompts/index.tssrc/renderer/components/MaestroPromptsTab.tsxsrc/renderer/components/QuickActionsModal.tsxsrc/renderer/components/RightPanel.tsxsrc/renderer/components/Wizard/services/phaseGenerator.tssrc/renderer/components/Wizard/services/wizardPrompts.tssrc/renderer/constants/shortcuts.tssrc/renderer/global.d.tssrc/renderer/hooks/agent/useAgentListeners.tssrc/renderer/hooks/agent/useMergeTransferHandlers.tssrc/renderer/hooks/batch/batchUtils.tssrc/renderer/hooks/batch/useAutoRunHandlers.tssrc/renderer/hooks/input/index.tssrc/renderer/hooks/input/useInputProcessing.tssrc/renderer/hooks/keyboard/useMainKeyboardHandler.tssrc/renderer/hooks/ui/useAppInitialization.tssrc/renderer/hooks/wizard/useWizardHandlers.tssrc/renderer/services/contextGroomer.tssrc/renderer/services/contextSummarizer.tssrc/renderer/services/inlineWizardConversation.tssrc/renderer/services/inlineWizardDocumentGeneration.tssrc/renderer/services/promptInit.tssrc/renderer/stores/agentStore.tssrc/renderer/stores/settingsStore.tssrc/renderer/types/index.ts
💤 Files with no reviewable changes (3)
- .github/workflows/ci.yml
- .gitignore
- scripts/generate-prompts.mjs
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (7)
src/renderer/services/inlineWizardConversation.ts (1)
32-50: Potential race condition on concurrent initialization calls.If
loadInlineWizardConversationPromptsis called multiple times before the first call completes (e.g., rapid component mounts), multiple IPC round-trips will occur since the check on line 33 only guards against completed loads. This is wasteful but not incorrect since the prompts are idempotent.Consider guarding with an in-flight promise if this becomes a hot path:
♻️ Optional fix to deduplicate concurrent calls
let cachedWizardInlineIteratePrompt = ''; let cachedWizardInlineNewPrompt = ''; let inlineWizardConversationPromptsLoaded = false; +let loadingPromise: Promise<void> | null = null; export async function loadInlineWizardConversationPrompts(force = false): Promise<void> { if (inlineWizardConversationPromptsLoaded && !force) return; + if (loadingPromise && !force) return loadingPromise; - const [iterateResult, newResult] = await Promise.all([ + loadingPromise = (async () => { + const [iterateResult, newResult] = await Promise.all([ window.maestro.prompts.get('wizard-inline-iterate'), window.maestro.prompts.get('wizard-inline-new'), - ]); + ]); - if (!iterateResult.success || iterateResult.content === undefined) { - throw new Error(iterateResult.error || 'Failed to load prompt: wizard-inline-iterate'); - } - if (!newResult.success || newResult.content === undefined) { - throw new Error(newResult.error || 'Failed to load prompt: wizard-inline-new'); - } + if (!iterateResult.success || iterateResult.content === undefined) { + throw new Error(iterateResult.error || 'Failed to load prompt: wizard-inline-iterate'); + } + if (!newResult.success || newResult.content === undefined) { + throw new Error(newResult.error || 'Failed to load prompt: wizard-inline-new'); + } - cachedWizardInlineIteratePrompt = iterateResult.content; - cachedWizardInlineNewPrompt = newResult.content; - inlineWizardConversationPromptsLoaded = true; + cachedWizardInlineIteratePrompt = iterateResult.content; + cachedWizardInlineNewPrompt = newResult.content; + inlineWizardConversationPromptsLoaded = true; + })(); + + try { + await loadingPromise; + } finally { + loadingPromise = null; + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/services/inlineWizardConversation.ts` around lines 32 - 50, The function loadInlineWizardConversationPrompts currently only checks inlineWizardConversationPromptsLoaded and can trigger duplicate IPC calls; introduce a module-scoped in-flight promise (e.g., inlineWizardConversationPromptsLoadingPromise) and at the start of loadInlineWizardConversationPrompts return that promise if present, otherwise set it to the Promise that performs the current logic; ensure you clear or unset inlineWizardConversationPromptsLoadingPromise on completion or rejection and still set inlineWizardConversationPromptsLoaded and cachedWizardInlineIteratePrompt/cachedWizardInlineNewPrompt as before so concurrent callers share the same initialization work.src/renderer/components/Wizard/services/wizardPrompts.ts (1)
19-39: Error handling improvement looks good.The implementation now correctly throws errors before setting
wizardPromptsLoaded = true, ensuring the loaded flag accurately reflects successful loading state. This properly addresses the previous concern about misleading "not loaded" errors.One minor observation: if
loadWizardPrompts()is called concurrently (e.g., from multiple components during initialization), both calls will execute the full loading sequence sincewizardPromptsLoadedremainsfalseuntil after theawait. This isn't a correctness issue but could be optimized by caching the in-flight promise.♻️ Optional: Cache the loading promise to deduplicate concurrent calls
let cachedWizardSystemPrompt: string = ''; let cachedWizardSystemContinuationPrompt: string = ''; let wizardPromptsLoaded = false; +let loadingPromise: Promise<void> | null = null; export async function loadWizardPrompts(force = false): Promise<void> { - if (wizardPromptsLoaded && !force) return; + if (wizardPromptsLoaded && !force) return; + if (loadingPromise && !force) return loadingPromise; + loadingPromise = (async () => { const [systemResult, continuationResult] = await Promise.all([ window.maestro.prompts.get('wizard-system'), window.maestro.prompts.get('wizard-system-continuation'), ]); if (!systemResult.success || systemResult.content === undefined) { throw new Error(systemResult.error || 'Failed to load prompt: wizard-system'); } if (!continuationResult.success || continuationResult.content === undefined) { throw new Error( continuationResult.error || 'Failed to load prompt: wizard-system-continuation' ); } cachedWizardSystemPrompt = systemResult.content; cachedWizardSystemContinuationPrompt = continuationResult.content; wizardPromptsLoaded = true; + })(); + + try { + await loadingPromise; + } finally { + loadingPromise = null; + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/Wizard/services/wizardPrompts.ts` around lines 19 - 39, Add deduplication for concurrent loadWizardPrompts() calls by introducing an in-flight promise (e.g., wizardPromptsLoadPromise) that is set at the start of loadWizardPrompts and returned/awaited by subsequent callers; ensure the promise respects the force flag (force should create a new in-flight promise), set cachedWizardSystemPrompt/cachedWizardSystemContinuationPrompt and wizardPromptsLoaded only after the promise resolves successfully, and clear/reset the in-flight promise on error so future calls can retry.src/renderer/components/Wizard/services/phaseGenerator.ts (2)
20-29: Good fix for the silent failure issue; consider also catching empty content.The fix properly addresses the past review concern—the loaded flag is now only set on success, and failures throw an error. However, the check
result.content === undefinedwouldn't catch an empty string''returned from an empty prompt file.🛡️ Suggested improvement to also reject empty content
export async function loadPhaseGeneratorPrompts(force = false): Promise<void> { if (phaseGeneratorPromptsLoaded && !force) return; const result = await window.maestro.prompts.get('wizard-document-generation'); - if (!result.success || result.content === undefined) { + if (!result.success || !result.content) { throw new Error(result.error || 'Failed to load prompt: wizard-document-generation'); } cachedWizardDocumentGenerationPrompt = result.content; phaseGeneratorPromptsLoaded = true; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/Wizard/services/phaseGenerator.ts` around lines 20 - 29, In loadPhaseGeneratorPrompts, the current check only rejects undefined content but allows empty strings; update the success condition for window.maestro.prompts.get('wizard-document-generation') so that result.content is validated (e.g., !== undefined && result.content !== null && String(result.content).trim() !== '') and treat empty/whitespace content as a failure (throwing the same Error and not setting cachedWizardDocumentGenerationPrompt or phaseGeneratorPromptsLoaded) so the loaded flag is only set on truly valid prompt content.
600-601: Consider adding defensive prompt loading at the start ofgenerateDocuments.While the PR design loads prompts at app startup, adding
await loadPhaseGeneratorPrompts()early in this method would make it more self-contained and resilient to scenarios where startup loading is bypassed (e.g., during testing or development). The call is a no-op if prompts are already loaded.🛡️ Suggested defensive loading
try { + // Ensure prompts are loaded (no-op if already loaded at startup) + await loadPhaseGeneratorPrompts(); + // Get the agent configuration wizardDebugLogger.log('info', 'Fetching agent configuration', {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/Wizard/services/phaseGenerator.ts` around lines 600 - 601, Add defensive prompt loading at the start of generateDocuments: call await loadPhaseGeneratorPrompts() near the top of the generateDocuments function so prompts are guaranteed to be available even if app startup loading was skipped (the function is a no-op if already loaded). Locate generateDocuments in phaseGenerator.ts and insert the awaited loadPhaseGeneratorPrompts() call before any prompt-dependent operations (e.g., before retrieving agent configuration or calling buildPhasePrompt); keep existing error handling intact.src/cli/services/batch-processor.ts (1)
27-56: Refactor to import prompt definitions from catalog instead of duplicating the mapping.The filenameMap in
getCliPrompt()duplicates the source-of-truth definitions fromsrc/prompts/catalog.ts(CORE_PROMPT_ENTRIES). This creates maintenance drift risk—if new CLI-compatible prompts are added to catalog, they won't be available here without manual updates.Consider importing
CORE_PROMPT_DEFINITIONSfrom catalog and filtering/mapping it as needed, or creating a dedicated export in catalog for CLI-specific prompts.Note: The dev path calculation
path.join(__dirname, '..', '..', 'prompts', filename)is correct and resolves tosrc/prompts/as intended. No change needed there.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/services/batch-processor.ts` around lines 27 - 56, Replace the hardcoded filenameMap in getCliPrompt with an import from the prompts catalog (e.g., CORE_PROMPT_ENTRIES or CORE_PROMPT_DEFINITIONS from src/prompts/catalog.ts) and derive the id->filename mapping by filtering/mapping the catalog entries (select the entries intended for CLI, or add an export for CLI-specific prompts if necessary); update getCliPrompt to look up the filename from that derived map and keep the existing devPath/bundledPath read-and-cache logic intact (symbols to change: getCliPrompt, filenameMap -> use CORE_PROMPT_ENTRIES/CORE_PROMPT_DEFINITIONS).src/main/group-chat/group-chat-router.ts (1)
43-43: UsePROMPT_IDS.GROUP_CHAT_PARTICIPANT_REQUESTinstead of duplicating the literal ID.The catalog was added to prevent prompt-ID drift, but these new call sites reintroduce a runtime-only string literal. Importing the shared constant gives you rename safety and keeps the main/renderer prompt paths aligned.
Refactor sketch
import { buildAgentArgs, applyAgentConfigOverrides, getContextWindowValue, } from '../utils/agent-args'; import { getPrompt } from '../prompt-manager'; +import { PROMPT_IDS } from '../../prompts'; import { wrapSpawnWithSsh } from '../utils/ssh-spawn-wrapper';- getPrompt('group-chat-participant-request'), + getPrompt(PROMPT_IDS.GROUP_CHAT_PARTICIPANT_REQUEST),- const basePrompt = applyPromptTemplate(getPrompt('group-chat-participant-request'), { + const basePrompt = applyPromptTemplate( + getPrompt(PROMPT_IDS.GROUP_CHAT_PARTICIPANT_REQUEST), + { PARTICIPANT_NAME: participantName, GROUP_CHAT_NAME: chat.name, READ_ONLY_NOTE: readOnlyNote, GROUP_CHAT_FOLDER: groupChatFolder, HISTORY_CONTEXT: historyContext, READ_ONLY_LABEL: readOnlyLabel, MESSAGE: 'Please continue from where you left off based on the recovery context below.', READ_ONLY_INSTRUCTION: readOnlyInstruction, - }); + } + );Also applies to: 838-850, 1386-1395
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/group-chat/group-chat-router.ts` at line 43, Replace the runtime string literal prompt ID with the shared constant: import PROMPT_IDS and use PROMPT_IDS.GROUP_CHAT_PARTICIPANT_REQUEST wherever getPrompt(...) is called in group-chat-router.ts (and the other noted call sites) instead of the duplicated literal; update the imports to include PROMPT_IDS and change calls like getPrompt("group_chat_participant_request", ...) to getPrompt(PROMPT_IDS.GROUP_CHAT_PARTICIPANT_REQUEST, ...) so the code uses the canonical prompt ID constant.src/renderer/hooks/input/useInputProcessing.ts (1)
15-18:DEFAULT_IMAGE_ONLY_PROMPTexport creates a misleading API surface.The constant is exported as an empty string while
getImageOnlyPrompt()returns the actual cached prompt. This creates confusion:
- Tests mock
DEFAULT_IMAGE_ONLY_PROMPTwith'Describe this image'(seeuseInputHandlers.test.ts:90-100)- Production code exports
''- Consumers might use the constant instead of the getter, getting wrong behavior
Consider removing the constant export or making it clearly named (e.g.,
IMAGE_ONLY_PROMPT_FALLBACK) to indicate it's not the runtime value.Also applies to: 51-51
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/hooks/input/useInputProcessing.ts` around lines 15 - 18, DEFAULT_IMAGE_ONLY_PROMPT currently exports an empty string while getImageOnlyPrompt() returns the runtime cached value, which is confusing for consumers and tests; either remove the exported DEFAULT_IMAGE_ONLY_PROMPT or rename it to a clear fallback constant (e.g., IMAGE_ONLY_PROMPT_FALLBACK) and update usages/tests to call getImageOnlyPrompt() for the runtime prompt; specifically change the export of DEFAULT_IMAGE_ONLY_PROMPT and any references to it, ensure getImageOnlyPrompt() reads from cachedImageOnlyPrompt, and update test mocks (use the new fallback name or mock getImageOnlyPrompt()) so production and tests use the same API surface.
🤖 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/BatchRunnerModal.test.tsx`:
- Around line 17-30: The test setup must force-reload the batch prompt cache to
avoid order-dependent failures: update the beforeAll to call
loadBatchPrompts(true) (instead of loadBatchPrompts()) after setting
window.maestro.prompts so the module reinitializes and getDefaultBatchPrompt()
returns the prompts defined in this suite; alternatively, reset the module
between suites if you prefer, but the simplest fix is to call
loadBatchPrompts(true) here.
In `@src/main/group-chat/group-chat-router.ts`:
- Around line 112-117: The applyPromptTemplate function currently mutates
rendered per-token causing nested/late tokens inside user content (e.g., MESSAGE
or HISTORY_CONTEXT) to be re-expanded; change applyPromptTemplate to perform a
single-pass replacement by building one RegExp that matches all placeholder keys
at once (escape token names) and use a single replace callback that looks up the
matched token in the replacements map and returns the value (or the original
match if missing), so placeholders are substituted atomically and internal
content is not reprocessed.
In `@src/main/prompt-manager.ts`:
- Around line 260-278: The deletion of user customization happens before reading
the bundled file, risking irreversible loss if fs.readFile fails; in the
withSerializedCustomizationMutation block, call getBundledPromptsPath and await
fs.readFile(filePath, 'utf-8') to obtain bundledContent (using def.filename)
before modifying customizations, then only delete customizations.prompts[id] and
await saveUserCustomizations(customizations) after the read succeeds; finally
update promptCache.set(id, { content: bundledContent, isModified: false }) and
logger.info as before.
In `@src/renderer/components/BatchRunnerModal.tsx`:
- Line 103: BatchRunnerModal currently calls getDefaultBatchPrompt()
unconditionally during render which throws if loadBatchPrompts() hasn’t
finished; change BatchRunnerModal to stop invoking getDefaultBatchPrompt()
during render — either accept a defaultBatchPrompt prop from the caller
(AppModals) which already knows prompts are initialized, or gate the call behind
a prompts-ready check/flag (e.g., promptsReady or similar) and only call
getDefaultBatchPrompt() inside a useEffect or conditional once
loadBatchPrompts() has completed; update any usages (notably where AppModals
mounts BatchRunnerModal) to provide the loaded default or ensure the modal isn’t
mounted until prompts are initialized.
- Around line 405-406: The current hasUnsavedChanges logic prevents saving a
reset-to-default because it requires prompt !== defaultBatchPrompt; change
hasUnsavedChanges so it is true whenever the prompt differs from savedPrompt OR
when the savedPrompt was a customization but the user reset to the default (i.e.
savedPrompt !== defaultBatchPrompt and prompt === defaultBatchPrompt).
Concretely, update the hasUnsavedChanges expression (used alongside isModified)
to something like: hasUnsavedChanges = prompt !== savedPrompt || (savedPrompt
!== defaultBatchPrompt && prompt === defaultBatchPrompt), referencing prompt,
savedPrompt, defaultBatchPrompt, isModified and the Save action to locate where
to apply the change.
In `@src/renderer/hooks/symphony/useSymphonyContribution.ts`:
- Line 25: The handler creates the Auto Run config using getDefaultBatchPrompt()
before the batch-prompt cache is ready, causing throws and half-created
sessions; fix by ensuring loadBatchPrompts() has completed (await or an
ensureLoaded helper) before calling getDefaultBatchPrompt() and before
committing the new session / scheduling startBatchRun(); update the handler in
useSymphonyContribution.ts to await loadBatchPrompts() (or check a loaded flag)
prior to building the Auto Run config and before invoking startBatchRun().
In `@src/renderer/hooks/wizard/useWizardHandlers.ts`:
- Around line 38-43: Before calling the synchronous guards
getAutorunSynopsisPrompt() and getDefaultBatchPrompt() ensure the renderer
prompts are fully initialized by awaiting the async prompt loader; add an await
call to the existing async initializer (e.g., ensureRendererPromptsLoaded() or
promptService.initializePrompts()) immediately before each use of
getAutorunSynopsisPrompt and getDefaultBatchPrompt (locations around the current
usages and also at the other occurrences mentioned). This guarantees the
/history initialization completes before invoking the getters and prevents
exceptions during auto-run/onboarding setup.
In `@src/renderer/services/inlineWizardDocumentGeneration.ts`:
- Around line 49-60: The getters getWizardDocumentGenerationPrompt and
getWizardInlineIterateGenerationPrompt currently throw if prompts aren't loaded;
change them to load the cache on demand by making each function async, and when
inlineWizardDocGenPromptsLoaded is false await loadInlineWizardDocGenPrompts()
before returning cachedWizardDocumentGenerationPrompt or
cachedWizardInlineIterateGenerationPrompt. Also update call sites (e.g.,
generateInlineDocuments and the other usage around the referenced lines) to
await the new async getters or explicitly await loadInlineWizardDocGenPrompts()
beforehand so the prompts are guaranteed loaded.
In `@src/renderer/stores/settingsStore.ts`:
- Around line 48-59: loadSettingsStorePrompts mutates
DEFAULT_AI_COMMANDS[0].prompt in place which won't trigger Zustand subscribers;
instead, after loading cachedCommitCommandPrompt in loadSettingsStorePrompts,
create a new commands array (copy DEFAULT_AI_COMMANDS with the updated prompt)
and update the settings store via its setter (e.g., setCustomAICommands or
equivalent) so the store receives a new array reference; do the same wherever
DEFAULT_AI_COMMANDS is updated (also around lines 148-154) to ensure UIs see the
change rather than mutating the original DEFAULT_AI_COMMANDS object.
---
Nitpick comments:
In `@src/cli/services/batch-processor.ts`:
- Around line 27-56: Replace the hardcoded filenameMap in getCliPrompt with an
import from the prompts catalog (e.g., CORE_PROMPT_ENTRIES or
CORE_PROMPT_DEFINITIONS from src/prompts/catalog.ts) and derive the id->filename
mapping by filtering/mapping the catalog entries (select the entries intended
for CLI, or add an export for CLI-specific prompts if necessary); update
getCliPrompt to look up the filename from that derived map and keep the existing
devPath/bundledPath read-and-cache logic intact (symbols to change:
getCliPrompt, filenameMap -> use CORE_PROMPT_ENTRIES/CORE_PROMPT_DEFINITIONS).
In `@src/main/group-chat/group-chat-router.ts`:
- Line 43: Replace the runtime string literal prompt ID with the shared
constant: import PROMPT_IDS and use PROMPT_IDS.GROUP_CHAT_PARTICIPANT_REQUEST
wherever getPrompt(...) is called in group-chat-router.ts (and the other noted
call sites) instead of the duplicated literal; update the imports to include
PROMPT_IDS and change calls like getPrompt("group_chat_participant_request",
...) to getPrompt(PROMPT_IDS.GROUP_CHAT_PARTICIPANT_REQUEST, ...) so the code
uses the canonical prompt ID constant.
In `@src/renderer/components/Wizard/services/phaseGenerator.ts`:
- Around line 20-29: In loadPhaseGeneratorPrompts, the current check only
rejects undefined content but allows empty strings; update the success condition
for window.maestro.prompts.get('wizard-document-generation') so that
result.content is validated (e.g., !== undefined && result.content !== null &&
String(result.content).trim() !== '') and treat empty/whitespace content as a
failure (throwing the same Error and not setting
cachedWizardDocumentGenerationPrompt or phaseGeneratorPromptsLoaded) so the
loaded flag is only set on truly valid prompt content.
- Around line 600-601: Add defensive prompt loading at the start of
generateDocuments: call await loadPhaseGeneratorPrompts() near the top of the
generateDocuments function so prompts are guaranteed to be available even if app
startup loading was skipped (the function is a no-op if already loaded). Locate
generateDocuments in phaseGenerator.ts and insert the awaited
loadPhaseGeneratorPrompts() call before any prompt-dependent operations (e.g.,
before retrieving agent configuration or calling buildPhasePrompt); keep
existing error handling intact.
In `@src/renderer/components/Wizard/services/wizardPrompts.ts`:
- Around line 19-39: Add deduplication for concurrent loadWizardPrompts() calls
by introducing an in-flight promise (e.g., wizardPromptsLoadPromise) that is set
at the start of loadWizardPrompts and returned/awaited by subsequent callers;
ensure the promise respects the force flag (force should create a new in-flight
promise), set cachedWizardSystemPrompt/cachedWizardSystemContinuationPrompt and
wizardPromptsLoaded only after the promise resolves successfully, and
clear/reset the in-flight promise on error so future calls can retry.
In `@src/renderer/hooks/input/useInputProcessing.ts`:
- Around line 15-18: DEFAULT_IMAGE_ONLY_PROMPT currently exports an empty string
while getImageOnlyPrompt() returns the runtime cached value, which is confusing
for consumers and tests; either remove the exported DEFAULT_IMAGE_ONLY_PROMPT or
rename it to a clear fallback constant (e.g., IMAGE_ONLY_PROMPT_FALLBACK) and
update usages/tests to call getImageOnlyPrompt() for the runtime prompt;
specifically change the export of DEFAULT_IMAGE_ONLY_PROMPT and any references
to it, ensure getImageOnlyPrompt() reads from cachedImageOnlyPrompt, and update
test mocks (use the new fallback name or mock getImageOnlyPrompt()) so
production and tests use the same API surface.
In `@src/renderer/services/inlineWizardConversation.ts`:
- Around line 32-50: The function loadInlineWizardConversationPrompts currently
only checks inlineWizardConversationPromptsLoaded and can trigger duplicate IPC
calls; introduce a module-scoped in-flight promise (e.g.,
inlineWizardConversationPromptsLoadingPromise) and at the start of
loadInlineWizardConversationPrompts return that promise if present, otherwise
set it to the Promise that performs the current logic; ensure you clear or unset
inlineWizardConversationPromptsLoadingPromise on completion or rejection and
still set inlineWizardConversationPromptsLoaded and
cachedWizardInlineIteratePrompt/cachedWizardInlineNewPrompt as before so
concurrent callers share the same initialization work.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 646516f1-1b67-42f4-b43e-29ec15676dae
📒 Files selected for processing (37)
docs/maestro-prompts.mdsrc/__tests__/main/group-chat/group-chat-moderator.test.tssrc/__tests__/main/ipc/handlers/director-notes.test.tssrc/__tests__/main/ipc/handlers/tabNaming.test.tssrc/__tests__/main/prompt-manager.test.tssrc/__tests__/renderer/components/BatchRunnerModal.test.tsxsrc/__tests__/renderer/components/MaestroPromptsTab.test.tsxsrc/__tests__/renderer/components/RightPanel.test.tsxsrc/__tests__/renderer/hooks/batch/batchUtils.test.tssrc/__tests__/renderer/hooks/input/useInputProcessing.prompts.test.tssrc/__tests__/renderer/hooks/useInputProcessing.test.tssrc/__tests__/renderer/hooks/useSymphonyContribution.test.tssrc/__tests__/renderer/hooks/useWizardHandlers.test.tssrc/__tests__/renderer/stores/agentStore.test.tssrc/cli/services/batch-processor.tssrc/main/group-chat/group-chat-router.tssrc/main/ipc/handlers/director-notes.tssrc/main/prompt-manager.tssrc/prompts/catalog.tssrc/prompts/index.tssrc/renderer/components/BatchRunnerModal.tsxsrc/renderer/components/MaestroPromptsTab.tsxsrc/renderer/components/RightPanel.tsxsrc/renderer/components/Wizard/services/phaseGenerator.tssrc/renderer/components/Wizard/services/wizardPrompts.tssrc/renderer/hooks/batch/batchUtils.tssrc/renderer/hooks/batch/index.tssrc/renderer/hooks/batch/usePlaybookManagement.tssrc/renderer/hooks/input/useInputProcessing.tssrc/renderer/hooks/symphony/useSymphonyContribution.tssrc/renderer/hooks/wizard/useWizardHandlers.tssrc/renderer/services/contextGroomer.tssrc/renderer/services/contextSummarizer.tssrc/renderer/services/inlineWizardConversation.tssrc/renderer/services/inlineWizardDocumentGeneration.tssrc/renderer/services/promptInit.tssrc/renderer/stores/settingsStore.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- src/tests/renderer/stores/agentStore.test.ts
- src/tests/main/ipc/handlers/director-notes.test.ts
- src/tests/renderer/components/MaestroPromptsTab.test.tsx
- src/tests/main/prompt-manager.test.ts
- src/renderer/components/MaestroPromptsTab.tsx
- src/renderer/services/contextSummarizer.ts
- src/tests/main/ipc/handlers/tabNaming.test.ts
- src/renderer/components/RightPanel.tsx
- src/tests/main/group-chat/group-chat-moderator.test.ts
Comment on lines
+49
to
+60
| function getWizardDocumentGenerationPrompt(): string { | ||
| if (!inlineWizardDocGenPromptsLoaded) { | ||
| throw new Error('Inline wizard document generation prompt not loaded'); | ||
| } | ||
| return cachedWizardDocumentGenerationPrompt; | ||
| } | ||
|
|
||
| function getWizardInlineIterateGenerationPrompt(): string { | ||
| if (!inlineWizardDocGenPromptsLoaded) { | ||
| throw new Error('Inline wizard iterate generation prompt not loaded'); | ||
| } | ||
| return cachedWizardInlineIterateGenerationPrompt; |
There was a problem hiding this comment.
Make document generation load its prompt cache on demand.
These getters now throw until loadInlineWizardDocGenPrompts() runs, but generateInlineDocuments() does not ensure that preload happened first. That turns a startup-order race into a hard failure for the first inline wizard generation.
💡 Suggested fix
export async function generateInlineDocuments(
config: DocumentGenerationConfig
): Promise<DocumentGenerationResult> {
+ await loadInlineWizardDocGenPrompts();
+
const { agentType, directoryPath, autoRunFolderPath, projectName, callbacks } = config;Also applies to: 407-409
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/services/inlineWizardDocumentGeneration.ts` around lines 49 -
60, The getters getWizardDocumentGenerationPrompt and
getWizardInlineIterateGenerationPrompt currently throw if prompts aren't loaded;
change them to load the cache on demand by making each function async, and when
inlineWizardDocGenPromptsLoaded is false await loadInlineWizardDocGenPrompts()
before returning cachedWizardDocumentGenerationPrompt or
cachedWizardInlineIterateGenerationPrompt. Also update call sites (e.g.,
generateInlineDocuments and the other usage around the referenced lines) to
await the new async getters or explicitly await loadInlineWizardDocGenPrompts()
beforehand so the prompts are guaranteed loaded.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/renderer/hooks/symphony/useSymphonyContribution.ts (1)
31-35:⚠️ Potential issue | 🟠 MajorChange
startBatchRundependency type toPromise<void>and handle the rejection.
useBatchProcessorexportsstartBatchRunas an async function returningPromise<void>, but the dependency inUseSymphonyContributionDeps(line 33) is typed as=> void. The call site at line 260 fire-and-forgets the call insidesetTimeout, causing unhandled promise rejections if batch startup fails—after the session is created, modal is closed, and the Auto Run tab is activated.Suggested fix
export interface UseSymphonyContributionDeps { /** Start a batch run for a session */ - startBatchRun: (sessionId: string, config: BatchRunConfig, folderPath: string) => void; + startBatchRun: ( + sessionId: string, + config: BatchRunConfig, + folderPath: string + ) => Promise<void>; /** Ref to input element for focus */ inputRef: React.RefObject<HTMLTextAreaElement | null>; }setTimeout(() => { console.log( '[Symphony] Auto-starting batch run with', batchConfig.documents.length, 'documents' ); - startBatchRun(newId, batchConfig, data.autoRunPath!); + void startBatchRun(newId, batchConfig, data.autoRunPath!).catch((error) => { + console.error('[Symphony] Failed to auto-start batch run:', error); + notifyToast({ + type: 'error', + title: 'Symphony Error', + message: 'Failed to start Auto Run.', + }); + }); }, 500);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/hooks/symphony/useSymphonyContribution.ts` around lines 31 - 35, The dependency type for startBatchRun in UseSymphonyContributionDeps is incorrect (typed as (sessionId, config, folderPath) => void) but useBatchProcessor exports it as an async function; update the type to return Promise<void> and ensure callers handle rejections: change the signature in UseSymphonyContributionDeps to startBatchRun: (sessionId: string, config: BatchRunConfig, folderPath: string) => Promise<void>, then at the call site that currently invokes startBatchRun inside setTimeout (the fire-and-forget invocation), either await the call inside an async callback or attach .catch(err => { /* log/handle */ }) to handle errors so unhandled promise rejections cannot occur.
♻️ Duplicate comments (2)
src/renderer/stores/settingsStore.ts (1)
48-74:⚠️ Potential issue | 🟠 MajorKeep
/commitunavailable until a non-empty prompt is loaded.
''currently counts as a successful fetch, setscommitPromptLoaded = true, and overwrites the built-in/commitcommand with blank text for the rest of the session. Require a non-empty prompt before updating the cache or store.💡 Suggested fix
- if (!result.success || result.content === undefined) { + if (!result.success || !result.content?.trim()) { throw new Error(result.error || 'Failed to load prompt: commit-command'); }🤖 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 48 - 74, The loadSettingsStorePrompts function currently treats an empty string as a successful prompt load and sets commitPromptLoaded/cachedCommitCommandPrompt and updates DEFAULT_AI_COMMANDS and useSettingsStore; change loadSettingsStorePrompts so after retrieving result.content you verify it's a non-empty string (e.g. result.content && result.content.trim().length > 0) before assigning cachedCommitCommandPrompt, setting commitPromptLoaded, or mutating DEFAULT_AI_COMMANDS and the store; if the content is empty, throw or return an error (same behavior as when result.success is false) so the built-in /commit remains unavailable until a non-empty prompt is loaded.src/renderer/services/inlineWizardDocumentGeneration.ts (1)
35-46:⚠️ Potential issue | 🟠 MajorDon't mark blank prompts as loaded.
''currently passes the success check, setsinlineWizardDocGenPromptsLoaded = true, and latergenerateDocumentPrompt()runs with empty instructions until reload. Reject empty/whitespace content and leave the cache unset so the feature can retry.💡 Suggested fix
- if (!docGenResult.success || docGenResult.content === undefined) { + if (!docGenResult.success || !docGenResult.content?.trim()) { throw new Error(docGenResult.error || 'Failed to load prompt: wizard-document-generation'); } - if (!iterateGenResult.success || iterateGenResult.content === undefined) { + if (!iterateGenResult.success || !iterateGenResult.content?.trim()) { throw new Error( iterateGenResult.error || 'Failed to load prompt: wizard-inline-iterate-generation' );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/services/inlineWizardDocumentGeneration.ts` around lines 35 - 46, The current success checks accept empty strings, causing cachedWizardDocumentGenerationPrompt and cachedWizardInlineIterateGenerationPrompt to be set and inlineWizardDocGenPromptsLoaded toggled true even when content is blank; update the validation for docGenResult.content and iterateGenResult.content to treat empty or whitespace-only strings as failures (e.g., use trim() and check length > 0), throw the same error when blank, and only assign to cachedWizardDocumentGenerationPrompt / cachedWizardInlineIterateGenerationPrompt and set inlineWizardDocGenPromptsLoaded = true when both contents are non-empty; this ensures generateDocumentPrompt() won’t run with empty instructions.
🤖 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/prompt-manager.ts`:
- Around line 228-248: Reject blank or whitespace-only core prompt content at
the persistence boundary by validating the content in savePrompt(id: string,
content: string) before persisting: trim the content and if it is empty throw a
descriptive Error (e.g., "Prompt content cannot be empty or whitespace") rather
than saving; only proceed to call withSerializedCustomizationMutation,
saveUserCustomizations and update promptCache.set(id, ...) when the trimmed
content is non-empty so blank customizations cannot be stored or applied.
In `@src/renderer/components/BatchRunnerModal.tsx`:
- Around line 162-170: The code currently treats an empty saved prompt as “no
prompt” by using || to fall back to initialDefaultBatchPrompt; update
initializations and any places noted (savedPrompt, prompt, initialPromptRef and
any similar uses around lines ~361-375 and ~443-445) to preserve an explicit
empty string by using a null/undefined-only check (e.g. ?? or initialPrompt ===
undefined) instead of ||, and ensure handleSave persists the exact string so
reopening the modal doesn't restore the default or miscompute dirty state;
reference variables: defaultBatchPrompt, prompt, savedPrompt, initialPromptRef,
initialPrompt, initialDefaultBatchPrompt, and function handleSave.
---
Outside diff comments:
In `@src/renderer/hooks/symphony/useSymphonyContribution.ts`:
- Around line 31-35: The dependency type for startBatchRun in
UseSymphonyContributionDeps is incorrect (typed as (sessionId, config,
folderPath) => void) but useBatchProcessor exports it as an async function;
update the type to return Promise<void> and ensure callers handle rejections:
change the signature in UseSymphonyContributionDeps to startBatchRun:
(sessionId: string, config: BatchRunConfig, folderPath: string) =>
Promise<void>, then at the call site that currently invokes startBatchRun inside
setTimeout (the fire-and-forget invocation), either await the call inside an
async callback or attach .catch(err => { /* log/handle */ }) to handle errors so
unhandled promise rejections cannot occur.
---
Duplicate comments:
In `@src/renderer/services/inlineWizardDocumentGeneration.ts`:
- Around line 35-46: The current success checks accept empty strings, causing
cachedWizardDocumentGenerationPrompt and
cachedWizardInlineIterateGenerationPrompt to be set and
inlineWizardDocGenPromptsLoaded toggled true even when content is blank; update
the validation for docGenResult.content and iterateGenResult.content to treat
empty or whitespace-only strings as failures (e.g., use trim() and check length
> 0), throw the same error when blank, and only assign to
cachedWizardDocumentGenerationPrompt / cachedWizardInlineIterateGenerationPrompt
and set inlineWizardDocGenPromptsLoaded = true when both contents are non-empty;
this ensures generateDocumentPrompt() won’t run with empty instructions.
In `@src/renderer/stores/settingsStore.ts`:
- Around line 48-74: The loadSettingsStorePrompts function currently treats an
empty string as a successful prompt load and sets
commitPromptLoaded/cachedCommitCommandPrompt and updates DEFAULT_AI_COMMANDS and
useSettingsStore; change loadSettingsStorePrompts so after retrieving
result.content you verify it's a non-empty string (e.g. result.content &&
result.content.trim().length > 0) before assigning cachedCommitCommandPrompt,
setting commitPromptLoaded, or mutating DEFAULT_AI_COMMANDS and the store; if
the content is empty, throw or return an error (same behavior as when
result.success is false) so the built-in /commit remains unavailable until a
non-empty prompt is loaded.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5e828d0d-516d-4839-842a-1288e996fdec
📒 Files selected for processing (11)
src/__tests__/main/prompt-manager.test.tssrc/__tests__/renderer/components/BatchRunnerModal.test.tsxsrc/__tests__/renderer/hooks/useSymphonyContribution.test.tssrc/__tests__/renderer/hooks/useWizardHandlers.test.tssrc/main/group-chat/group-chat-router.tssrc/main/prompt-manager.tssrc/renderer/components/BatchRunnerModal.tsxsrc/renderer/hooks/symphony/useSymphonyContribution.tssrc/renderer/hooks/wizard/useWizardHandlers.tssrc/renderer/services/inlineWizardDocumentGeneration.tssrc/renderer/stores/settingsStore.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/renderer/hooks/wizard/useWizardHandlers.ts
- src/tests/renderer/hooks/useWizardHandlers.test.ts
- src/tests/renderer/hooks/useSymphonyContribution.test.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/__tests__/renderer/hooks/useSymphonyContribution.test.ts (1)
1027-1034:⚠️ Potential issue | 🟠 MajorUse Vitest's async timer helpers for the auto-start assertions.
These blocks advance the clock synchronously, but the timeout callback fires a promise-returning
startBatchRun()that's not awaited. Using the synchronous timer API risks test flakiness or false negatives if the microtask queue isn't flushed before assertions run.Suggested change
- act(() => { - vi.advanceTimersByTime(500); - }); + await act(async () => { + await vi.advanceTimersByTimeAsync(500); + });Apply the same pattern to the other 500 ms auto-start tests in this block. In the rejection case, this also lets you drop the extra
await Promise.resolve()flush.Also applies to: 1049-1059, 1080-1091, 1109-1117, 1195-1216
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/renderer/hooks/useSymphonyContribution.test.ts` around lines 1027 - 1034, The test advances timers synchronously which can miss awaiting the promise-returning startBatchRun(); replace synchronous timer calls with Vitest's async timer helpers (e.g., await vi.advanceTimersByTimeAsync(500) or await vi.runAllTimersAsync()) so the timeout callback and subsequent promise resolution are awaited before asserting calls to startBatchRun and loadBatchPrompts; apply the same change to the other 500ms auto-start tests (the blocks around startBatchRun/loadBatchPrompts and the rejection case) and remove the now-unnecessary await Promise.resolve() flush in the rejection test.
🧹 Nitpick comments (3)
src/renderer/components/BatchRunnerModal.tsx (1)
363-396: Fix inconsistent indentation in the useEffect block.Lines 368 and 386-388 have extra leading tabs compared to the surrounding code. The
await loadBatchPrompts()and thecatchblock should align with the rest of the function body.🔧 Proposed fix
const loadDefaultPrompt = async () => { try { - await loadBatchPrompts(); - if (cancelled) return; + await loadBatchPrompts(); + if (cancelled) return; - const loadedDefaultPrompt = getDefaultBatchPrompt(); + const loadedDefaultPrompt = getDefaultBatchPrompt(); ... - } catch (error) { - console.error('[BatchRunnerModal] Failed to load default prompt:', error); - } + } catch (error) { + console.error('[BatchRunnerModal] Failed to load default prompt:', error); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/BatchRunnerModal.tsx` around lines 363 - 396, The useEffect contains inconsistent indentation inside the inner async function loadDefaultPrompt: align the await loadBatchPrompts() line and the catch block with the rest of the function body so all statements in loadDefaultPrompt are at the same indentation level; update the block around await loadBatchPrompts(), the getDefaultBatchPrompt()/setDefaultBatchPrompt/setPrompt/setSavedPrompt logic, and the catch(...) handler to match the function's indentation (the function name loadDefaultPrompt, the try { ... } and corresponding catch should be consistently indented) to fix formatting without changing any logic.src/renderer/hooks/symphony/useSymphonyContribution.ts (1)
136-136: Fix inconsistent indentation on session creation block.Lines 136 and 197 have an extra leading space/tab before the code. The
const newSessiondeclaration and its closing brace should align with the surrounding code.🔧 Proposed fix
- const newSession: Session = { + const newSession: Session = { id: newId, ... - }; + };Also applies to: 197-197
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/hooks/symphony/useSymphonyContribution.ts` at line 136, The session creation block 'const newSession: Session = {' is mis-indented (extra leading space/tab) along with its closing '}' — locate the 'const newSession: Session' declaration inside useSymphonyContribution and the matching closing brace and remove the extra leading whitespace so the declaration and its closing brace align with the surrounding code block; repeat the same fix for the second occurrence around the other 'const newSession: Session' instance (the one noted at the other location) to ensure both blocks use consistent indentation.src/__tests__/renderer/hooks/useSymphonyContribution.test.ts (1)
1182-1216: Add theloadBatchPrompts()failure case to this block.This now covers
startBatchRun()rejecting, but the new disk-backed prompt load is another async failure point in the same auto-start branch. A matching rejection test would protect the new prompt-loading integration and keep the toast/log behavior explicit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/renderer/hooks/useSymphonyContribution.test.ts` around lines 1182 - 1216, The test currently only simulates startBatchRun() rejecting; also add a parallel failure case for loadBatchPrompts() by creating a test (or extending this block) that injects a deps mock where loadBatchPrompts is a vi.fn().mockRejectedValue(new Error('prompt load failed')), then call useSymphonyContribution(deps) and invoke result.current.handleStartContribution(data) just like the existing flow; assert that console.error (spy) was called with '[Symphony] Failed to auto-start batch run:' and an Error, and that notifyToast was called with the same error toast payload — this ensures loadBatchPrompts rejection is exercised alongside startBatchRun in the auto-start branch.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/__tests__/renderer/hooks/useSymphonyContribution.test.ts`:
- Around line 1027-1034: The test advances timers synchronously which can miss
awaiting the promise-returning startBatchRun(); replace synchronous timer calls
with Vitest's async timer helpers (e.g., await vi.advanceTimersByTimeAsync(500)
or await vi.runAllTimersAsync()) so the timeout callback and subsequent promise
resolution are awaited before asserting calls to startBatchRun and
loadBatchPrompts; apply the same change to the other 500ms auto-start tests (the
blocks around startBatchRun/loadBatchPrompts and the rejection case) and remove
the now-unnecessary await Promise.resolve() flush in the rejection test.
---
Nitpick comments:
In `@src/__tests__/renderer/hooks/useSymphonyContribution.test.ts`:
- Around line 1182-1216: The test currently only simulates startBatchRun()
rejecting; also add a parallel failure case for loadBatchPrompts() by creating a
test (or extending this block) that injects a deps mock where loadBatchPrompts
is a vi.fn().mockRejectedValue(new Error('prompt load failed')), then call
useSymphonyContribution(deps) and invoke
result.current.handleStartContribution(data) just like the existing flow; assert
that console.error (spy) was called with '[Symphony] Failed to auto-start batch
run:' and an Error, and that notifyToast was called with the same error toast
payload — this ensures loadBatchPrompts rejection is exercised alongside
startBatchRun in the auto-start branch.
In `@src/renderer/components/BatchRunnerModal.tsx`:
- Around line 363-396: The useEffect contains inconsistent indentation inside
the inner async function loadDefaultPrompt: align the await loadBatchPrompts()
line and the catch block with the rest of the function body so all statements in
loadDefaultPrompt are at the same indentation level; update the block around
await loadBatchPrompts(), the
getDefaultBatchPrompt()/setDefaultBatchPrompt/setPrompt/setSavedPrompt logic,
and the catch(...) handler to match the function's indentation (the function
name loadDefaultPrompt, the try { ... } and corresponding catch should be
consistently indented) to fix formatting without changing any logic.
In `@src/renderer/hooks/symphony/useSymphonyContribution.ts`:
- Line 136: The session creation block 'const newSession: Session = {' is
mis-indented (extra leading space/tab) along with its closing '}' — locate the
'const newSession: Session' declaration inside useSymphonyContribution and the
matching closing brace and remove the extra leading whitespace so the
declaration and its closing brace align with the surrounding code block; repeat
the same fix for the second occurrence around the other 'const newSession:
Session' instance (the one noted at the other location) to ensure both blocks
use consistent indentation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 05cfaee7-87a5-4ecf-afb0-0b59e8ab3b44
📒 Files selected for processing (10)
src/__tests__/main/prompt-manager.test.tssrc/__tests__/renderer/components/BatchRunnerModal.test.tsxsrc/__tests__/renderer/hooks/useSymphonyContribution.test.tssrc/__tests__/renderer/services/inlineWizardDocumentGeneration.test.tssrc/__tests__/renderer/stores/settingsStore.test.tssrc/main/prompt-manager.tssrc/renderer/components/BatchRunnerModal.tsxsrc/renderer/hooks/symphony/useSymphonyContribution.tssrc/renderer/services/inlineWizardDocumentGeneration.tssrc/renderer/stores/settingsStore.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/tests/renderer/services/inlineWizardDocumentGeneration.test.ts
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/__tests__/renderer/hooks/useSymphonyContribution.test.ts (1)
61-64: Make the batch prompt mock enforce the load-before-read contract.
getDefaultBatchPromptalways succeeds here, so this suite would still pass if a future refactor called it beforeloadBatchPrompts()again. Have the mock throw untilloadBatchPromptsflips a loaded flag, or assert call order, so the original regression stays covered.🧪 Suggested mock shape
+let batchPromptsLoaded = false; + vi.mock('../../../renderer/hooks/batch/batchUtils', () => ({ - getDefaultBatchPrompt: vi.fn(() => 'mock-default-batch-prompt'), - loadBatchPrompts: vi.fn().mockResolvedValue(undefined), + getDefaultBatchPrompt: vi.fn(() => { + if (!batchPromptsLoaded) { + throw new Error('batch prompts not loaded'); + } + return 'mock-default-batch-prompt'; + }), + loadBatchPrompts: vi.fn().mockImplementation(async () => { + batchPromptsLoaded = true; + }), }));Also reset
batchPromptsLoaded = falseinbeforeEach.Also applies to: 191-191
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/renderer/hooks/useSymphonyContribution.test.ts` around lines 61 - 64, The current mock for getDefaultBatchPrompt always succeeds and can hide regressions; change the mock implementation so getDefaultBatchPrompt checks a shared flag (e.g., batchPromptsLoaded) and throws or returns an error until loadBatchPrompts sets that flag to true, and ensure loadBatchPrompts.mockResolvedValue sets batchPromptsLoaded = true; also reset batchPromptsLoaded = false in beforeEach so each test enforces the load-before-read contract; reference the mocked functions getDefaultBatchPrompt and loadBatchPrompts and the flag batchPromptsLoaded when making these changes.src/renderer/hooks/symphony/useSymphonyContribution.ts (1)
284-285: AddinputRefto theuseCallbackdeps.
handleStartContributioncloses overinputRef.currentvia the timeout at Line 259, butinputRefis omitted here. If the parent ever swaps the ref object, this callback will keep focusing the stale element.♻️ Proposed fix
- [sessions, defaultSaveToHistory, startBatchRun] + [sessions, defaultSaveToHistory, startBatchRun, inputRef]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/hooks/symphony/useSymphonyContribution.ts` around lines 284 - 285, The useCallback that defines handleStartContribution currently lists [sessions, defaultSaveToHistory, startBatchRun] as dependencies but omits inputRef; update the dependency array to include inputRef so the callback captures the latest ref object (i.e., change the deps to [sessions, defaultSaveToHistory, startBatchRun, inputRef]). Ensure this change is made where handleStartContribution is defined so the timeout that accesses inputRef.current uses the freshest ref.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/__tests__/renderer/hooks/useSymphonyContribution.test.ts`:
- Around line 61-64: The current mock for getDefaultBatchPrompt always succeeds
and can hide regressions; change the mock implementation so
getDefaultBatchPrompt checks a shared flag (e.g., batchPromptsLoaded) and throws
or returns an error until loadBatchPrompts sets that flag to true, and ensure
loadBatchPrompts.mockResolvedValue sets batchPromptsLoaded = true; also reset
batchPromptsLoaded = false in beforeEach so each test enforces the
load-before-read contract; reference the mocked functions getDefaultBatchPrompt
and loadBatchPrompts and the flag batchPromptsLoaded when making these changes.
In `@src/renderer/hooks/symphony/useSymphonyContribution.ts`:
- Around line 284-285: The useCallback that defines handleStartContribution
currently lists [sessions, defaultSaveToHistory, startBatchRun] as dependencies
but omits inputRef; update the dependency array to include inputRef so the
callback captures the latest ref object (i.e., change the deps to [sessions,
defaultSaveToHistory, startBatchRun, inputRef]). Ensure this change is made
where handleStartContribution is defined so the timeout that accesses
inputRef.current uses the freshest ref.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 11353f3b-c481-4b56-8f20-faaee87cc8b5
📒 Files selected for processing (3)
src/__tests__/renderer/hooks/useSymphonyContribution.test.tssrc/renderer/components/BatchRunnerModal.tsxsrc/renderer/hooks/symphony/useSymphonyContribution.ts
The prompts IPC handler was defined and exported but never registered, causing "No handler registered for 'prompts:getAll'" at runtime. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Author
|
I've completed all the fixes for issues I can figure out. Opus and Codex also went through the PR changes. I believe this is ready to go |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/renderer/hooks/input/useInputProcessing.ts (1)
25-47:⚠️ Potential issue | 🟠 MajorDon't make prompt reads depend on a hidden one-time bootstrap.
getImageOnlyPrompt()andgetMaestroSystemPrompt()now throw until some other code has awaitedloadInputProcessingPrompts().src/renderer/stores/agentStore.tsconsumes them directly while processing queued work, so any startup race or failed renderer init turns a normal send into a"prompt not loaded"failure instead of loading the prompt on demand. Please expose anensureInputProcessingPromptsLoaded()helper with in-flight dedupe, and have call sites await that before reading the cache.Suggested direction
let cachedImageOnlyPrompt = ''; let cachedMaestroSystemPrompt = ''; let inputProcessingPromptsLoaded = false; +let inputProcessingPromptsLoadPromise: Promise<void> | null = null; export async function loadInputProcessingPrompts(force = false): Promise<void> { if (inputProcessingPromptsLoaded && !force) return; const [imageResult, systemResult] = await Promise.all([ window.maestro.prompts.get('image-only-default'), window.maestro.prompts.get('maestro-system-prompt'), ]); @@ cachedImageOnlyPrompt = imageResult.content; cachedMaestroSystemPrompt = systemResult.content; inputProcessingPromptsLoaded = true; } + +export async function ensureInputProcessingPromptsLoaded(force = false): Promise<void> { + if (inputProcessingPromptsLoaded && !force) return; + if (!inputProcessingPromptsLoadPromise || force) { + inputProcessingPromptsLoadPromise = loadInputProcessingPrompts(force).finally(() => { + inputProcessingPromptsLoadPromise = null; + }); + } + await inputProcessingPromptsLoadPromise; +}Then the async send/queue paths can
await ensureInputProcessingPromptsLoaded()before calling the getters.Also applies to: 57-72
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/hooks/input/useInputProcessing.ts` around lines 25 - 47, loadInputProcessingPrompts currently must be awaited elsewhere or getters (getImageOnlyPrompt/getMaestroSystemPrompt) throw, causing race failures; add an ensureInputProcessingPromptsLoaded() helper that returns/awaits a single in-flight Promise to dedupe concurrent calls (use a module-level loadingPromise variable that is set when loadInputProcessingPrompts is invoked and cleared on success/failure), update consumers (e.g., agentStore async send/queue paths) to await ensureInputProcessingPromptsLoaded() before calling getImageOnlyPrompt/getMaestroSystemPrompt, and keep the existing cachedImageOnlyPrompt/cachedMaestroSystemPrompt and inputProcessingPromptsLoaded flags untouched so getters read the cache after the helper completes.
🧹 Nitpick comments (2)
src/main/group-chat/group-chat-router.ts (1)
845-857: Extract a shared builder for the participant request prompt.The replacement map is now duplicated in both the normal routing path and the recovery path. The next placeholder change in
group-chat-participant-request.mdwill be easy to update in one place and miss in the other.♻️ Suggested refactor
+type ParticipantRequestPromptParams = { + participantName: string; + groupChatName: string; + groupChatFolder: string; + historyContext: string; + message: string; + readOnlyNote: string; + readOnlyLabel: string; + readOnlyInstruction: string; +}; + +function buildParticipantRequestPrompt({ + participantName, + groupChatName, + groupChatFolder, + historyContext, + message, + readOnlyNote, + readOnlyLabel, + readOnlyInstruction, +}: ParticipantRequestPromptParams): string { + return applyPromptTemplate(getPrompt(PROMPT_IDS.GROUP_CHAT_PARTICIPANT_REQUEST), { + PARTICIPANT_NAME: participantName, + GROUP_CHAT_NAME: groupChatName, + READ_ONLY_NOTE: readOnlyNote, + GROUP_CHAT_FOLDER: groupChatFolder, + HISTORY_CONTEXT: historyContext, + READ_ONLY_LABEL: readOnlyLabel, + MESSAGE: message, + READ_ONLY_INSTRUCTION: readOnlyInstruction, + }); +} ... - const participantPrompt = applyPromptTemplate( - getPrompt(PROMPT_IDS.GROUP_CHAT_PARTICIPANT_REQUEST), - { - PARTICIPANT_NAME: participantName, - GROUP_CHAT_NAME: updatedChat.name, - READ_ONLY_NOTE: readOnlyNote, - GROUP_CHAT_FOLDER: groupChatFolder, - HISTORY_CONTEXT: historyContext, - READ_ONLY_LABEL: readOnlyLabel, - MESSAGE: message, - READ_ONLY_INSTRUCTION: readOnlyInstruction, - } - ); + const participantPrompt = buildParticipantRequestPrompt({ + participantName, + groupChatName: updatedChat.name, + groupChatFolder, + historyContext, + message, + readOnlyNote, + readOnlyLabel, + readOnlyInstruction, + }); ... - const basePrompt = applyPromptTemplate(getPrompt(PROMPT_IDS.GROUP_CHAT_PARTICIPANT_REQUEST), { - PARTICIPANT_NAME: participantName, - GROUP_CHAT_NAME: chat.name, - READ_ONLY_NOTE: readOnlyNote, - GROUP_CHAT_FOLDER: groupChatFolder, - HISTORY_CONTEXT: historyContext, - READ_ONLY_LABEL: readOnlyLabel, - MESSAGE: 'Please continue from where you left off based on the recovery context below.', - READ_ONLY_INSTRUCTION: readOnlyInstruction, - }); + const basePrompt = buildParticipantRequestPrompt({ + participantName, + groupChatName: chat.name, + groupChatFolder, + historyContext, + message: 'Please continue from where you left off based on the recovery context below.', + readOnlyNote, + readOnlyLabel, + readOnlyInstruction, + });Also applies to: 1399-1408
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/group-chat/group-chat-router.ts` around lines 845 - 857, Extract the duplicated replacement map into a single helper (e.g., buildParticipantPrompt) that returns the result of applyPromptTemplate(getPrompt(PROMPT_IDS.GROUP_CHAT_PARTICIPANT_REQUEST), replacements). The helper should accept the unique values used in both places (participantName, updatedChat.name or group chat name, readOnlyNote, groupChatFolder, historyContext, readOnlyLabel, message, readOnlyInstruction) and construct the replacements object and call applyPromptTemplate/getPrompt internally; then replace the inline creation of participantPrompt in both the normal routing path (where participantPrompt is currently created) and the recovery path to call this new helper instead. Ensure both call sites use the same helper so future placeholder changes affect only one location.src/renderer/global.d.ts (1)
2111-2144: Model the new prompt IPC responses as discriminated unions.Line 2113-Line 2143 currently allows impossible states like
success: truewith no payload orsuccess: falsewith noerror. The handlers shown insrc/main/ipc/handlers/prompts.tsalready return success/data and failure/error as distinct cases, so tightening the declaration here will give renderer call sites proper narrowing. I’d also avoid re-declaring the prompt object inline, since the same six-field shape already exists insrc/main/prompt-manager.ts:30-37.♻️ Suggested type tightening
// Core Prompts API (Maestro system prompts) prompts: { - get: (id: string) => Promise<{ - success: boolean; - content?: string; - error?: string; - }>; - getAll: () => Promise<{ - success: boolean; - prompts?: Array<{ - id: string; - filename: string; - description: string; - category: string; - content: string; - isModified: boolean; - }>; - error?: string; - }>; - getAllIds: () => Promise<{ - success: boolean; - ids?: string[]; - error?: string; - }>; - save: (id: string, content: string) => Promise<{ - success: boolean; - error?: string; - }>; - reset: (id: string) => Promise<{ - success: boolean; - content?: string; - error?: string; - }>; + get: (id: string) => Promise< + | { success: true; content: string } + | { success: false; error: string } + >; + getAll: () => Promise< + | { + success: true; + prompts: Array<{ + id: string; + filename: string; + description: string; + category: string; + content: string; + isModified: boolean; + }>; + } + | { success: false; error: string } + >; + getAllIds: () => Promise< + | { success: true; ids: string[] } + | { success: false; error: string } + >; + save: (id: string, content: string) => Promise< + | { success: true } + | { success: false; error: string } + >; + reset: (id: string) => Promise< + | { success: true; content: string } + | { success: false; error: string } + >; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/global.d.ts` around lines 2111 - 2144, Tighten the prompts IPC types to discriminated unions so success cases always carry their payload and failure cases always carry an error: change get to Promise<{ success: true; content: string } | { success: false; error: string }>, getAll to Promise<{ success: true; prompts: PromptObject[] } | { success: false; error: string }>, getAllIds to Promise<{ success: true; ids: string[] } | { success: false; error: string }>, save to Promise<{ success: true } | { success: false; error: string }>, and reset to Promise<{ success: true; content: string } | { success: false; error: string }>; also stop redeclaring the six-field prompt shape inline and reference the existing shared prompt type (the six-field prompt object used by the prompt manager) instead of an inline type.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/renderer/hooks/input/useInputProcessing.ts`:
- Around line 25-47: loadInputProcessingPrompts currently must be awaited
elsewhere or getters (getImageOnlyPrompt/getMaestroSystemPrompt) throw, causing
race failures; add an ensureInputProcessingPromptsLoaded() helper that
returns/awaits a single in-flight Promise to dedupe concurrent calls (use a
module-level loadingPromise variable that is set when loadInputProcessingPrompts
is invoked and cleared on success/failure), update consumers (e.g., agentStore
async send/queue paths) to await ensureInputProcessingPromptsLoaded() before
calling getImageOnlyPrompt/getMaestroSystemPrompt, and keep the existing
cachedImageOnlyPrompt/cachedMaestroSystemPrompt and inputProcessingPromptsLoaded
flags untouched so getters read the cache after the helper completes.
---
Nitpick comments:
In `@src/main/group-chat/group-chat-router.ts`:
- Around line 845-857: Extract the duplicated replacement map into a single
helper (e.g., buildParticipantPrompt) that returns the result of
applyPromptTemplate(getPrompt(PROMPT_IDS.GROUP_CHAT_PARTICIPANT_REQUEST),
replacements). The helper should accept the unique values used in both places
(participantName, updatedChat.name or group chat name, readOnlyNote,
groupChatFolder, historyContext, readOnlyLabel, message, readOnlyInstruction)
and construct the replacements object and call applyPromptTemplate/getPrompt
internally; then replace the inline creation of participantPrompt in both the
normal routing path (where participantPrompt is currently created) and the
recovery path to call this new helper instead. Ensure both call sites use the
same helper so future placeholder changes affect only one location.
In `@src/renderer/global.d.ts`:
- Around line 2111-2144: Tighten the prompts IPC types to discriminated unions
so success cases always carry their payload and failure cases always carry an
error: change get to Promise<{ success: true; content: string } | { success:
false; error: string }>, getAll to Promise<{ success: true; prompts:
PromptObject[] } | { success: false; error: string }>, getAllIds to Promise<{
success: true; ids: string[] } | { success: false; error: string }>, save to
Promise<{ success: true } | { success: false; error: string }>, and reset to
Promise<{ success: true; content: string } | { success: false; error: string }>;
also stop redeclaring the six-field prompt shape inline and reference the
existing shared prompt type (the six-field prompt object used by the prompt
manager) instead of an inline type.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a1f34e77-8fb7-4c88-9610-b1157feec225
📒 Files selected for processing (7)
src/main/group-chat/group-chat-router.tssrc/renderer/global.d.tssrc/renderer/hooks/agent/useAgentListeners.tssrc/renderer/hooks/input/useInputProcessing.tssrc/renderer/services/contextGroomer.tssrc/renderer/stores/agentStore.tssrc/renderer/types/index.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/renderer/hooks/agent/useAgentListeners.ts
- src/renderer/types/index.ts
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)
package.json (1)
47-47:⚠️ Potential issue | 🔴 CriticalStale reference to removed
build:promptsscript.The
validate:pushscript still invokesnpm run build:prompts, but that script has been removed from this PR. Runningnpm run validate:pushwill fail with "missing script: build:prompts".🐛 Proposed fix
- "validate:push": "npm run build:prompts && npm run format:check:all && npm run lint && npm run lint:eslint && npm run test", + "validate:push": "npm run format:check:all && npm run lint && npm run lint:eslint && npm run test",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` at line 47, The package.json script "validate:push" references a removed script "build:prompts", causing failures; update "validate:push" (the npm script name) to remove the "npm run build:prompts" step or replace it with the correct existing script (e.g., build or another CI step) so the command sequence in "validate:push" only calls valid scripts; ensure you run npm run validate:push locally to verify no "missing script: build:prompts" error remains.
🤖 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/input/useInputProcessing.ts`:
- Around line 980-982: The code currently calls getMaestroSystemPrompt()
unconditionally and assigns it to currentMaestroSystemPrompt even for existing
sessions; change the logic so getMaestroSystemPrompt() is only invoked when
isNewSession (i.e., when !tabAgentSessionId) is true: move the
getMaestroSystemPrompt() call and currentMaestroSystemPrompt assignment inside
the if (isNewSession) block (or guard it with if (!tabAgentSessionId) before
calling), so follow-up sends for existing sessions do not depend on the
asynchronous prompt initialization.
---
Outside diff comments:
In `@package.json`:
- Line 47: The package.json script "validate:push" references a removed script
"build:prompts", causing failures; update "validate:push" (the npm script name)
to remove the "npm run build:prompts" step or replace it with the correct
existing script (e.g., build or another CI step) so the command sequence in
"validate:push" only calls valid scripts; ensure you run npm run validate:push
locally to verify no "missing script: build:prompts" error remains.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0079f7d0-3336-47a0-807f-5c3a5488ea21
📒 Files selected for processing (11)
CLAUDE.mdpackage.jsonsrc/__tests__/main/ipc/handlers/director-notes.test.tssrc/__tests__/renderer/hooks/useInputProcessing.test.tssrc/main/ipc/handlers/director-notes.tssrc/renderer/constants/shortcuts.tssrc/renderer/global.d.tssrc/renderer/hooks/input/useInputProcessing.tssrc/renderer/hooks/keyboard/useMainKeyboardHandler.tssrc/renderer/services/contextGroomer.tssrc/renderer/types/index.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- src/renderer/hooks/keyboard/useMainKeyboardHandler.ts
- src/tests/renderer/hooks/useInputProcessing.test.ts
- src/renderer/constants/shortcuts.ts
- src/tests/main/ipc/handlers/director-notes.test.ts
- src/renderer/types/index.ts
- src/main/ipc/handlers/director-notes.ts
Comment on lines
980
to
+982
| const isNewSession = !tabAgentSessionId; | ||
| if (isNewSession && maestroSystemPrompt) { | ||
| const currentMaestroSystemPrompt = getMaestroSystemPrompt(); | ||
| if (isNewSession && currentMaestroSystemPrompt) { |
There was a problem hiding this comment.
Only read the system prompt for brand-new sessions.
Line 981 makes every batch send depend on getMaestroSystemPrompt(), even when tabAgentSessionId already exists and the value will never be used. Because renderer prompt init is asynchronous, this can fail follow-up sends in existing sessions for no benefit.
Suggested fix
// For NEW sessions (no agentSessionId), prepend Maestro system prompt
// This introduces Maestro and sets directory restrictions for the agent
const isNewSession = !tabAgentSessionId;
- const currentMaestroSystemPrompt = getMaestroSystemPrompt();
- if (isNewSession && currentMaestroSystemPrompt) {
+ if (isNewSession) {
+ const currentMaestroSystemPrompt = getMaestroSystemPrompt();
+ if (!currentMaestroSystemPrompt) return;
// Get git branch for template substitution
let gitBranch: string | undefined;
if (freshSession.isGitRepo) {
try {
const status = await gitService.getStatus(freshSession.cwd);
@@
// Prepend system prompt to user's message
effectivePrompt = `${substitutedSystemPrompt}\n\n---\n\n# User Request\n\n${effectivePrompt}`;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/hooks/input/useInputProcessing.ts` around lines 980 - 982, The
code currently calls getMaestroSystemPrompt() unconditionally and assigns it to
currentMaestroSystemPrompt even for existing sessions; change the logic so
getMaestroSystemPrompt() is only invoked when isNewSession (i.e., when
!tabAgentSessionId) is true: move the getMaestroSystemPrompt() call and
currentMaestroSystemPrompt assignment inside the if (isNewSession) block (or
guard it with if (!tabAgentSessionId) before calling), so follow-up sends for
existing sessions do not depend on the asynchronous prompt initialization.
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
Resolves #197
generate-prompts.mjs) with runtime loading fromsrc/prompts/*.mdviaprompt-manager.ts. Prompts are bundled asextraResourcesfor all platforms.userData/core-prompts-customizations.json. User edits take priority over bundled defaults, with reset-to-default support.Cmd+Shift+2.build:promptsscript andsrc/generated/removed.Test plan
prompt-manager.tscovering load, customize, reset, and error pathsnpm run dev) and production buildsCmd+Shift+2🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Packaging
Documentation
Behavior
Tests