feat: wire feedback modal and IPC wiring#491
Open
jeffscottward wants to merge 7 commits intoRunMaestro:mainfrom
Open
feat: wire feedback modal and IPC wiring#491jeffscottward wants to merge 7 commits intoRunMaestro:mainfrom
jeffscottward wants to merge 7 commits intoRunMaestro:mainfrom
Conversation
📝 WalkthroughWalkthroughAdds a Send Feedback feature: renderer UI (FeedbackView/Modal, About/QuickActions integration), preload bridge, main-process IPC handlers to check GH CLI auth and submit feedback via an agent using a feedback prompt template, plus modal state/hooks and global typings. 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 form
Renderer->>Preload: invoke checkGhAuth
Preload->>Main: ipc invoke feedback:check-gh-auth
Main->>GHCli: gh auth status (cached)
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 injected prompt to process via ProcessManager
Agent->>GHCli: run gh issue create (via agent steps)
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 | ✅ 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)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 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.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
21
to
28
| function isRunningSession(session: Session): boolean { | ||
| if (session.toolType === 'terminal') { | ||
| return false; | ||
| } | ||
|
|
||
| return ( | ||
| session.state === 'busy' || session.state === 'waiting_input' || session.state === 'connecting' | ||
| ); |
There was a problem hiding this comment.
isRunningSession excludes 'idle' sessions, but idle agents are running and can accept input. This means users can't send feedback to ready agents that aren't actively processing.
Suggested change
| function isRunningSession(session: Session): boolean { | |
| if (session.toolType === 'terminal') { | |
| return false; | |
| } | |
| return ( | |
| session.state === 'busy' || session.state === 'waiting_input' || session.state === 'connecting' | |
| ); | |
| function isRunningSession(session: Session): boolean { | |
| if (session.toolType === 'terminal') { | |
| return false; | |
| } | |
| return ( | |
| session.state === 'idle' || | |
| session.state === 'busy' || | |
| session.state === 'waiting_input' || | |
| session.state === 'connecting' | |
| ); | |
| } |
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
Comment on lines
+177
to
+202
| <button | ||
| onClick={() => window.maestro.shell.openExternal('https://runmaestro.ai')} | ||
| className="p-1 rounded hover:bg-white/10 transition-colors" | ||
| title="Visit runmaestro.ai" | ||
| style={{ color: theme.colors.accent }} | ||
| > | ||
| <Globe className="w-4 h-4" /> | ||
| </button> | ||
| <button | ||
| onClick={() => window.maestro.shell.openExternal('https://runmaestro.ai/discord')} | ||
| className="p-1 rounded hover:bg-white/10 transition-colors" | ||
| title="Join our Discord" | ||
| style={{ color: theme.colors.accent }} | ||
| > | ||
| <svg className="w-4 h-4" viewBox="0 0 24 24" fill="currentColor"> | ||
| <path d="M20.317 4.37a19.791 19.791 0 0 0-4.885-1.515.074.074 0 0 0-.079.037c-.21.375-.444.864-.608 1.25a18.27 18.27 0 0 0-5.487 0 12.64 12.64 0 0 0-.617-1.25.077.077 0 0 0-.079-.037A19.736 19.736 0 0 0 3.677 4.37a.07.07 0 0 0-.032.027C.533 9.046-.32 13.58.099 18.057a.082.082 0 0 0 .031.057 19.9 19.9 0 0 0 5.993 3.03.078.078 0 0 0 .084-.028 14.09 14.09 0 0 0 1.226-1.994.076.076 0 0 0-.041-.106 13.107 13.107 0 0 1-1.872-.892.077.077 0 0 1-.008-.128 10.2 10.2 0 0 0 .372-.292.074.074 0 0 1 .077-.01c3.928 1.793 8.18 1.793 12.062 0a.074.074 0 0 1 .078.01c.12.098.246.198.373.292a.077.077 0 0 1-.006.127 12.299 12.299 0 0 1-1.873.892.077.077 0 0 0-.041.107c.36.698.772 1.362 1.225 1.993a.076.076 0 0 0 .084.028 19.839 19.839 0 0 0 6.002-3.03.077.077 0 0 0 .032-.054c.5-5.177-.838-9.674-3.549-13.66a.061.061 0 0 0-.031-.03zM8.02 15.33c-1.183 0-2.157-1.085-2.157-2.419 0-1.333.956-2.419 2.157-2.419 1.21 0 2.176 1.096 2.157 2.42 0 1.333-.956 2.418-2.157 2.418zm7.975 0c-1.183 0-2.157-1.085-2.157-2.419 0-1.333.955-2.419 2.157-2.419 1.21 0 2.176 1.096 2.157 2.42 0 1.333-.946 2.418-2.157 2.418z" /> | ||
| </svg> | ||
| </button> | ||
| <button | ||
| onClick={() => window.maestro.shell.openExternal('https://docs.runmaestro.ai/')} | ||
| className="p-1 rounded hover:bg-white/10 transition-colors" | ||
| title="Documentation" | ||
| style={{ color: theme.colors.accent }} | ||
| > | ||
| <BookOpen className="w-4 h-4" /> | ||
| </button> |
There was a problem hiding this comment.
Add type="button" and aria-label to remaining icon-only buttons.
The Globe, Discord, and Documentation buttons are missing type="button" and aria-label attributes, while the adjacent Feedback and Close buttons have them. For consistency and accessibility, apply the same treatment.
🛡️ Proposed fix
<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 }}
>
<svg className="w-4 h-4" viewBox="0 0 24 24" fill="currentColor">
...
</svg>
</button>
<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>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <button | |
| onClick={() => window.maestro.shell.openExternal('https://runmaestro.ai')} | |
| className="p-1 rounded hover:bg-white/10 transition-colors" | |
| title="Visit runmaestro.ai" | |
| style={{ color: theme.colors.accent }} | |
| > | |
| <Globe className="w-4 h-4" /> | |
| </button> | |
| <button | |
| onClick={() => window.maestro.shell.openExternal('https://runmaestro.ai/discord')} | |
| className="p-1 rounded hover:bg-white/10 transition-colors" | |
| title="Join our Discord" | |
| style={{ color: theme.colors.accent }} | |
| > | |
| <svg className="w-4 h-4" viewBox="0 0 24 24" fill="currentColor"> | |
| <path d="M20.317 4.37a19.791 19.791 0 0 0-4.885-1.515.074.074 0 0 0-.079.037c-.21.375-.444.864-.608 1.25a18.27 18.27 0 0 0-5.487 0 12.64 12.64 0 0 0-.617-1.25.077.077 0 0 0-.079-.037A19.736 19.736 0 0 0 3.677 4.37a.07.07 0 0 0-.032.027C.533 9.046-.32 13.58.099 18.057a.082.082 0 0 0 .031.057 19.9 19.9 0 0 0 5.993 3.03.078.078 0 0 0 .084-.028 14.09 14.09 0 0 0 1.226-1.994.076.076 0 0 0-.041-.106 13.107 13.107 0 0 1-1.872-.892.077.077 0 0 1-.008-.128 10.2 10.2 0 0 0 .372-.292.074.074 0 0 1 .077-.01c3.928 1.793 8.18 1.793 12.062 0a.074.074 0 0 1 .078.01c.12.098.246.198.373.292a.077.077 0 0 1-.006.127 12.299 12.299 0 0 1-1.873.892.077.077 0 0 0-.041.107c.36.698.772 1.362 1.225 1.993a.076.076 0 0 0 .084.028 19.839 19.839 0 0 0 6.002-3.03.077.077 0 0 0 .032-.054c.5-5.177-.838-9.674-3.549-13.66a.061.061 0 0 0-.031-.03zM8.02 15.33c-1.183 0-2.157-1.085-2.157-2.419 0-1.333.956-2.419 2.157-2.419 1.21 0 2.176 1.096 2.157 2.42 0 1.333-.956 2.418-2.157 2.418zm7.975 0c-1.183 0-2.157-1.085-2.157-2.419 0-1.333.955-2.419 2.157-2.419 1.21 0 2.176 1.096 2.157 2.42 0 1.333-.946 2.418-2.157 2.418z" /> | |
| </svg> | |
| </button> | |
| <button | |
| onClick={() => window.maestro.shell.openExternal('https://docs.runmaestro.ai/')} | |
| className="p-1 rounded hover:bg-white/10 transition-colors" | |
| title="Documentation" | |
| style={{ color: theme.colors.accent }} | |
| > | |
| <BookOpen className="w-4 h-4" /> | |
| </button> | |
| <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 }} | |
| > | |
| <svg className="w-4 h-4" viewBox="0 0 24 24" fill="currentColor"> | |
| <path d="M20.317 4.37a19.791 19.791 0 0 0-4.885-1.515.074.074 0 0 0-.079.037c-.21.375-.444.864-.608 1.25a18.27 18.27 0 0 0-5.487 0 12.64 12.64 0 0 0-.617-1.25.077.077 0 0 0-.079-.037A19.736 19.736 0 0 0 3.677 4.37a.07.07 0 0 0-.032.027C.533 9.046-.32 13.58.099 18.057a.082.082 0 0 0 .031.057 19.9 19.9 0 0 0 5.993 3.03.078.078 0 0 0 .084-.028 14.09 14.09 0 0 0 1.226-1.994.076.076 0 0 0-.041-.106 13.107 13.107 0 0 1-1.872-.892.077.077 0 0 1-.008-.128 10.2 10.2 0 0 0 .372-.292.074.074 0 0 1 .077-.01c3.928 1.793 8.18 1.793 12.062 0a.074.074 0 0 1 .078.01c.12.098.246.198.373.292a.077.077 0 0 1-.006.127 12.299 12.299 0 0 1-1.873.892.077.077 0 0 0-.041.107c.36.698.772 1.362 1.225 1.993a.076.076 0 0 0 .084.028 19.839 19.839 0 0 0 6.002-3.03.077.077 0 0 0 .032-.054c.5-5.177-.838-9.674-3.549-13.66a.061.061 0 0 0-.031-.03zM8.02 15.33c-1.183 0-2.157-1.085-2.157-2.419 0-1.333.956-2.419 2.157-2.419 1.21 0 2.176 1.096 2.157 2.42 0 1.333-.956 2.418-2.157 2.418zm7.975 0c-1.183 0-2.157-1.085-2.157-2.419 0-1.333.955-2.419 2.157-2.419 1.21 0 2.176 1.096 2.157 2.42 0 1.333-.946 2.418-2.157 2.418z" /> | |
| </svg> | |
| </button> | |
| <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 (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.
- 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
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
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
Implements feedback modal and IPC wiring updates (renderer modal flow, IPC wiring, and related preload/main handlers).
Summary by CodeRabbit