feat(feedback): persistent, resumable Send Feedback drafts (#756)#1133
feat(feedback): persistent, resumable Send Feedback drafts (#756)#1133jSydorowicz21 wants to merge 4 commits into
Conversation
Feedback drafts now persist to a userData JSON file (mirroring the playbooks store) so they survive modal/app close. Adds drafts list/save/delete IPC + preload + store, a FeedbackDraftsList overlay with metadata and attachment indicators, resume wiring, and a save-or-discard close dialog. Covered by IPC, preload, store, and view tests.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughFeedback drafts are now persisted in the main process, exposed through preload and renderer APIs, and surfaced in the feedback modal and chat view for save, resume, list, and delete actions. ChangesFeedback drafts
Sequence Diagram(s)sequenceDiagram
participant useModalHandlers
participant FeedbackModal
participant useFeedbackDraftStore
participant DraftsApi as "window.maestro.feedback.drafts"
participant FeedbackHandlers as "src/main/ipc/handlers/feedback.ts"
participant FeedbackChatView
participant FeedbackDraftsList
useModalHandlers->>FeedbackModal: open feedback modal
FeedbackModal->>useFeedbackDraftStore: loadDrafts()
useFeedbackDraftStore->>DraftsApi: list()
DraftsApi->>FeedbackHandlers: feedback:drafts:list
FeedbackHandlers-->>DraftsApi: drafts[]
DraftsApi-->>useFeedbackDraftStore: drafts[]
FeedbackModal->>FeedbackDraftsList: render saved drafts
FeedbackDraftsList-->>FeedbackModal: onResume(draft.id) / onDelete(draft.id)
FeedbackModal->>useFeedbackDraftStore: requestResume(draft.id)
FeedbackModal->>FeedbackChatView: resumeDraftId
FeedbackChatView->>useFeedbackDraftStore: saveDraft()
useFeedbackDraftStore->>DraftsApi: save(draft)
DraftsApi->>FeedbackHandlers: feedback:drafts:save
FeedbackHandlers-->>DraftsApi: draft
DraftsApi-->>useFeedbackDraftStore: draft
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 adds persistent feedback drafts to the Send Feedback flow. The main changes are:
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (3): Last reviewed commit: "fix(feedback): dedupe concurrent first-s..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e647e3ada0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| category: lastResponse?.category ?? 'general_feedback', | ||
| summary: lastResponse?.summary ?? '', | ||
| confidence, | ||
| agentType: selectedAgent, |
There was a problem hiding this comment.
When a user saves after the assistant has reached the submit-ready state, this draft only keeps the category, summary, and confidence while dropping lastResponse.structured and the ready flag that createNewIssue() requires. On resume, confidence can show 80% or higher, but isReady is false and lastResponse is null, so the Submit Feedback button stays hidden until the user sends another message. Persist and hydrate the parsed response, or otherwise reconstruct it, so a ready draft remains submittable after resume.
Useful? React with 👍 / 👎.
| // Auto-select first available | ||
| const firstAvailable = AGENT_TILES.find((t) => t.supported && available.has(t.id)); | ||
| if (firstAvailable) setSelectedAgent(firstAvailable.id); | ||
| if (firstAvailable && !resumeDraft) setSelectedAgent(firstAvailable.id); |
There was a problem hiding this comment.
When resuming a draft created with a provider that is no longer available, this condition keeps selectedAgent set to the saved provider as long as any supported provider exists. The auto-start gate only checks availableTiles.length, and sendMessage() later throws if the selected agent is unavailable, with no UI left for the user to switch providers. Fall back to firstAvailable when the saved agentType is not in the detected set.
Useful? React with 👍 / 👎.
| try { | ||
| const { draft: saved } = await window.maestro.feedback.drafts.save(draft); | ||
| await get().loadDrafts(); | ||
| set({ activeDraftId: saved.id }); |
There was a problem hiding this comment.
When a new draft is saved, the IPC handler returns the generated id, but the store only updates activeDraftId and leaves activeDraft with its old empty id. The close and minimize paths later call saveDraft(activeDraft), so if the user clicks Save draft and then Save & Close or minimize without editing, the stale empty id is sent and the main handler creates a duplicate draft instead of updating the saved one. Patch the active snapshot with saved.id when saving succeeds.
Useful? React with 👍 / 👎.
| } catch { | ||
| return null; |
There was a problem hiding this comment.
If the IPC save fails because disk persistence throws or the bridge rejects, this catch converts the failure to null; callers do not check for null, so the Save draft button still shows Saved and Save & Close still closes the modal, leaving users thinking their feedback is persisted when it was not. Let the error propagate or return a failure state that callers must handle before showing success or closing.
Useful? React with 👍 / 👎.
| const handleResume = useCallback((id: string) => { | ||
| setShowDrafts(false); | ||
| useFeedbackDraftStore.getState().requestResume(id); | ||
| setResumeNonce((n) => n + 1); |
There was a problem hiding this comment.
When the editor already contains unsaved feedback, clicking a saved draft immediately requests the resume and bumps the key, which unmounts the current FeedbackChatView and runs its reset cleanup without saving or asking about the live activeDraft. A user who opens the Drafts list while composing a new unsaved report loses that in-progress text as soon as they resume another draft, so this path should save or confirm before switching drafts.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/main/ipc/handlers/feedback.ts (2)
208-216: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value
listreturns on-disk drafts without normalization.
readDrafts()castsdata.draftsstraight toFeedbackDraft[], sofeedback:drafts:listhands the renderer whatever is on disk, whereassavealways normalizes. If the file is ever from an older schema or hand-edited, malformed drafts reach the UI untyped. Mapping read results throughnormalizeDraftkeeps the read/write contract symmetric.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/ipc/handlers/feedback.ts` around lines 208 - 216, readDrafts() is returning raw on-disk draft objects instead of normalized FeedbackDrafts, so feedback:drafts:list can surface malformed legacy or hand-edited data. Update readDrafts() in src/main/ipc/handlers/feedback.ts to map any parsed data.drafts entries through normalizeDraft before returning them, keeping the read path symmetric with save() and ensuring the renderer only receives normalized drafts.
13-13: 📐 Maintainability & Code Quality | 🔵 TrivialUse the shared UUID helper instead of
crypto.randomUUIDPer coding guidelines, ID generation should use
generateUUID()fromsrc/shared/uuid.tsto avoid duplicate implementations.♻️ Suggested change
- import { randomUUID } from 'crypto'; + import { generateUUID } from '../../../shared/uuid';- id: typeof d.id === 'string' && d.id ? d.id : randomUUID(), + id: typeof d.id === 'string' && d.id ? d.id : generateUUID(),This applies to line 13 and line 295 where
randomUUID()is used.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/ipc/handlers/feedback.ts` at line 13, Replace the direct UUID generation in feedback.ts with the shared generateUUID() helper from src/shared/uuid.ts. Update the import at the top of the file and change both places that currently call randomUUID() so they use generateUUID() instead, keeping the existing logic in the feedback handler functions unchanged.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/ipc/handlers/feedback.ts`:
- Around line 1216-1254: The draft persistence handlers in feedback:drafts:save
and feedback:drafts:delete are doing an unguarded read-modify-write cycle that
can interleave and overwrite concurrent changes. Add a shared in-memory
serialization mechanism around the draft operations in
src/main/ipc/handlers/feedback.ts, such as a single promise chain or mutex used
by both ipcMain.handle callbacks. Ensure readDrafts(), the mutation logic, and
writeDrafts() run atomically per request so overlapping saves/deletes cannot
clobber each other.
In `@src/renderer/components/FeedbackChatView.tsx`:
- Around line 594-600: The draft snapshot is using a stale or missing draft id
because `serializeDraft` and the `useEffect` that calls
`useFeedbackDraftStore.getState().setActiveDraft(...)` do not depend on
`activeDraftId`. Update both dependency lists so `FeedbackChatView` re-runs the
serialization whenever `activeDraftId` changes, ensuring the snapshot always
carries the current identifier. This should be fixed in the `serializeDraft`
logic and the surrounding snapshot effect that reads `messages`, `attachments`,
and `inputValue`.
---
Nitpick comments:
In `@src/main/ipc/handlers/feedback.ts`:
- Around line 208-216: readDrafts() is returning raw on-disk draft objects
instead of normalized FeedbackDrafts, so feedback:drafts:list can surface
malformed legacy or hand-edited data. Update readDrafts() in
src/main/ipc/handlers/feedback.ts to map any parsed data.drafts entries through
normalizeDraft before returning them, keeping the read path symmetric with
save() and ensuring the renderer only receives normalized drafts.
- Line 13: Replace the direct UUID generation in feedback.ts with the shared
generateUUID() helper from src/shared/uuid.ts. Update the import at the top of
the file and change both places that currently call randomUUID() so they use
generateUUID() instead, keeping the existing logic in the feedback handler
functions unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 39c8f3f5-b7d4-4b71-bead-cdb746b6c8e4
📒 Files selected for processing (13)
src/__tests__/main/ipc/handlers/feedback.test.tssrc/__tests__/main/preload/feedback.test.tssrc/__tests__/renderer/components/FeedbackChatView.test.tsxsrc/__tests__/renderer/stores/feedbackDraftStore.test.tssrc/__tests__/setup.tssrc/main/ipc/handlers/feedback.tssrc/main/preload/feedback.tssrc/renderer/components/FeedbackChatView.tsxsrc/renderer/components/FeedbackDraftsList.tsxsrc/renderer/components/FeedbackModal.tsxsrc/renderer/global.d.tssrc/renderer/hooks/modal/useModalHandlers.tssrc/renderer/stores/feedbackDraftStore.ts
Addresses all review findings: discard no longer deletes a resumed draft; failed saves keep the modal open and surface a save-error banner instead of silently closing/minimizing; the minted draft id is propagated back to the live snapshot so save+minimize no longer duplicates; concurrent draft writes are serialized via the shared atomic-json-store keyed write queue (mirrors group-chat-storage); resumed drafts restore the submit-ready response and fall back to an available provider when the saved one is gone; switching to another draft auto-saves the current one first.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 43ff3e7f3c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Preserve any unsaved in-progress editor before we unmount it to resume a | ||
| // different draft, so switching drafts never silently discards work. | ||
| const { activeDraft, activeDraftId, saveDraft } = useFeedbackDraftStore.getState(); | ||
| if (activeDraft && activeDraftId !== id) { |
There was a problem hiding this comment.
When the editor is already showing draft A and the user has typed unsaved changes, opening Saved Drafts and clicking draft A again skips saveDraft because activeDraftId === id, then setResumeNonce remounts FeedbackChatView from the persisted copy. That drops the unsaved edits to the same draft. Treat this case as a no-op or save before remounting.
Useful? React with 👍 / 👎.
| inputDraft: | ||
| typeof d.inputDraft === 'string' | ||
| ? sanitizeTextInput(d.inputDraft).slice(0, MAX_FEEDBACK_FIELD_LENGTH) |
There was a problem hiding this comment.
When a user writes a long report in the composer and saves or minimizes before sending it, inputDraft is silently capped at MAX_FEEDBACK_FIELD_LENGTH (5000 chars), while the chat input itself has no matching limit and sent messages are preserved up to 20000 chars. On resume, everything after 5000 chars is gone with no warning. Use the draft message limit here or enforce the same cap in the textarea before saving.
Useful? React with 👍 / 👎.
| const messages = Array.isArray(d.messages) | ||
| ? (d.messages | ||
| .map(normalizeDraftMessage) | ||
| .filter((m): m is FeedbackDraftMessage => m !== null) | ||
| .slice(0, MAX_DRAFT_MESSAGES) as FeedbackDraftMessage[]) |
There was a problem hiding this comment.
When a feedback conversation grows past 200 messages, this cap keeps the oldest messages and drops the newest ones. Saving or minimizing at that point means the resumed draft loses the latest user details and assistant questions, which are the most relevant context for continuing or submitting the report. Trim from the front instead, for example by keeping the last MAX_DRAFT_MESSAGES entries.
Useful? React with 👍 / 👎.
Round-2 review fixes: trim drafts from the front so the NEWEST messages survive the 200-message cap (was keeping oldest); raise the persisted composer inputDraft cap from 5000 to the 20000 draft-message limit so long unsent reports are not silently truncated on resume; reopening the already-active draft is now a no-op (no remount) so unsaved edits are not dropped. +4 tests.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/__tests__/renderer/components/FeedbackModal.test.tsx`:
- Around line 14-37: The mocked feedback draft store is not propagating resume
state, so `FeedbackModal` tests can false-pass when `requestResume` is only a
spy. Update the `useFeedbackDraftStore` mock in `FeedbackModal.test.tsx` so
`requestResume` mutates `mockState.resumeDraftId` (and any related active/resume
fields needed by `FeedbackModal` and `FeedbackChatView`), then verify the
remounted chat receives the resumed draft id. Apply the same mock/state behavior
consistently across the affected test cases so the resume flow is actually
exercised.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1ea0bd7b-2a05-423f-813e-8186207a568f
📒 Files selected for processing (10)
src/__tests__/main/ipc/handlers/feedback.test.tssrc/__tests__/renderer/components/FeedbackChatView.test.tsxsrc/__tests__/renderer/components/FeedbackModal.test.tsxsrc/__tests__/renderer/stores/feedbackDraftStore.test.tssrc/main/ipc/handlers/feedback.tssrc/main/preload/feedback.tssrc/renderer/components/FeedbackChatView.tsxsrc/renderer/components/FeedbackModal.tsxsrc/renderer/global.d.tssrc/renderer/stores/feedbackDraftStore.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- src/renderer/global.d.ts
- src/main/preload/feedback.ts
- src/main/ipc/handlers/feedback.ts
- src/renderer/components/FeedbackChatView.tsx
- src/renderer/components/FeedbackModal.tsx
- src/renderer/stores/feedbackDraftStore.ts
| const mockState = { | ||
| isMinimized: false, | ||
| hasDraft: false, | ||
| drafts: [] as FeedbackDraft[], | ||
| resumeDraftId: null as string | null, | ||
| activeDraftId: null as string | null, | ||
| activeDraft: null as FeedbackDraft | null, | ||
| setMinimized: vi.fn(), | ||
| setHasDraft: vi.fn(), | ||
| setActiveDraft: vi.fn(), | ||
| loadDrafts: vi.fn().mockResolvedValue(undefined), | ||
| saveDraft: vi.fn().mockResolvedValue('saved-id'), | ||
| deleteDraft: vi.fn().mockResolvedValue(undefined), | ||
| requestResume: vi.fn(), | ||
| clearResume: vi.fn(), | ||
| reset: vi.fn(), | ||
| }; | ||
|
|
||
| vi.mock('../../../renderer/stores/feedbackDraftStore', () => ({ | ||
| useFeedbackDraftStore: Object.assign( | ||
| (selector: (s: typeof mockState) => unknown) => selector(mockState), | ||
| { getState: () => mockState } | ||
| ), | ||
| })); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Make the mocked draft store propagate resumeDraftId.
requestResume is only a spy here, so clicking resume-draft-2 never changes mockState.resumeDraftId. Because FeedbackModal also bumps resumeNonce, this test can still observe a remount even if the resumed draft id is never forwarded to FeedbackChatView. Update the mock to mutate resumeDraftId and assert the remounted chat receives that id, otherwise this scenario can false-pass while the actual resume flow is broken.
Suggested test mock fix
const mockState = {
...
- requestResume: vi.fn(),
+ requestResume: vi.fn((id: string) => {
+ mockState.resumeDraftId = id;
+ }),
...
};
vi.mock('../../../renderer/components/FeedbackChatView', () => ({
- FeedbackChatView: () => {
+ FeedbackChatView: ({ resumeDraftId }: { resumeDraftId?: string | null }) => {
React.useEffect(() => {
mockChat.mountCount += 1;
}, []);
- return <div data-testid="chat-view" />;
+ return <div data-testid="chat-view" data-resume-draft-id={resumeDraftId ?? ''} />;
},
}));Also applies to: 77-84, 179-198
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/__tests__/renderer/components/FeedbackModal.test.tsx` around lines 14 -
37, The mocked feedback draft store is not propagating resume state, so
`FeedbackModal` tests can false-pass when `requestResume` is only a spy. Update
the `useFeedbackDraftStore` mock in `FeedbackModal.test.tsx` so `requestResume`
mutates `mockState.resumeDraftId` (and any related active/resume fields needed
by `FeedbackModal` and `FeedbackChatView`), then verify the remounted chat
receives the resumed draft id. Apply the same mock/state behavior consistently
across the affected test cases so the resume flow is actually exercised.
|
@greptile re-review please |
|
Want your agent to iterate on Greptile's feedback? Try greploops. |
| const handleSaveDraft = useCallback(async () => { | ||
| setIsSavingDraft(true); | ||
| try { | ||
| const savedId = await useFeedbackDraftStore.getState().saveDraft(serializeDraft()); |
There was a problem hiding this comment.
serializeDraft() still sends id: '' until the async save returns and patches the active draft id. During that window, minimize or Save & Close can read the still-empty activeDraft from the store and issue a second save. The main handler treats each empty id as a new draft, so a quick Save draft followed by minimize or close can persist duplicate drafts instead of updating the first one. The first save should claim a stable id before it awaits IPC, or the other save paths should reuse or wait for the in-flight save.
Round-3 review fix: a brand-new draft serializes with id='' until the async save returns; the IPC handler mints a fresh UUID per empty-id call (outside the write queue), so two concurrent first-saves (e.g. Save Draft then immediate minimize/Save and Close) created duplicate rows. Serialize new-draft saves via a module-level in-flight promise: the concurrent call awaits it, reuses the minted id, and re-issues the save so the handler upserts the same row. Already-saved drafts are unaffected. +1 test.
|
@greptile re-review please |
1 participant
Summary
Closes #756. Send Feedback drafts now persist across modal/app close and become a browsable, resumable list with metadata and attachments. Previously a draft was only ephemeral in-memory React state.
Changes
feedback:drafts:list|save|deleteIPC handlers backed by auserDataJSON file (mirrors the playbooks persistence pattern:crypto.randomUUIDids, createdAt/updatedAt, upsert-by-id, capped, sorted byupdatedAtdesc).window.maestro.feedbacklockstep touch-points updated: preloadFeedbackApi, the inlineglobal.d.tsliteral, the store, and the test setup mock.feedbackDraftStoreextended withloadDrafts/saveDraft/deleteDraft/resume tracking;reset()now clears only ephemeral session state and preserves the drafts list.FeedbackDraftsListoverlay (suggested name, created/updated times, attachment indicator, resume/delete), a Drafts header button with count badge inFeedbackModal, resume wiring, and a save-or-discard close dialog.Verification
npm run lint(tsc x3): cleanvitest run: IPC handler, preload, store, and view tests: 38 tests passeslint+prettier: cleanSummary by CodeRabbit