feat(desktop): add feedback button and modal#1103
Closed
Kitenite wants to merge 6 commits into
Closed
Conversation
- Move organization dropdown to right side of TopBar - Report Issue now opens feedback modal instead of GitHub
Contributor
📝 WalkthroughWalkthroughAdds a complete feedback submission system featuring a FeedbackButton component with modal dialog for message and image attachment, a Zustand-based modal state store with persistence, a new PostgreSQL feedback table with user and organization relationships, a TRPC feedback.create mutation for server-side validation and persistence, and integrates the modal trigger into the OrganizationDropdown's Report Issue action. Changes
Sequence DiagramsequenceDiagram
participant User
participant FeedbackButton
participant FeedbackModalStore
participant TRPC Client
participant TRPC Server
participant Database
User->>FeedbackButton: Click Feedback Button
FeedbackButton->>FeedbackModalStore: openModal()
FeedbackModalStore->>FeedbackButton: Modal Opens
User->>FeedbackButton: Add message & images
FeedbackButton->>FeedbackModalStore: setMessage(), addImage()
FeedbackButton->>User: Display preview
User->>FeedbackButton: Submit
FeedbackButton->>TRPC Client: feedback.create.mutate({message, images})
TRPC Client->>TRPC Server: POST /trpc/feedback.create
TRPC Server->>TRPC Server: Validate input (zod)
TRPC Server->>Database: INSERT feedback record
Database->>Database: Create record with FK constraints
Database->>TRPC Server: Return created row
TRPC Server->>TRPC Client: Return success response
TRPC Client->>FeedbackButton: Mutation success
FeedbackButton->>User: Show success toast
FeedbackButton->>FeedbackModalStore: clearForm(), closeModal()
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/FeedbackButton/FeedbackButton.tsx`:
- Around line 49-71: handleFileSelect currently ignores FileReader failures and
assumes event.target.result is non-null; add an onerror handler on the
FileReader to handle read failures (reader.onerror) and report the error (e.g.,
via console.error or the app's notification/error handler) and avoid calling
addImage on failure. Also guard the onload result so if event.target?.result is
null skip adding the image and report an appropriate error, and ensure
fileInputRef.current.value is still cleared after both success and error paths;
update the FileReader usage in handleFileSelect to include these checks and
error reporting.
In `@apps/desktop/src/renderer/stores/feedback-modal.ts`:
- Around line 22-52: The persist is currently saving images (base64) which can
exceed localStorage quotas; update the useFeedbackModalStore persist config to
avoid persisting images by changing partialize to only include message (remove
images) or alternatively supply a custom storage implementation (e.g.,
IndexedDB) to the persist call; target the persist config and the partialize
function in useFeedbackModalStore and ensure image mutations (addImage,
removeImage, clearForm) continue to operate in-memory on the images array while
only message is written to persistent storage.
In `@packages/trpc/src/router/feedback/feedback.ts`:
- Around line 17-34: The destructured "created" from the
db.insert(...).returning() call in the mutation can be undefined if the returned
array is empty; update the mutation handler (the async function inside
.mutation) to check the returned array before destructuring or after (e.g.,
inspect the array result of db.insert(feedback).values(...).returning()), and if
no record was returned throw a descriptive error (or call ctx.throw / trpcError)
instead of returning undefined; ensure you reference the insert call and the
local variable "created" so the guard covers the db.insert(...).returning()
result and returns a valid object or throws.
🧹 Nitpick comments (3)
packages/db/drizzle/0017_add_feedback_table.sql (2)
1-9: Consider the implications of storing image data URLs directly in the database.Storing base64-encoded image data URLs in the
imagesjsonb column can lead to very large row sizes, potentially impacting:
- Query performance when selecting feedback records
- Database storage costs
- Backup/restore times
For a feedback feature with moderate volume, this may be acceptable. However, if feedback volume grows, consider uploading images to object storage (S3, etc.) and storing only the URLs.
13-14: Consider adding an index onorganization_idif org-scoped queries are expected.The migration creates indexes on
user_idandcreated_at, but not onorganization_id. If you anticipate querying feedback by organization (e.g., admin dashboard filtering by org), an index would improve performance.📊 Suggested index addition
CREATE INDEX "feedback_organization_id_idx" ON "feedback" USING btree ("organization_id");packages/trpc/src/router/feedback/feedback.ts (1)
10-15: Consider adding size limits for image data URLs.The input validation limits the array to 10 images but doesn't restrict individual string sizes. Since images are stored as base64 data URLs, a single image could be several megabytes. Consider adding a max length per image string to prevent excessively large payloads.
🛡️ Suggested validation enhancement
.input( z.object({ message: z.string().min(1).max(10000), - images: z.array(z.string()).max(10).default([]), + images: z.array(z.string().max(5_000_000)).max(10).default([]), // ~5MB per image metadata: z.record(z.string(), z.unknown()).optional(), }), )
Comment on lines
+49
to
+71
| const handleFileSelect = (e: React.ChangeEvent<HTMLInputElement>) => { | ||
| const files = e.target.files; | ||
| if (!files) return; | ||
|
|
||
| for (const file of files) { | ||
| if (!file.type.startsWith("image/")) continue; | ||
|
|
||
| const reader = new FileReader(); | ||
| reader.onload = (event) => { | ||
| const dataUrl = event.target?.result as string; | ||
| addImage({ | ||
| id: crypto.randomUUID(), | ||
| dataUrl, | ||
| name: file.name, | ||
| }); | ||
| }; | ||
| reader.readAsDataURL(file); | ||
| } | ||
|
|
||
| if (fileInputRef.current) { | ||
| fileInputRef.current.value = ""; | ||
| } | ||
| }; |
Contributor
There was a problem hiding this comment.
Add error handling for FileReader failures.
The FileReader.onerror callback is not handled. If a file read fails (e.g., corrupted file, permission issue), the user receives no feedback. Additionally, event.target?.result could be null.
🛡️ Proposed fix with error handling
const handleFileSelect = (e: React.ChangeEvent<HTMLInputElement>) => {
const files = e.target.files;
if (!files) return;
for (const file of files) {
if (!file.type.startsWith("image/")) continue;
const reader = new FileReader();
reader.onload = (event) => {
- const dataUrl = event.target?.result as string;
+ const dataUrl = event.target?.result;
+ if (typeof dataUrl !== "string") return;
addImage({
id: crypto.randomUUID(),
dataUrl,
name: file.name,
});
};
+ reader.onerror = () => {
+ console.error("[feedback] Failed to read file:", file.name);
+ toast.error(`Failed to read ${file.name}`);
+ };
reader.readAsDataURL(file);
}
if (fileInputRef.current) {
fileInputRef.current.value = "";
}
};🤖 Prompt for AI Agents
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/FeedbackButton/FeedbackButton.tsx`
around lines 49 - 71, handleFileSelect currently ignores FileReader failures and
assumes event.target.result is non-null; add an onerror handler on the
FileReader to handle read failures (reader.onerror) and report the error (e.g.,
via console.error or the app's notification/error handler) and avoid calling
addImage on failure. Also guard the onload result so if event.target?.result is
null skip adding the image and report an appropriate error, and ensure
fileInputRef.current.value is still cleared after both success and error paths;
update the FileReader usage in handleFileSelect to include these checks and
error reporting.
Comment on lines
+22
to
+52
| export const useFeedbackModalStore = create<FeedbackModalState>()( | ||
| devtools( | ||
| persist( | ||
| (set) => ({ | ||
| isOpen: false, | ||
| message: "", | ||
| images: [], | ||
|
|
||
| openModal: () => set({ isOpen: true }), | ||
| closeModal: () => set({ isOpen: false }), | ||
|
|
||
| setMessage: (message) => set({ message }), | ||
|
|
||
| addImage: (image) => | ||
| set((state) => ({ images: [...state.images, image] })), | ||
|
|
||
| removeImage: (id) => | ||
| set((state) => ({ | ||
| images: state.images.filter((img) => img.id !== id), | ||
| })), | ||
|
|
||
| clearForm: () => set({ message: "", images: [] }), | ||
| }), | ||
| { | ||
| name: "feedback-form-storage", | ||
| partialize: (state) => ({ | ||
| message: state.message, | ||
| images: state.images, | ||
| }), | ||
| }, | ||
| ), |
Contributor
There was a problem hiding this comment.
Persisting image data URLs can exceed storage quotas and break rehydration.
persist defaults to localStorage; storing base64 images there can easily hit quota and throw, which prevents draft persistence. Consider moving image persistence to a larger store (e.g., IndexedDB) or exclude images from persisted state and keep them in-memory only.
💡 Minimal mitigation (exclude images from persisted state)
{
name: "feedback-form-storage",
partialize: (state) => ({
message: state.message,
- images: state.images,
}),
},Zustand persist default storage (localStorage) size limits and how to configure custom storage (e.g., IndexedDB)
🤖 Prompt for AI Agents
In `@apps/desktop/src/renderer/stores/feedback-modal.ts` around lines 22 - 52, The
persist is currently saving images (base64) which can exceed localStorage
quotas; update the useFeedbackModalStore persist config to avoid persisting
images by changing partialize to only include message (remove images) or
alternatively supply a custom storage implementation (e.g., IndexedDB) to the
persist call; target the persist config and the partialize function in
useFeedbackModalStore and ensure image mutations (addImage, removeImage,
clearForm) continue to operate in-memory on the images array while only message
is written to persistent storage.
Comment on lines
+17
to
+34
| .mutation(async ({ ctx, input }) => { | ||
| const [created] = await db | ||
| .insert(feedback) | ||
| .values({ | ||
| userId: ctx.session.user.id, | ||
| organizationId: ctx.session.session.activeOrganizationId, | ||
| message: input.message, | ||
| images: input.images, | ||
| metadata: input.metadata, | ||
| }) | ||
| .returning(); | ||
|
|
||
| console.log( | ||
| "[feedback/create] Feedback submitted by user:", | ||
| ctx.session.user.id, | ||
| ); | ||
|
|
||
| return created; |
Contributor
There was a problem hiding this comment.
Handle potential undefined return from destructuring.
The destructured created could be undefined if the insert somehow returns an empty array (though Drizzle typically throws on failure). Adding a guard ensures type safety and prevents returning undefined to the client.
🛡️ Proposed fix
.mutation(async ({ ctx, input }) => {
const [created] = await db
.insert(feedback)
.values({
userId: ctx.session.user.id,
organizationId: ctx.session.session.activeOrganizationId,
message: input.message,
images: input.images,
metadata: input.metadata,
})
.returning();
+ if (!created) {
+ throw new Error("Failed to create feedback record");
+ }
+
console.log(
"[feedback/create] Feedback submitted by user:",
ctx.session.user.id,
);
return created;
}),📝 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
| .mutation(async ({ ctx, input }) => { | |
| const [created] = await db | |
| .insert(feedback) | |
| .values({ | |
| userId: ctx.session.user.id, | |
| organizationId: ctx.session.session.activeOrganizationId, | |
| message: input.message, | |
| images: input.images, | |
| metadata: input.metadata, | |
| }) | |
| .returning(); | |
| console.log( | |
| "[feedback/create] Feedback submitted by user:", | |
| ctx.session.user.id, | |
| ); | |
| return created; | |
| .mutation(async ({ ctx, input }) => { | |
| const [created] = await db | |
| .insert(feedback) | |
| .values({ | |
| userId: ctx.session.user.id, | |
| organizationId: ctx.session.session.activeOrganizationId, | |
| message: input.message, | |
| images: input.images, | |
| metadata: input.metadata, | |
| }) | |
| .returning(); | |
| if (!created) { | |
| throw new Error("Failed to create feedback record"); | |
| } | |
| console.log( | |
| "[feedback/create] Feedback submitted by user:", | |
| ctx.session.user.id, | |
| ); | |
| return created; | |
| }), |
🤖 Prompt for AI Agents
In `@packages/trpc/src/router/feedback/feedback.ts` around lines 17 - 34, The
destructured "created" from the db.insert(...).returning() call in the mutation
can be undefined if the returned array is empty; update the mutation handler
(the async function inside .mutation) to check the returned array before
destructuring or after (e.g., inspect the array result of
db.insert(feedback).values(...).returning()), and if no record was returned
throw a descriptive error (or call ctx.throw / trpcError) instead of returning
undefined; ensure you reference the insert call and the local variable "created"
so the guard covers the db.insert(...).returning() result and returns a valid
object or throws.
Contributor
🚀 Preview Deployment🔗 Preview Links
Preview updates automatically with new commits |
Collaborator
Author
|
Closing — stale and conflicting, will restart fresh if needed. |
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
Changes
feedbacktable in database schemafeedback.createtRPC mutationFeedbackButtoncomponent with modalfeedback-modalZustand store (persists draft until sent/cleared)OrganizationDropdownto open feedback modal from "Report Issue"Test plan
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.