Daily branch 2025 11 17 #103
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughReplaces inline SVG share icons with Changes
Sequence Diagram(s)sequenceDiagram
participant User as User (UI)
participant Messages as Messages component
participant Dialog as AllFilesDialog
participant Cache as FileUrlCacheContext
participant Convex as Convex action (server)
participant Storage as S3 / External storage
rect rgb(235,245,255)
Note over Messages,Dialog: User opens "View all files"
end
User->>Messages: Click "View all files"
Messages->>Dialog: open(message, fileDetails)
Dialog->>Cache: check cached fileUrls for items
alt missing URLs
Dialog->>Convex: batch-request presigned URLs (fileId/storageId)
Convex-->>Dialog: returns URLs or per-file errors
Dialog->>Cache: store returned URLs
end
Dialog->>User: render file list (isLoadingUrls / per-file availability)
User->>Dialog: click download (single or batch)
alt URL available
Dialog->>Storage: fetch using presigned URL -> stream/zip
else URL missing
Dialog->>Convex: request URL -> then Storage
end
Dialog-->>User: deliver ZIP or per-file errors
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
- Update handleShowAllFiles to accept effectiveFileDetails parameter - Include files with storageId, s3Key, or fileId (not just url) - Pass all file properties (storageId, fileId, s3Key, mediaType) to dialog - Refactor AllFilesDialog to batch fetch URLs on open - Use FileUrlCacheContext to avoid redundant API calls - Add loading state while URLs are being fetched - Improve performance with parallel URL fetching
- Add path normalization to try /home/user/ prefix for relative paths - Try multiple path variations when file not found at first location - Remove debug logging added for testing
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
lib/ai/tools/get-terminal-files.ts (1)
32-100: Per-file multi-path handling and background checks look good; consider minor DX/observability tweaksThe overall control flow per file (try absolute/relative variants, skip when background processes are active, accumulate
lastError, and classify intoprovidedFilesvsblockedFiles) looks correct and is easy to reason about. Nice improvement in user-facing reasons for blocked files.A few small suggestions:
- The tool description still says “Use full file paths” (Lines 10–17, 21–23), but you now support relative paths by trying both
/home/user/...and the raw path. It may be worth updating the description to mention that relative paths are also accepted so the behavior is explicit.- In the
catch (bgCheckError)block (Lines 70–72), you silently swallow any failure frombackgroundProcessTracker.hasActiveProcessesForFiles. Adding at least debug-level logging there would help diagnose misconfigurations or tracker failures without impacting user-facing behavior.- When pushing blocked files due to upload failures (Lines 93–98), you only include the last error; if you start adding more candidate paths or heuristics later, you might consider aggregating the distinct error messages per file (even just joined with
;) to make debugging easier, while still keepingpath: originalPathas you do now.app/components/AllFilesDialog.tsx (3)
33-47: FileItemfileUrlhandling and UI gating look solidPassing a resolved
fileUrland hiding the Download button when it’s missing keeps the row clickable for selection while preventing broken downloads. The early return inhandleDownloadalso guards against accidental calls without a URL.If you expect very large files, consider a small per-item “downloading…” state to avoid double-clicks, but this is optional polish.
Also applies to: 110-121
252-297: Tighten batch download error handling for failed fetchesThe batch ZIP logic correctly prefers pre-fetched URLs (
fileUrls/file.part.url) and falls back to fetching as needed, with per-file try/catch. However, failed HTTP responses (e.g. 403/404) will still be zipped as whateverresponse.blob()returns, without logging.Consider checking
response.okbefore adding the blob to JSZip:- if (url) { - const response = await fetch(url); - const blob = await response.blob(); + if (url) { + const response = await fetch(url); + if (!response.ok) { + console.error( + `Failed to fetch ${file.part.name} for ZIP:`, + response.status, + ); + return; + } + const blob = await response.blob();This keeps corrupted or unauthorized responses out of the archive and improves debuggability.
401-405: GlobalisLoadingUrlsgate is clear and user-friendlyShowing a single “Loading files…” state while URLs are being prefetched is a simple, predictable UX, and the per-row
fileUrlresolution (fileUrls.get(index) || file.part.url || null) lines up with the FileItem behavior.If you later want more granularity, you could render the list while loading and only hide the Download buttons for rows without a resolved URL, but the current approach is perfectly acceptable.
Also applies to: 409-419
app/components/Messages.tsx (1)
449-454: “View all files” click handler wiring looks good (minor refactor opportunity)Passing
messagepluseffectiveFileDetails || []intohandleShowAllFilesaligns with the filtering used to buildsavedFiles, and the early-return in the handler ensures no dialog for empty inputs.If you want to reduce duplicated filtering, you could pass
savedFilesdirectly and lethandleShowAllFilesskip the.filter(...), but this is purely optional.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (5)
app/components/AllFilesDialog.tsx(7 hunks)app/components/ChatHeader.tsx(2 hunks)app/components/Messages.tsx(2 hunks)lib/ai/tools/get-terminal-files.ts(2 hunks)lib/moderation.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- lib/moderation.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- app/components/ChatHeader.tsx
🧰 Additional context used
🧬 Code graph analysis (3)
app/components/Messages.tsx (2)
types/chat.ts (1)
ChatMessage(84-87)types/file.ts (1)
FileDetails(84-91)
lib/ai/tools/get-terminal-files.ts (1)
lib/ai/tools/utils/sandbox-file-uploader.ts (1)
uploadSandboxFileToConvex(42-153)
app/components/AllFilesDialog.tsx (3)
__mocks__/convex-react.ts (2)
useConvex(28-28)useAction(9-9)convex/s3Actions.ts (1)
getFileUrlAction(79-148)app/contexts/FileUrlCacheContext.tsx (1)
useFileUrlCacheContext(28-30)
🔇 Additional comments (2)
lib/ai/tools/get-terminal-files.ts (1)
120-122: Error normalization is good; verify exposure of raw messages is acceptableNormalizing
errorto a string and returningresult: "Error providing files: ..."is helpful, but this will surface the raw error message from lower layers (sandbox, Convex, S3, etc.) to the tool consumer.Please double-check that:
- This
resultstring isn’t shown directly to end users without sanitization, and- Downstream error messages from
uploadSandboxFileToConvexor sandbox APIs can’t contain sensitive details (e.g., internal URLs, stack traces, or secrets).If either of these isn’t guaranteed, consider mapping low-level errors to a more generic user-facing message while still logging the detailed error server-side.
app/components/Messages.tsx (1)
186-210: FileDetails → dialog file mapping is consistent and safe
handleShowAllFilescorrectly guards against emptyfileDetails, filters byurl/storageId/s3Key, and maps into the{ part, partIndex, messageId }shape that AllFilesDialog expects. Usingfile.url ?? undefinedkeeps the part shape clean while still supporting URL-prefetched entries.The empty dependency array is fine here since you only rely on stable state setters.
…log closes Add cancellation flag to prevent in-flight fetchAllUrls promises from calling setFileUrls/setIsLoadingUrls after the dialog closes. The cleanup function sets cancelled=true when the effect unmounts, and fetchAllUrls checks this flag before updating state.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/components/AllFilesDialog.tsx(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/components/AllFilesDialog.tsx (3)
__mocks__/convex-react.ts (2)
useConvex(28-28)useAction(9-9)convex/s3Actions.ts (1)
getFileUrlAction(79-148)app/contexts/FileUrlCacheContext.tsx (1)
useFileUrlCacheContext(28-30)
🔇 Additional comments (4)
app/components/AllFilesDialog.tsx (4)
36-124: LGTM! Clean file-URL integration in FileItem.The
fileUrlprop is properly guarded (line 46) and the download button visibility is correctly tied to URL availability (line 110). Error handling is in place for fetch failures.
149-218: Well done! Cancellation flag correctly implements the suggested fix.The cancellation pattern properly addresses the race condition flagged in the previous review. The cleanup function (lines 215-217) sets the flag, and the state updates (lines 207-210) are guarded, preventing stale updates after the dialog closes.
261-340: LGTM! Robust batch download with good fallback logic.The implementation correctly uses pre-fetched URLs (line 275) and falls back to fetching missing ones (lines 278-290). Per-file error handling (lines 301-303) ensures that individual failures don't break the entire batch. The ZIP filename sanitization (lines 319-322) is a nice touch.
410-428: LGTM! Loading state and URL propagation work correctly.The loading indicator (lines 410-413) provides good UX while URLs are being fetched. The fileUrl derivation (line 418) correctly falls back to
file.part.urlwhen not in the fetched map, and proper propagation to FileItem (line 427) enables the download functionality.
Summary by CodeRabbit
New Features
Refactor
Chores