feat: wire feedback modal and IPC wiring - #491
Merged
jeffscottward merged 33 commits intoMar 26, 2026
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a Send Feedback feature: IPC handlers (gh auth check + submit), preload bridge, feedback prompt template, renderer UI (FeedbackView/Modal), About/QuickActions integration, modal state/hooks, and global typings. (50 words) Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Renderer as Feedback View
participant Preload as Preload (ipcRenderer)
participant Main as Main Process (IPC)
participant Agent as Agent Process
participant GHCli as GitHub CLI
participant GitHub as GitHub API
User->>Renderer: open feedback form
Renderer->>Preload: invoke checkGhAuth()
Preload->>Main: ipc invoke feedback:check-gh-auth
Main->>GHCli: run `gh auth status` (cache consulted)
GHCli-->>Main: auth status
Main-->>Preload: { authenticated, message? }
Preload-->>Renderer: auth result
alt authenticated
User->>Renderer: select agent + write feedback + submit
Renderer->>Preload: invoke feedback:submit {sessionId, feedbackText}
Preload->>Main: ipc invoke feedback:submit
Main->>Agent: send prompt (feedback injected) via ProcessManager
Agent->>GHCli: run `gh issue create ...`
GHCli->>GitHub: create issue
GitHub-->>GHCli: issue URL
Agent-->>Main: report output (issue URL / success)
Main-->>Preload: { success }
Preload-->>Renderer: { success }
Renderer->>User: switch to agent, close modal
else not authenticated
Renderer->>User: show auth message, disable form
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
src/main/preload/feedback.ts (1)
48-49: ProcessManager.write() safely writes to stdin without shell interpolation; remove unwarranted security concern.The handler correctly forwards raw
sessionId/feedbackTextacross the renderer→main privilege boundary. However, verification shows the concern about shell-interpolation is unfounded:ProcessManager.write()directly writes toprocess.ptyProcess.write(data)orprocess.childProcess.stdin.write(data), with no command execution or string interpolation. The feedbackText is substituted into a template viaString.replace()and written as stdin data—an inherently safe pattern.That said, the handler lacks explicit input validation (e.g., bounds checks on
feedbackText, verification thatsessionIdexists). Consider adding defensive validation for code robustness, though the current stdin-write design mitigates injection risk.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/preload/feedback.ts` around lines 48 - 49, The reviewer flagged a shell-interpolation risk that doesn't exist because ProcessManager.write writes directly to process.ptyProcess.write / process.childProcess.stdin.write; remove or update any comment/handler logic that asserts a shell-injection risk and instead add defensive input validation in the feedback submit handler (submit in src/main/preload/feedback.ts) and upstream where the handler resolves sessionId: enforce a reasonable max length for feedbackText, reject empty strings, and verify sessionId maps to an existing session before invoking the IPC; keep ProcessManager.write unchanged since it performs raw stdin writes (no interpolation).src/renderer/components/AppModals.tsx (1)
1802-1805: Consider unifying feedback modal open-state sourcing.At Line 2147 the component self-sources modal booleans from
useModalStore, butfeedbackModalOpenremains prop-driven (Line 2222). This mixed ownership can drift over time; prefer store-sourcing here too for consistency and lower wiring overhead.Also applies to: 2222-2225, 2477-2481
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/AppModals.tsx` around lines 1802 - 1805, The feedback modal open-state should be sourced from the same store as the other modals instead of a prop: in the AppModals component replace usage of the prop feedbackModalOpen and its local handlers with the modal boolean from useModalStore (the same selector used for other modals) and call the store's close action when closing the modal; update references to onCloseFeedbackModal (and any prop-driven state at the prop list) to use the store close function so all modal booleans (including feedback) are consistently read/written from useModalStore.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/ipc/handlers/feedback.ts`:
- Around line 114-128: The IPC feedback handler (the async callback that takes
sessionId and feedbackText and calls getProcessManager(),
fs.readFile(getPromptPath()), and processManager.write) must validate inputs at
the main-process boundary: ensure sessionId and feedbackText are strings,
non-empty, and within reasonable length limits (e.g., max length for
feedbackText), and reject or return { success: false, error: '...' } for invalid
inputs; additionally trim/sanitize feedbackText (strip dangerous control chars
or excessive newlines) before using promptTemplate.replace and before calling
processManager.write(sessionId, ...). Implement these checks at the top of that
handler (before getPromptPath()/fs.readFile/processManager.write) and return
clear error messages for each validation failure.
In `@src/renderer/components/AboutModal.tsx`:
- Around line 137-144: In AboutModal, the icon-only back and feedback buttons
(the button that calls setView('about') and the other icon button around
ArrowLeft/feedback icon) rely only on title; update both Button elements to
include explicit type="button" and an appropriate aria-label (e.g.,
aria-label="Back to About" for the setView('about') button and aria-label="Send
feedback" for the feedback button) so screen readers and default button behavior
are handled correctly; the relevant symbols to edit are the button elements that
render ArrowLeft and the feedback icon inside the AboutModal component.
In `@src/renderer/components/FeedbackView.tsx`:
- Around line 164-167: Update the FeedbackView component to associate labels
with their form controls and use “agent” in user-facing text: change the visible
label text from "Target Agent Session" to "Target Agent" and ensure the <label>
elements for the related <select> elements include htmlFor attributes that match
the corresponding select id (do the same for the other label at the second
occurrence). Keep the code-level types/names using the Session interface intact
(no renaming in code), only modify the displayed string and add the htmlFor/id
pairing so screen readers have a programmatic association.
- Around line 156-170: The form wrapper currently only applies 'opacity-40
pointer-events-none' when authState.authenticated is false, which blocks mouse
clicks but still allows keyboard focus; update the interactive controls (e.g.,
the <select> bound to selectedSessionId via setSelectedSessionId, the
inputs/textareas around lines referenced, and any submit controls using
isSubmittingDisabled) to also set the disabled prop when authState.authenticated
is false (or compute a single boolean like isAuthDisabled =
!authState.authenticated || isSubmittingDisabled and pass it to disabled on
select, inputs, textarea and buttons) so keyboard interaction is prevented
consistently across the form elements.
- Around line 113-116: The current branch that checks result.success overwrites
any backend-provided error with a fixed message; update the failure path in
FeedbackView (the block that checks result.success) to surface the backend error
instead: if result.success is false, call setSubmitError using the backend
message (e.g., result.error or result.message) and only fall back to the
existing "selected agent..." string when no backend message exists, then call
setSubmitting(false) and return; locate the logic referencing result.success,
setSubmitError, and setSubmitting and replace the hardcoded message with the
conditional use of the backend-provided error.
---
Nitpick comments:
In `@src/main/preload/feedback.ts`:
- Around line 48-49: The reviewer flagged a shell-interpolation risk that
doesn't exist because ProcessManager.write writes directly to
process.ptyProcess.write / process.childProcess.stdin.write; remove or update
any comment/handler logic that asserts a shell-injection risk and instead add
defensive input validation in the feedback submit handler (submit in
src/main/preload/feedback.ts) and upstream where the handler resolves sessionId:
enforce a reasonable max length for feedbackText, reject empty strings, and
verify sessionId maps to an existing session before invoking the IPC; keep
ProcessManager.write unchanged since it performs raw stdin writes (no
interpolation).
In `@src/renderer/components/AppModals.tsx`:
- Around line 1802-1805: The feedback modal open-state should be sourced from
the same store as the other modals instead of a prop: in the AppModals component
replace usage of the prop feedbackModalOpen and its local handlers with the
modal boolean from useModalStore (the same selector used for other modals) and
call the store's close action when closing the modal; update references to
onCloseFeedbackModal (and any prop-driven state at the prop list) to use the
store close function so all modal booleans (including feedback) are consistently
read/written from useModalStore.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
src/main/ipc/handlers/feedback.tssrc/main/ipc/handlers/index.tssrc/main/preload/feedback.tssrc/main/preload/index.tssrc/prompts/feedback.mdsrc/renderer/App.tsxsrc/renderer/components/AboutModal.tsxsrc/renderer/components/AppModals.tsxsrc/renderer/components/FeedbackModal.tsxsrc/renderer/components/FeedbackView.tsxsrc/renderer/components/QuickActionsModal.tsxsrc/renderer/constants/modalPriorities.tssrc/renderer/global.d.tssrc/renderer/hooks/modal/useModalHandlers.tssrc/renderer/stores/modalStore.ts
Greptile SummaryThis PR implements a feedback submission system that allows users to report bugs and suggest features directly from within Maestro. The feedback flow integrates with GitHub CLI to automatically create issues in the RunMaestro/Maestro repository. Key changes:
Implementation notes:
Issue found:
Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant FeedbackView
participant IPC
participant Main Process
participant ProcessManager
participant Agent
participant GitHub CLI
User->>FeedbackView: Opens feedback modal
FeedbackView->>IPC: feedback.checkGhAuth()
IPC->>Main Process: feedback:check-gh-auth
Main Process->>GitHub CLI: gh auth status
GitHub CLI-->>Main Process: exit code 0 (authenticated)
Main Process-->>IPC: {authenticated: true}
IPC-->>FeedbackView: Auth success
User->>FeedbackView: Enters feedback text
User->>FeedbackView: Selects target agent
User->>FeedbackView: Clicks "Send Feedback"
FeedbackView->>IPC: feedback.submit(sessionId, text)
IPC->>Main Process: feedback:submit
Main Process->>Main Process: Load feedback.md template
Main Process->>Main Process: Replace {{FEEDBACK}} placeholder
Main Process->>ProcessManager: write(sessionId, prompt)
ProcessManager->>Agent: Writes prompt to agent process
Agent->>Agent: Classifies feedback
Agent->>Agent: Formats GitHub issue
Agent->>GitHub CLI: gh label create "Maestro-feedback"
Agent->>GitHub CLI: gh issue create
GitHub CLI-->>Agent: Issue URL
Agent-->>User: Returns issue URL
Main Process-->>IPC: {success: true}
IPC-->>FeedbackView: Success
FeedbackView->>User: Switches to agent session
Last reviewed commit: 0c67650 |
![greptile-apps[bot]](https://avatars.github.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/renderer/components/AboutModal.tsx`:
- Around line 177-202: The three icon-only buttons in AboutModal (the Globe
button that renders <Globe>, the Discord button with the SVG, and the
Documentation button that renders <BookOpen>) are missing type="button" and
aria-label attributes; update each of those button elements to include
type="button" and a descriptive aria-label (e.g., "Visit runmaestro.ai", "Join
our Discord", "Documentation") to match the existing Feedback/Close buttons and
improve accessibility and consistency.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/ipc/handlers/feedback.tssrc/renderer/components/AboutModal.tsxsrc/renderer/components/FeedbackView.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- src/renderer/components/FeedbackView.tsx
- src/main/ipc/handlers/feedback.ts
jeffscottward
force-pushed
the
symphony/issue-457-mm82lf08
branch
from
March 1, 2026 21:03
36e922e to
4805547
Compare
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/renderer/components/AboutModal.tsx (1)
122-134:⚠️ Potential issue | 🟠 MajorFix Escape flow short-circuit so feedback view can always go back to About.
Line 124 currently returns early whenever
badgeEscapeHandlerRef.currentexists, even if the handler did not consume Escape. This can prevent the new feedback Escape behavior on Line 128 from running.🔧 Proposed fix
const handleEscape = useCallback(() => { // If badge overlay is open, close it first - if (badgeEscapeHandlerRef.current) { - badgeEscapeHandlerRef.current(); + if (badgeEscapeHandlerRef.current?.()) { return; } if (view === 'feedback') { setView('about'); return; } // Otherwise close the modal onCloseRef.current(); }, [view]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/AboutModal.tsx` around lines 122 - 134, The Escape flow currently always returns if badgeEscapeHandlerRef.current exists, preventing the feedback->about transition in handleEscape; change the call so you invoke badgeEscapeHandlerRef.current() and only return when that call indicates it consumed the event (e.g., returns true); otherwise fall through to the existing view check (if view === 'feedback' then setView('about')) and finally call onCloseRef.current(). Update handleEscape to use the boolean result of badgeEscapeHandlerRef.current() and, if relying on a new return contract, ensure other code that sets badgeEscapeHandlerRef provides a boolean return value.
♻️ Duplicate comments (1)
src/renderer/components/AboutModal.tsx (1)
177-202:⚠️ Potential issue | 🟡 MinorAdd explicit button semantics on icon-only external-link controls.
Line 177, Line 185, and Line 195 are icon-only
<button>elements missingtype="button"andaria-label, which hurts accessibility and consistency with nearby controls.♿ Proposed patch
<button + type="button" onClick={() => window.maestro.shell.openExternal('https://runmaestro.ai')} className="p-1 rounded hover:bg-white/10 transition-colors" title="Visit runmaestro.ai" + aria-label="Visit runmaestro.ai" style={{ color: theme.colors.accent }} > <Globe className="w-4 h-4" /> </button> <button + type="button" onClick={() => window.maestro.shell.openExternal('https://runmaestro.ai/discord')} className="p-1 rounded hover:bg-white/10 transition-colors" title="Join our Discord" + aria-label="Join our Discord" style={{ color: theme.colors.accent }} > @@ <button + type="button" onClick={() => window.maestro.shell.openExternal('https://docs.runmaestro.ai/')} className="p-1 rounded hover:bg-white/10 transition-colors" title="Documentation" + aria-label="Documentation" style={{ color: theme.colors.accent }} > <BookOpen className="w-4 h-4" /> </button>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/AboutModal.tsx` around lines 177 - 202, The three icon-only buttons in AboutModal that call window.maestro.shell.openExternal('https://runmaestro.ai'), window.maestro.shell.openExternal('https://runmaestro.ai/discord'), and window.maestro.shell.openExternal('https://docs.runmaestro.ai/') should include explicit button semantics: add type="button" and a descriptive aria-label for each (e.g., "Visit runmaestro.ai", "Join our Discord", "Open documentation") to the button elements that render the <Globe />, the Discord SVG, and <BookOpen /> so they are accessible and consistent with other controls.
🧹 Nitpick comments (1)
src/main/preload/feedback.ts (1)
44-50: TypecreateFeedbackApireturn asFeedbackApito prevent API drift.This keeps the preload bridge contract enforced at compile time if methods are changed later.
♻️ Proposed patch
-export function createFeedbackApi() { +export function createFeedbackApi(): FeedbackApi { return { checkGhAuth: (): Promise<FeedbackAuthResponse> => ipcRenderer.invoke('feedback:check-gh-auth'), submit: (sessionId: string, feedbackText: string): Promise<FeedbackSubmitResponse> => ipcRenderer.invoke('feedback:submit', { sessionId, feedbackText }), }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/preload/feedback.ts` around lines 44 - 50, The createFeedbackApi function currently returns an inferred object type which can drift; explicitly annotate its return type as FeedbackApi by changing the signature to export function createFeedbackApi(): FeedbackApi { … } and ensure the returned object implements the FeedbackApi interface (i.e., includes checkGhAuth and submit with the correct signatures) so the preload bridge contract is enforced at compile time.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/renderer/components/AboutModal.tsx`:
- Around line 122-134: The Escape flow currently always returns if
badgeEscapeHandlerRef.current exists, preventing the feedback->about transition
in handleEscape; change the call so you invoke badgeEscapeHandlerRef.current()
and only return when that call indicates it consumed the event (e.g., returns
true); otherwise fall through to the existing view check (if view === 'feedback'
then setView('about')) and finally call onCloseRef.current(). Update
handleEscape to use the boolean result of badgeEscapeHandlerRef.current() and,
if relying on a new return contract, ensure other code that sets
badgeEscapeHandlerRef provides a boolean return value.
---
Duplicate comments:
In `@src/renderer/components/AboutModal.tsx`:
- Around line 177-202: The three icon-only buttons in AboutModal that call
window.maestro.shell.openExternal('https://runmaestro.ai'),
window.maestro.shell.openExternal('https://runmaestro.ai/discord'), and
window.maestro.shell.openExternal('https://docs.runmaestro.ai/') should include
explicit button semantics: add type="button" and a descriptive aria-label for
each (e.g., "Visit runmaestro.ai", "Join our Discord", "Open documentation") to
the button elements that render the <Globe />, the Discord SVG, and <BookOpen />
so they are accessible and consistent with other controls.
---
Nitpick comments:
In `@src/main/preload/feedback.ts`:
- Around line 44-50: The createFeedbackApi function currently returns an
inferred object type which can drift; explicitly annotate its return type as
FeedbackApi by changing the signature to export function createFeedbackApi():
FeedbackApi { … } and ensure the returned object implements the FeedbackApi
interface (i.e., includes checkGhAuth and submit with the correct signatures) so
the preload bridge contract is enforced at compile time.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
src/main/ipc/handlers/feedback.tssrc/main/ipc/handlers/index.tssrc/main/preload/feedback.tssrc/main/preload/index.tssrc/prompts/feedback.mdsrc/renderer/App.tsxsrc/renderer/components/AboutModal.tsxsrc/renderer/components/AppModals.tsxsrc/renderer/components/FeedbackModal.tsxsrc/renderer/components/FeedbackView.tsxsrc/renderer/components/QuickActionsModal.tsxsrc/renderer/constants/modalPriorities.tssrc/renderer/global.d.tssrc/renderer/hooks/modal/useModalHandlers.tssrc/renderer/stores/modalStore.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- src/renderer/components/FeedbackModal.tsx
- src/renderer/components/QuickActionsModal.tsx
- src/renderer/App.tsx
- src/renderer/stores/modalStore.ts
- src/renderer/constants/modalPriorities.ts
- src/prompts/feedback.md
- src/renderer/components/FeedbackView.tsx
- src/main/preload/index.ts
jeffscottward
pushed a commit
to jeffscottward/Maestro
that referenced
this pull request
Mar 3, 2026
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@prompt.XXXXXX.txt`:
- Around line 1-17: Remove the accidental agent prompt/debug artifact file
(prompt.XXXXXX.txt) from the PR: delete the file or revert its addition so only
product code changes remain, ensuring no references to the file remain in the
branch or imports; commit the deletion under the same PR branch to clear the
unrelated CI/debug payload content that caused the failing checks.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
payload.XXXXXX.jsonprompt.XXXXXX.txtsrc/__tests__/renderer/components/AboutModal.test.tsx
✅ Files skipped from review due to trivial changes (1)
- payload.XXXXXX.json
jeffscottward
pushed a commit
to jeffscottward/Maestro
that referenced
this pull request
Mar 3, 2026
jeffscottward
pushed a commit
to jeffscottward/Maestro
that referenced
this pull request
Mar 3, 2026
- Add input validation at IPC boundary (sessionId, feedbackText length/empty checks) - Add type="button" and aria-label to AboutModal icon-only buttons - Surface backend error messages instead of hardcoded string in FeedbackView - Add isFormDisabled to properly disable form controls when auth is missing - Associate labels with form controls via htmlFor/id pairings - Change "Target Agent Session" to "Target Agent" in user-facing text - Include idle state in isRunningSession so ready agents can receive feedback Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
jeffscottward
force-pushed
the
symphony/issue-457-mm82lf08
branch
from
March 13, 2026 23:42
b0b90b3 to
070ebf1
Compare
Fix collapsed group spacing for empty groups
Contributor
Author
|
Successful end-to-end Maestro feedback flow with text submission and inline screenshot rendering. Valid issue from the UI flow:
|
Contributor
Author
|
Copy pasting the report that my local AI Building this feature sent to me in email Completed
Validation
Links
Not completed / blockers
|
# Conflicts: # docs/releases.md # src/__tests__/renderer/components/SessionList.test.tsx # src/renderer/components/AboutModal.tsx # src/renderer/components/AppModals.tsx # src/renderer/components/SessionList/SessionList.tsx
This was referenced Apr 12, 2026
7 tasks
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.
Closes #457
Closes #575
Implements feedback modal and IPC wiring updates (renderer modal flow, IPC wiring, and related preload/main handlers).
Summary by CodeRabbit
New Features
Documentation
Tests