-
Notifications
You must be signed in to change notification settings - Fork 25
feat(comments): R2 media uploads and composer improvements #143
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,12 +5,14 @@ import { | |
| } from "@diffkit/icons"; | ||
| import { | ||
| MarkdownEditor, | ||
| type MarkdownEditorHandle, | ||
| type MentionCandidate, | ||
| } from "@diffkit/ui/components/markdown-editor"; | ||
| import { toast } from "@diffkit/ui/components/sonner"; | ||
| import { Spinner } from "@diffkit/ui/components/spinner"; | ||
| import { useQuery, useQueryClient } from "@tanstack/react-query"; | ||
| import { useMemo, useState } from "react"; | ||
| import { useMemo, useRef, useState } from "react"; | ||
| import { useCommentMediaUpload } from "#/hooks/use-comment-media-upload"; | ||
| import { createComment, updatePullState } from "#/lib/github.functions"; | ||
| import { | ||
| type GitHubQueryScope, | ||
|
|
@@ -60,6 +62,10 @@ export function DetailCommentBox({ | |
| const [isTogglingState, setIsTogglingState] = useState(false); | ||
| const [mentionActivated, setMentionActivated] = useState(false); | ||
| const queryClient = useQueryClient(); | ||
| const editorRef = useRef<MarkdownEditorHandle>(null); | ||
| const commentActionsRef = useRef<HTMLDivElement>(null); | ||
| const { media: mediaUpload, onPaste: onMediaPaste } = | ||
| useCommentMediaUpload(editorRef); | ||
|
Comment on lines
+67
to
+68
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Disable comment actions while uploads are still pending.
Also applies to: 176-183 🤖 Prompt for AI Agents |
||
|
|
||
| const viewerQuery = useQuery(githubViewerQueryOptions(scope)); | ||
| const viewerLogin = viewerQuery.data?.login; | ||
|
|
@@ -167,17 +173,24 @@ export function DetailCommentBox({ | |
| return ( | ||
| <div className="flex flex-col gap-2"> | ||
| <MarkdownEditor | ||
| ref={editorRef} | ||
| scrollAnchorRef={commentActionsRef} | ||
| value={value} | ||
| onChange={setValue} | ||
| placeholder="Leave a comment..." | ||
| compact | ||
| media={mediaUpload} | ||
| onPaste={onMediaPaste} | ||
| mentions={{ | ||
| candidates: mentionCandidates, | ||
| onActivate: () => setMentionActivated(true), | ||
| isLoading: collaboratorsQuery.isLoading && mentionActivated, | ||
| }} | ||
| /> | ||
| <div className="flex items-center justify-end gap-2"> | ||
| <div | ||
| ref={commentActionsRef} | ||
| className="flex items-center justify-end gap-2 pb-3" | ||
| > | ||
| {pullState && ( | ||
| <button | ||
| type="button" | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -53,6 +53,16 @@ export default { | |||||||||||||||||||||||||||||||||||||||||||||
| return handleWebSocketUpgrade(request, env); | ||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| if ( | ||||||||||||||||||||||||||||||||||||||||||||||
| url.pathname === "/api/comment-media/upload" && | ||||||||||||||||||||||||||||||||||||||||||||||
| request.method === "POST" | ||||||||||||||||||||||||||||||||||||||||||||||
| ) { | ||||||||||||||||||||||||||||||||||||||||||||||
| const { handleCommentMediaUpload } = await import( | ||||||||||||||||||||||||||||||||||||||||||||||
| "#/lib/comment-media-upload.handler" | ||||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||||
| return handleCommentMediaUpload(request); | ||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+56
to
+64
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Apply security headers on early API return path too. The new upload branch (Line 56-Line 64) returns before the global header-setting block, so this endpoint misses 💡 Proposed fix if (
url.pathname === "/api/comment-media/upload" &&
request.method === "POST"
) {
const { handleCommentMediaUpload } = await import(
"#/lib/comment-media-upload.handler"
);
- return handleCommentMediaUpload(request);
+ const response = await handleCommentMediaUpload(request);
+ for (const [key, value] of Object.entries(SECURITY_HEADERS)) {
+ response.headers.set(key, value);
+ }
+ return response;
}📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| // TanStack Start's type only declares (request, env?) but the runtime | ||||||||||||||||||||||||||||||||||||||||||||||
| // handler created by @cloudflare/vite-plugin passes (request, env, ctx) | ||||||||||||||||||||||||||||||||||||||||||||||
| // through to the underlying Worker fetch signature. | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,206 @@ | ||
| import { | ||
| getCommentMediaUploadPlaceholderText, | ||
| type MarkdownEditorHandle, | ||
| type MarkdownEditorMediaUpload, | ||
| } from "@diffkit/ui/components/markdown-editor"; | ||
| import { toast } from "@diffkit/ui/components/sonner"; | ||
| import { useCallback } from "react"; | ||
| import { useDropzone } from "react-dropzone"; | ||
| import type { CommentMediaKind } from "#/lib/comment-media.server"; | ||
| import { | ||
| type FinalizeCommentMediaResult, | ||
| finalizeCommentMediaUpload, | ||
| } from "#/lib/media.functions"; | ||
|
|
||
| async function probeImageDimensions(file: File): Promise<{ | ||
| width: number; | ||
| height: number; | ||
| }> { | ||
| const url = URL.createObjectURL(file); | ||
| try { | ||
| return await new Promise<{ width: number; height: number }>( | ||
| (resolve, reject) => { | ||
| const img = new Image(); | ||
| img.onload = () => { | ||
| resolve({ width: img.naturalWidth, height: img.naturalHeight }); | ||
| }; | ||
| img.onerror = () => | ||
| reject(new Error("Could not read image dimensions")); | ||
| img.src = url; | ||
| }, | ||
| ); | ||
| } finally { | ||
| URL.revokeObjectURL(url); | ||
| } | ||
| } | ||
|
|
||
| async function probeVideoDimensions(file: File): Promise<{ | ||
| width: number; | ||
| height: number; | ||
| }> { | ||
| const url = URL.createObjectURL(file); | ||
| try { | ||
| return await new Promise<{ width: number; height: number }>( | ||
| (resolve, reject) => { | ||
| const video = document.createElement("video"); | ||
| video.preload = "metadata"; | ||
| video.onloadedmetadata = () => { | ||
| resolve({ width: video.videoWidth, height: video.videoHeight }); | ||
| }; | ||
| video.onerror = () => | ||
| reject(new Error("Could not read video dimensions")); | ||
| video.src = url; | ||
| }, | ||
| ); | ||
| } finally { | ||
| URL.revokeObjectURL(url); | ||
| } | ||
| } | ||
|
|
||
| async function probeDimensions( | ||
| file: File, | ||
| kind: CommentMediaKind, | ||
| ): Promise<{ width: number; height: number }> { | ||
| try { | ||
| return kind === "image" | ||
| ? await probeImageDimensions(file) | ||
| : await probeVideoDimensions(file); | ||
| } catch { | ||
| return { width: 1, height: 1 }; | ||
| } | ||
| } | ||
|
|
||
| type UploadJson = { | ||
| key: string; | ||
| publicUrl: string; | ||
| kind: CommentMediaKind; | ||
| contentType: string; | ||
| }; | ||
|
|
||
| export function useCommentMediaUpload( | ||
| editorRef: React.RefObject<MarkdownEditorHandle | null>, | ||
| ) { | ||
| const uploadAndInsert = useCallback( | ||
| async (file: File) => { | ||
| const id = crypto.randomUUID(); | ||
| const placeholder = getCommentMediaUploadPlaceholderText(id); | ||
| editorRef.current?.insertAtCaret(`${placeholder}\n`); | ||
|
|
||
| const formData = new FormData(); | ||
| formData.append("file", file); | ||
|
|
||
| try { | ||
| const response = await fetch("/api/comment-media/upload", { | ||
| method: "POST", | ||
| body: formData, | ||
| credentials: "include", | ||
| }); | ||
|
|
||
| if (!response.ok) { | ||
| let message = "Upload failed"; | ||
| try { | ||
| const payload = (await response.json()) as { error?: string }; | ||
| if (payload.error) message = payload.error; | ||
| } catch { | ||
| // ignore | ||
| } | ||
| throw new Error(message); | ||
| } | ||
|
|
||
| const payload = (await response.json()) as UploadJson; | ||
| const dimensions = await probeDimensions(file, payload.kind); | ||
|
|
||
| const finalized: FinalizeCommentMediaResult = | ||
| await finalizeCommentMediaUpload({ | ||
| data: { | ||
| key: payload.key, | ||
| width: dimensions.width, | ||
| height: dimensions.height, | ||
| kind: payload.kind, | ||
| fileName: file.name, | ||
| }, | ||
| }); | ||
|
|
||
| if (!finalized.ok) { | ||
| throw new Error(finalized.error); | ||
| } | ||
|
|
||
| editorRef.current?.replaceUploadPlaceholder(id, `${finalized.html}\n`); | ||
| } catch (error) { | ||
| const message = | ||
| error instanceof Error ? error.message : "Upload failed"; | ||
| toast.error(message); | ||
| editorRef.current?.replaceUploadPlaceholder( | ||
| id, | ||
| `*Could not upload "${file.name}": ${message}*\n`, | ||
| ); | ||
| } | ||
| }, | ||
| [editorRef], | ||
| ); | ||
|
|
||
| const processFiles = useCallback( | ||
| async (files: File[]) => { | ||
| if (files.length === 0) return; | ||
| for (const file of files) { | ||
| await uploadAndInsert(file); | ||
| } | ||
| }, | ||
| [uploadAndInsert], | ||
| ); | ||
|
|
||
| const onDrop = useCallback( | ||
| (acceptedFiles: File[]) => { | ||
| void processFiles(acceptedFiles); | ||
| }, | ||
| [processFiles], | ||
| ); | ||
|
|
||
| const { getRootProps, getInputProps, isDragActive, open } = useDropzone({ | ||
| onDrop, | ||
| noClick: true, | ||
| noKeyboard: true, | ||
| accept: { | ||
| "image/*": [".png", ".jpg", ".jpeg", ".gif", ".webp"], | ||
| "video/*": [".mp4", ".webm", ".mov"], | ||
| }, | ||
| }); | ||
|
|
||
| const onPaste = useCallback( | ||
| (event: React.ClipboardEvent<HTMLTextAreaElement>) => { | ||
| const items = event.clipboardData?.items; | ||
| if (!items) return; | ||
|
|
||
| const files: File[] = []; | ||
| for (const item of items) { | ||
| if (item.kind !== "file") continue; | ||
| const file = item.getAsFile(); | ||
| if (!file) continue; | ||
| if ( | ||
| !file.type.startsWith("image/") && | ||
| !file.type.startsWith("video/") | ||
| ) { | ||
| continue; | ||
| } | ||
| files.push(file); | ||
| } | ||
|
|
||
| if (files.length === 0) return; | ||
|
|
||
| event.preventDefault(); | ||
| void processFiles(files); | ||
| }, | ||
| [processFiles], | ||
| ); | ||
|
|
||
| const media: MarkdownEditorMediaUpload = { | ||
| isDragActive, | ||
| rootProps: getRootProps(), | ||
| inputProps: getInputProps(), | ||
| onToolbarAttach: () => { | ||
| open(); | ||
| }, | ||
| }; | ||
|
|
||
| return { media, onPaste }; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
Repository: stylessh/diffkit
Length of output: 5112
🏁 Script executed:
fd "comment-reply-form.tsx" apps/dashboard/src --exec cat -n {}Repository: stylessh/diffkit
Length of output: 4221
Gate submit button on pending media uploads to prevent submitting with unresolved placeholders.
The
disabled={!value.trim() || isSending}condition on line 116 does not account for in-flight uploads. Users can currently submit comments containing unresolved upload placeholders. TheuseCommentMediaUploadhook does not expose a pending upload state, so either:Affected lines: 37-38, 96-97, 116-117