Open
Conversation
anirudhk-tech
requested review from
Rod-Christensen,
jmaionchi and
stepmikhaylov
as code owners
github-actions
bot
added
module:ui
|
No description provided. |
Contributor
|
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 end-to-end video support to Dropper: SSE video chunk handling and assembly, blob URL lifecycle management, parsing/grouping video results, a new VideosView component, UI/tabs wiring for videos, and an optional onSSE callback on the client upload API. Also includes error handling for malformed bridged file entries. Changes
Sequence DiagramsequenceDiagram
participant User
participant Client as Dropper UI
participant API as RocketRide API
participant Hook as useFileProcessing
participant BlobMgr as Blob URL Manager
participant View as VideosView
User->>Client: select/upload video file(s)
Client->>API: sendFiles(files, token, onSSE)
API-->>Hook: SSE (video_chunk) events
Hook->>BlobMgr: accumulate base64 chunks per filename
API-->>Hook: SSE (video_complete)
Hook->>BlobMgr: assemble Blob → create blob URL
Hook->>Client: update results state (includes videos)
Client->>View: render VideosView with grouped video URLs
View->>User: play/display video (onError -> show fallback)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/dropper-ui/src/components/DropperContainer.tsx (2)
165-183: 🧹 Nitpick | 🔵 TrivialComment is outdated after adding videos tab.
The comment on line 167 states the priority as "text > tables > images > results" but the actual implementation now includes
videosin the priority chain. Update the comment to reflect the current behavior.📝 Suggested fix
/** * Auto-switch to the most relevant tab when results are available - * Priority: text > tables > images > results + * Priority: text > tables > images > videos > results */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/dropper-ui/src/components/DropperContainer.tsx` around lines 165 - 183, Update the outdated comment above the useEffect that auto-switches tabs to reflect the actual priority order used by the logic: include "videos" in the chain so it matches the implementation (i.e., text > tables > images > videos > results); locate the useEffect block that checks results.textContent, results.tables, results.images, results.videos and calls setActiveTab to edit the comment accordingly.
84-92:⚠️ Potential issue | 🟡 MinorPotential loss of error context when
reasonis empty.The change from preserving the previous error message (
prev) to usingreason || nullwhenhasErroris true can result in losing a meaningful error message. If a disconnection event fires withhasError: truebut an emptyreasonstring, the previous informative error message will be replaced withnull.Consider preserving the previous error when
reasonis falsy:🛠️ Suggested fix
- setConnectionErrorMessage((prev) => (hasError ? reason || null : prev)); + setConnectionErrorMessage((prev) => (hasError ? reason || prev : prev));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/dropper-ui/src/components/DropperContainer.tsx` around lines 84 - 92, The handler handleDisconnected currently replaces the prior error message with reason || null when hasError is true, which discards a previous informative message if reason is an empty/falsy string; update the setConnectionErrorMessage call inside handleDisconnected to preserve the previous message when reason is falsy (e.g., use prev when reason is falsy rather than null) so that setConnectionErrorMessage((prev) => hasError ? (reason || prev) : prev) and leave the rest of the logic around connectionAttemptsRef and setStatusMessage unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/dropper-ui/src/hooks/useFileProcessing.ts`:
- Around line 185-206: The code assembling SSE video chunks
(videoChunksRef.current, used inside the uploadResults.map callback) must
validate that all expected chunks are present and defined before joining and
calling atob to avoid "undefined" being concatenated and corrupting the Blob;
update the assembly logic to check videoEntry.chunks for missing/undefined
entries and (if available) verify count against an expected total (or ensure
every element is a non-empty string), and if validation fails skip assembly for
that upload result (or mark it as incomplete and log an error) rather than
calling atob; keep the existing URL.revokeObjectURL and URL.createObjectURL
behavior (videoBlobUrlsRef) only when validation passes, so parseDropperResults
receives only valid enrichedResults.
- Around line 246-259: The client hook is passing an onSSE callback to
client.sendFiles but sendFiles currently only accepts (files, token) and ignores
a third arg, so video_chunk and video_complete events never reach
videoChunksRef; update the client API and implementation: modify sendFiles in
packages/client-typescript/src/client/client.ts to accept an optional onSSE
callback (match the pattern used by chat()), propagate SSE messages to that
callback and/or emit video_chunk and video_complete events when streaming
chunks, update any relevant types/signatures, and then keep the hook's
client.sendFiles(filesWithMimeTypes, authToken, async (type, data) => { ... })
call intact so videoChunksRef is populated. Ensure the emitted event names
exactly match "video_chunk" and "video_complete" and payload shapes expected by
VideoChunkSSEData/VideoCompleteSSEData.
---
Outside diff comments:
In `@apps/dropper-ui/src/components/DropperContainer.tsx`:
- Around line 165-183: Update the outdated comment above the useEffect that
auto-switches tabs to reflect the actual priority order used by the logic:
include "videos" in the chain so it matches the implementation (i.e., text >
tables > images > videos > results); locate the useEffect block that checks
results.textContent, results.tables, results.images, results.videos and calls
setActiveTab to edit the comment accordingly.
- Around line 84-92: The handler handleDisconnected currently replaces the prior
error message with reason || null when hasError is true, which discards a
previous informative message if reason is an empty/falsy string; update the
setConnectionErrorMessage call inside handleDisconnected to preserve the
previous message when reason is falsy (e.g., use prev when reason is falsy
rather than null) so that setConnectionErrorMessage((prev) => hasError ? (reason
|| prev) : prev) and leave the rest of the logic around connectionAttemptsRef
and setStatusMessage 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: ASSERTIVE
Plan: Pro
Run ID: 30c3f715-82de-472f-bbba-383b0a16a991
📒 Files selected for processing (8)
apps/dropper-ui/src/components/DropperContainer.tsxapps/dropper-ui/src/components/ResultsContent.tsxapps/dropper-ui/src/components/ResultsTabs.tsxapps/dropper-ui/src/components/Views/VideosView.tsxapps/dropper-ui/src/hooks/useFileProcessing.tsapps/dropper-ui/src/types/dropper.types.tsapps/dropper-ui/src/utils/dropperUtils.tspackages/client-typescript/src/client/types/data.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
apps/dropper-ui/src/hooks/useFileProcessing.ts (1)
190-194:⚠️ Potential issue | 🟠 MajorStill missing: validate the chunk list before decoding it.
videoEntry.chunks.join('')will stringify sparse holes / skipped entries, andatob()will throw on that payload. When that happens, this effect exits beforesetIsProcessing(false), so the batch can stay stuck in the processing state. The new size-cap branch makes this especially easy to hit because it intentionally stops buffering later chunks.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/dropper-ui/src/hooks/useFileProcessing.ts` around lines 190 - 194, The code using videoChunksRef.current.get(r.filepath) and then calling atob(videoEntry.chunks.join('')) must validate and sanitize the chunk list before decoding: check videoEntry.chunks for holes/undefined/null entries and either filter them out or ensure contiguous indexing (e.g., verify length equals expected count) and only join valid strings; additionally wrap the decoding logic (the atob + byte conversion) in a try/catch and ensure setIsProcessing(false) is always called in the finally path so a thrown error cannot leave the batch stuck in processing. Use the existing symbols videoEntry, videoEntry.chunks, atob, and setIsProcessing to locate where to add the validation, filtering, and error handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/dropper-ui/src/hooks/useFileProcessing.ts`:
- Around line 254-257: The current early return when entry.totalBytes + incoming
> MAX_VIDEO_BASE64_BYTES only drops the current chunk but lets later
`'complete'` handling promote the file back to 'completed'; modify the logic
around the size check (referencing entry.totalBytes, incoming,
MAX_VIDEO_BASE64_BYTES and setUploadedFiles) to mark the file as terminally
oversized (e.g., add an oversized flag or store the filename in an oversized
set), clear any partial buffer/state for that filename, and ensure downstream
handlers (the `'complete'` event path and video enrichment/status promotion
code) check this oversized marker and skip assembling chunks, enrichment and
status changes for that file so it remains in an 'error'/'oversized' terminal
state.
- Around line 198-199: The created Blob URL keeps the full base64/chunk data
alive in videoChunksRef, so after calling URL.createObjectURL(...) you should
free the original buffer: locate the code that creates blobUrl (using bytes,
videoEntry.mimeType, URL.createObjectURL and
videoBlobUrlsRef.current.set(r.filepath, blobUrl)) and remove or null out the
corresponding entry in videoChunksRef (e.g., delete
videoChunksRef.current.get(r.filepath) or
videoChunksRef.current.delete(r.filepath) / set to undefined) so the large chunk
array can be GC'd; ensure you only remove the chunks after blobUrl is created
and retained in videoBlobUrlsRef, and keep any existing blob URL revocation
logic elsewhere intact.
- Around line 299-303: Before clearing state in the reset block (where
setUploadProgress, setUploadResults and videoChunksRef.current.clear() are
called), iterate over videoBlobUrlsRef.current and call URL.revokeObjectURL on
each stored URL to free memory, then clear the map (e.g.,
videoBlobUrlsRef.current.clear()) so prior batch Blob URLs are revoked before
starting a fresh run; ensure this happens in the same reset code path
(referencing videoBlobUrlsRef, videoChunksRef, setUploadProgress,
setUploadResults and clearAll()).
---
Duplicate comments:
In `@apps/dropper-ui/src/hooks/useFileProcessing.ts`:
- Around line 190-194: The code using videoChunksRef.current.get(r.filepath) and
then calling atob(videoEntry.chunks.join('')) must validate and sanitize the
chunk list before decoding: check videoEntry.chunks for holes/undefined/null
entries and either filter them out or ensure contiguous indexing (e.g., verify
length equals expected count) and only join valid strings; additionally wrap the
decoding logic (the atob + byte conversion) in a try/catch and ensure
setIsProcessing(false) is always called in the finally path so a thrown error
cannot leave the batch stuck in processing. Use the existing symbols videoEntry,
videoEntry.chunks, atob, and setIsProcessing to locate where to add the
validation, filtering, and error handling.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 9ded5f7f-db95-4d74-a2a1-8ca332dc5ad3
📒 Files selected for processing (3)
apps/dropper-ui/src/components/Views/VideosView.tsxapps/dropper-ui/src/hooks/useFileProcessing.tspackages/client-typescript/src/client/client.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
apps/dropper-ui/src/hooks/useFileProcessing.ts (3)
307-311:⚠️ Potential issue | 🟠 MajorRevoke prior batch Blob URLs before starting a fresh run.
processFilesclearsvideoChunksRefbut leaves the previous batch's object URLs invideoBlobUrlsRef. Those Blobs remain pinned in memory until unmount orclearAll().🧹 Proposed fix
setUploadProgress([]); setUploadResults([]); + videoBlobUrlsRef.current.forEach((url) => URL.revokeObjectURL(url)); + videoBlobUrlsRef.current.clear(); videoChunksRef.current.clear();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/dropper-ui/src/hooks/useFileProcessing.ts` around lines 307 - 311, In processFiles, revoke any previously created object URLs stored in videoBlobUrlsRef before clearing state: iterate videoBlobUrlsRef.current (or its Map/array) and call URL.revokeObjectURL on each URL, then clear videoBlobUrlsRef.current along with setUploadProgress, setUploadResults, and videoChunksRef.current.clear(); ensure this revocation happens before starting the new run so old Blob URLs are not retained in memory (refer to processFiles, videoBlobUrlsRef, videoChunksRef, and clearAll semantics).
206-207:⚠️ Potential issue | 🟠 MajorFree the base64 buffer once the Blob URL is created.
After line 207, the UI only needs
blobUrl, but the full chunk array remains invideoChunksRef. This keeps both representations of the same video in memory.🧹 Proposed fix
const blobUrl = URL.createObjectURL(new Blob([bytes], { type: videoEntry.mimeType })); videoBlobUrlsRef.current.set(r.filepath, blobUrl); + videoChunksRef.current.delete(r.filepath);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/dropper-ui/src/hooks/useFileProcessing.ts` around lines 206 - 207, The code creates a Blob URL and stores it in videoBlobUrlsRef.current via URL.createObjectURL (using bytes and videoEntry.mimeType) but leaves the original base64/chunk buffer in videoChunksRef, retaining duplicate large memory; after creating and storing blobUrl (for r.filepath) remove the corresponding entry from videoChunksRef.current (e.g., delete or set to undefined for the key r.filepath) so only the blob URL remains; keep videoBlobUrlsRef, videoChunksRef, r.filepath, bytes, and URL.createObjectURL in mind when applying the change.
262-266:⚠️ Potential issue | 🟠 MajorOversize video state is not terminal for that file.
Returning early only drops the current chunk. The
videoChunksRefentry still exists, so subsequent smaller chunks continue accumulating. Whenvideo_completearrives and the upload finishes, the enrichment logic at lines 189-216 will attempt to assemble whatever partial chunks were buffered (unless they coincidentally fail validation).Consider clearing the entry and tracking an oversized flag to skip all further processing for that filename:
🛠️ Proposed fix sketch
+ /** Filenames that exceeded the video size limit — skip further chunk/enrichment processing */ + const oversizedFilesRef = useRef<Set<string>>(new Set()); // Inside the onSSE callback: if (type === 'video_chunk') { const chunk = data as unknown as VideoChunkSSEData; + if (oversizedFilesRef.current.has(chunk.filename)) return; const entry = videoChunksRef.current.get(chunk.filename) ?? { chunks: [], mimeType: chunk.mime_type, totalBytes: 0 }; const incoming = chunk.data.length; if (entry.totalBytes + incoming > MAX_VIDEO_BASE64_BYTES) { + oversizedFilesRef.current.add(chunk.filename); + videoChunksRef.current.delete(chunk.filename); setUploadedFiles((prev) => prev.map((f) => (f.file.name === chunk.filename ? { ...f, status: 'error' as const, error: 'Video too large to display in browser (limit: ~96 MB).' } : f))); return; }Then in the enrichment loop, skip files in
oversizedFilesRef.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/dropper-ui/src/hooks/useFileProcessing.ts` around lines 262 - 266, When a chunk would push entry.totalBytes over MAX_VIDEO_BASE64_BYTES, mark the file errored via setUploadedFiles as you already do, then clear the corresponding buffer in videoChunksRef (delete videoChunksRef.current[chunk.filename]) and record the filename in a new oversizedFilesRef (or similar Set) so future chunks are ignored; also update the enrichment loop that runs on "video_complete" to skip any filenames present in oversizedFilesRef so no partial assembly/validation occurs for those files. Ensure the chunk-receiving code checks oversizedFilesRef early and drops subsequent chunks silently for that filename.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/dropper-ui/src/components/DropperContainer.tsx`:
- Around line 115-117: The bridged file creation loop can throw if f.buffer is
not a valid ArrayBuffer — wrap the per-file creation in a defensive guard and
error handling: inside the block that iterates event.data.files (referencing
DataTransfer, event.data.files, and addFiles), verify f.buffer is an ArrayBuffer
or convertible (e.g., ArrayBuffer, Uint8Array) and coerce it if needed, and wrap
new File([...]) in a try-catch so malformed entries are skipped (or collected)
and a console.warn or logger records the filename and error; ensure addFiles is
only called with the successfully created FileList (or silently ignore bad
entries).
---
Duplicate comments:
In `@apps/dropper-ui/src/hooks/useFileProcessing.ts`:
- Around line 307-311: In processFiles, revoke any previously created object
URLs stored in videoBlobUrlsRef before clearing state: iterate
videoBlobUrlsRef.current (or its Map/array) and call URL.revokeObjectURL on each
URL, then clear videoBlobUrlsRef.current along with setUploadProgress,
setUploadResults, and videoChunksRef.current.clear(); ensure this revocation
happens before starting the new run so old Blob URLs are not retained in memory
(refer to processFiles, videoBlobUrlsRef, videoChunksRef, and clearAll
semantics).
- Around line 206-207: The code creates a Blob URL and stores it in
videoBlobUrlsRef.current via URL.createObjectURL (using bytes and
videoEntry.mimeType) but leaves the original base64/chunk buffer in
videoChunksRef, retaining duplicate large memory; after creating and storing
blobUrl (for r.filepath) remove the corresponding entry from
videoChunksRef.current (e.g., delete or set to undefined for the key r.filepath)
so only the blob URL remains; keep videoBlobUrlsRef, videoChunksRef, r.filepath,
bytes, and URL.createObjectURL in mind when applying the change.
- Around line 262-266: When a chunk would push entry.totalBytes over
MAX_VIDEO_BASE64_BYTES, mark the file errored via setUploadedFiles as you
already do, then clear the corresponding buffer in videoChunksRef (delete
videoChunksRef.current[chunk.filename]) and record the filename in a new
oversizedFilesRef (or similar Set) so future chunks are ignored; also update the
enrichment loop that runs on "video_complete" to skip any filenames present in
oversizedFilesRef so no partial assembly/validation occurs for those files.
Ensure the chunk-receiving code checks oversizedFilesRef early and drops
subsequent chunks silently for that filename.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 5f23c93b-3c00-4e6a-8e14-5c3f59a4329b
📒 Files selected for processing (2)
apps/dropper-ui/src/components/DropperContainer.tsxapps/dropper-ui/src/hooks/useFileProcessing.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/dropper-ui/src/hooks/useFileProcessing.ts`:
- Around line 268-287: The handler in useFileProcessing.ts is using unsafe
double-casts (as unknown as VideoChunkSSEData / VideoCompleteSSEData); add
runtime type guards (e.g., isVideoChunkSSEData(data) and
isVideoCompleteSSEData(data)) that validate required fields (filename,
chunk_index, data, mime_type/total_chunks for chunks; filename for complete) and
use them before treating data as VideoChunkSSEData/VideoCompleteSSEData; if
validation fails, log a warning and return. Update the video_chunk branch to use
the validated typed variable instead of the double-cast when accessing
chunk.filename, chunk.data, chunk.chunk_index and when updating
videoChunksRef.current and setUploadedFiles, and apply the same guard in the
video_complete branch before touching videoChunksRef.current.
- Around line 196-203: The current validation uses videoEntry.chunks.every(...)
which skips holes in sparse arrays; capture and store
VideoChunkSSEData.total_chunks when chunks arrive (e.g., set
videoEntry.totalChunks in the SSE chunk handler) and then validate assembly by
confirming videoEntry.totalChunks is a positive integer and that
videoEntry.chunks has exactly totalChunks defined entries with no holes (iterate
indices 0..totalChunks-1 or count non-empty string entries and compare to
totalChunks) before joining/atob; update both the chunk handler (where incoming
chunks are processed) and the assembly check that references videoEntry.chunks
to use videoEntry.totalChunks for completeness validation.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: f70614af-4e9f-4a35-9a47-57f1bda6c0ac
📒 Files selected for processing (1)
apps/dropper-ui/src/hooks/useFileProcessing.ts
anirudhk-tech
force-pushed
the
feat/dropper-video-extension-v2
branch
from
21d8bec to
bd9ae06
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/dropper-ui/src/components/DropperContainer.tsx (1)
116-123:⚠️ Potential issue | 🟡 MinorValidate bridged buffers before constructing
File.Line 118 still relies on
Filethrowing on malformedbuffervalues. That misses cases where the bridge delivers a plain object ornumber[]—theBlob/Fileconstructors can coerce those into string data and silently upload corrupted bytes instead of hitting thecatch. ValidateArrayBuffer/ typed arrays up front, and only coerce shapes you explicitly support.🛡️ Suggested hardening
- event.data.files.forEach((f: { buffer: ArrayBuffer; name: string; type: string; lastModified: number }) => { + event.data.files.forEach((f: { buffer: unknown; name: string; type: string; lastModified: number }) => { try { - dt.items.add(new File([f.buffer], f.name, { type: f.type, lastModified: f.lastModified })); + const bytes = + f.buffer instanceof ArrayBuffer + ? f.buffer + : ArrayBuffer.isView(f.buffer) + ? f.buffer + : Array.isArray(f.buffer) + ? new Uint8Array(f.buffer) + : null; + + if (!bytes) { + console.warn('[DropperContainer] Skipping malformed bridged file entry:', f.name); + return; + } + + dt.items.add(new File([bytes], f.name, { type: f.type, lastModified: f.lastModified })); } catch (err) { console.warn('[DropperContainer] Skipping malformed bridged file entry:', f.name, err); } }); - addFiles(dt.files); + if (dt.files.length > 0) { + addFiles(dt.files); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/dropper-ui/src/components/DropperContainer.tsx` around lines 116 - 123, The bridged file handling in DropperContainer (inside the event.data.files.forEach loop before dt.items.add and addFiles) must validate f.buffer types instead of relying on File to throw: explicitly accept only ArrayBuffer or ArrayBufferView (typed arrays / DataView) and, if accepted, normalize to an ArrayBuffer copy (e.g., use ArrayBuffer.isView check and slice/copy the underlying buffer) before constructing new File; for any other shapes (plain object, number[], string, null/undefined) log a warning and skip the entry so dt.items.add/new File never receives coerced/garbage data. Ensure the validation and normalization happen inline in the loop that calls dt.items.add and reference f.buffer, f.name, f.type, f.lastModified when logging skipped entries.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/dropper-ui/src/components/DropperContainer.tsx`:
- Around line 322-345: The UI conditional rendering in the DropperContainer
component currently only shows the connection error panel when isConnected is
false and statusMessage === 'CONNECTION_FAILED' (plus connectionErrorMessage);
update that conditional to also check for the disconnected-upload status (e.g.,
statusMessage === 'DISCONNECTED_UPLOAD') so rejected/disconnected uploads
surface to the user; in short, modify the JSX condition around
statusMessage/connectionErrorMessage (the block rendering the
connection-error-panel) to include 'DISCONNECTED_UPLOAD' (and any other relevant
upload-rejection status strings emitted elsewhere) alongside
'CONNECTION_FAILED'.
---
Duplicate comments:
In `@apps/dropper-ui/src/components/DropperContainer.tsx`:
- Around line 116-123: The bridged file handling in DropperContainer (inside the
event.data.files.forEach loop before dt.items.add and addFiles) must validate
f.buffer types instead of relying on File to throw: explicitly accept only
ArrayBuffer or ArrayBufferView (typed arrays / DataView) and, if accepted,
normalize to an ArrayBuffer copy (e.g., use ArrayBuffer.isView check and
slice/copy the underlying buffer) before constructing new File; for any other
shapes (plain object, number[], string, null/undefined) log a warning and skip
the entry so dt.items.add/new File never receives coerced/garbage data. Ensure
the validation and normalization happen inline in the loop that calls
dt.items.add and reference f.buffer, f.name, f.type, f.lastModified when logging
skipped entries.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 285445b3-19d7-47a8-bf9f-c9f57e441f65
📒 Files selected for processing (1)
apps/dropper-ui/src/components/DropperContainer.tsx
7 tasks
Collaborator
Author
|
@nihalnihalani @asclearuc could I get a review on this please? I addressed your comments on the old PR I closed. |
The cherry-picks from the old dropper-video branch brought in stale client.ts formatting changes (pre-dating the handle-based streaming rewrite). Reset to develop's version to avoid regressing ConnectionException import and the URI port-detection logic added in #377. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…rror logging - Restore onSSE parameter to sendFiles() and pass it through to the underlying pipe, so video chunk SSE events actually reach the UI (was silently dropped when client.ts was rebased onto develop) - Add finally block in uploadFile to close the pipe on write errors, preventing a server-side resource leak - Add MAX_VIDEO_BASE64_BYTES (128 MB) cap on SSE chunk accumulation with a user-facing error when exceeded, avoiding OOM on large videos - Add console.warn in handleVideoError for debugging failed Blob URL loads Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…instead of null for error
…common videoChunksRef
…ng in postMessage
anirudhk-tech
force-pushed
the
feat/dropper-video-extension-v2
branch
from
a0f45f1 to
81b707b
Compare
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
Added onSSE callback parameter to client.sendFiles() to receive real-time SSE events during file processing; useFileProcessing passes an onSSE handler that accumulates incoming video_chunk SSE events (indexed by chunk_index) into a videoChunksRef map, keyed by filename
Before calling parseDropperResults(), enriches uploadResults with assembled Blob URLs: joins the base64 chunks from videoChunksRef, decodes them into a Uint8Array, creates an object URL via URL.createObjectURL, and injects it as a video field with result_types: { video: 'video' } on the matching PIPELINE_RESULT
Extended the dropper UI to handle the new 'video' result type: parseDropperResults in dropperUtils.ts routes video/videos fields into a videoItems array; dropper.types.ts adds 'videos' to TabType and videos: GroupedContent[] to ProcessedResults; a new VideosView component
IMPORTANT NOTE: Only the typescript client has been updated for this PR to support SSE transfer of videos. The issue is about VSCode dropper UI not being able to show videos, which uses the typescript client. The python client not supporting SSE is a separate issue, and is out of scope for this PR.
Type
feature
Testing
./builder testpassesChecklist
Linked Issue
Fixes #633
Summary by CodeRabbit
New Features
Bug Fixes