refactor: align file mentions with sandbox files flow#1664
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 47 minutes and 41 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR replaces Supabase/signed-URL and ListedFileRow-based file mention flows with a sandbox-backed FileNode tree: types change from Changes
Sequence DiagramsequenceDiagram
participant User as User
participant UI as FileMentionsInput
participant Suggestion as SuggestionItem
participant ChatHook as useVercelChat
participant Sandbox as Sandbox API
User->>UI: open mention picker / select file
UI->>Suggestion: render entry (uses entry.path)
Suggestion->>Suggestion: detect image via path/display
alt image
Suggestion->>Sandbox: useSandboxFileContent.query(path, enabled)
Sandbox-->>Suggestion: imageUrl / content
Suggestion-->>UI: render <img> or placeholder
else non-image
Suggestion-->>UI: render placeholder
end
User->>UI: submit message with mentions
UI->>ChatHook: send with mention paths
activate ChatHook
ChatHook->>Sandbox: getFileContents(accessToken, path) for each mention
Sandbox-->>ChatHook: file contents
ChatHook->>ChatHook: derive MIME, build mentionAttachments and mentionTextContext
ChatHook-->>UI: finalize message payload (text + attachments)
deactivate ChatHook
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ❌ 1❌ Failed checks (1 warning)
✏️ 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
No issues found across 3 files
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Requires human review: Refactor involves deleting a local API route, switching to an external API, and changing authentication logic (Bearer tokens), which requires human verification.
![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)
hooks/useFilesManager.ts (1)
37-40: Extract token retrieval into one helper to remove duplication.The same access-token guard appears twice; pulling it into a local helper keeps the hook easier to maintain.
♻️ Suggested DRY refactor
+ async function getRequiredAccessToken() { + const accessToken = await getAccessToken(); + if (!accessToken) throw new Error("Please sign in to view files"); + return accessToken; + } + const { data, isLoading } = useQuery<{ files: Array<ListedFileRow> }>({ @@ - const accessToken = await getAccessToken(); - if (!accessToken) { - throw new Error("Please sign in to view files"); - } + const accessToken = await getRequiredAccessToken(); @@ - const accessToken = await getAccessToken(); - if (!accessToken) { - throw new Error("Please sign in to view files"); - } + const accessToken = await getRequiredAccessToken();As per coding guidelines, "
hooks/**/*.ts: DRY: Extract common hook logic into shared utilities; KISS: Avoid complex hook compositions, prefer simple state management."Also applies to: 66-69
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/useFilesManager.ts` around lines 37 - 40, Extract the duplicated access-token check into a small helper (e.g., ensureAccessToken or getRequiredAccessToken) and use it wherever getAccessToken + throw is repeated in this hook; the helper should call getAccessToken(), throw the same Error("Please sign in to view files") when falsy, and return the token otherwise so callers (the functions currently containing the duplicated blocks around getAccessToken) simply call the helper and proceed. Ensure the helper is defined near the top of useFilesManager.ts (or moved to a shared hook util if used elsewhere) and replace both occurrences (the blocks around getAccessToken at the current duplicated locations) with a single call to the new helper.lib/files/fetchFiles.ts (1)
53-53: Add a minimal runtime response-shape guard before returning JSON.
response.json()is trusted asFetchFilesResponsewithout validation; malformed payloads will fail downstream with less actionable errors.♻️ Suggested hardening
- return response.json(); + const payload: unknown = await response.json(); + if ( + !payload || + typeof payload !== "object" || + !Array.isArray((payload as { files?: unknown }).files) + ) { + throw new Error("Invalid response shape from /api/files"); + } + return payload as FetchFilesResponse;As per coding guidelines, "
lib/**/*.ts: For utility functions, ensure: Proper error handling; Use TypeScript for type safety; KISS: Prefer simple, readable implementations."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/files/fetchFiles.ts` at line 53, The call that returns response.json() should first parse and minimally validate the runtime shape against the expected FetchFilesResponse before trusting it; update the fetchFiles function (the place that does return response.json()) to await and store const payload = await response.json(), check simple invariants (e.g., payload is an object/array and contains the expected keys or types used downstream — reference the FetchFilesResponse shape), and if validation fails throw a clear Error describing the unexpected response (including response.status or a short payload snippet) so callers don't receive malformed data.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@hooks/useFilesManager.ts`:
- Around line 63-65: The prefetchQuery call in qc.prefetchQuery uses a queryKey
of ["files", ownerAccountId, artistAccountId, childPath] but the main query
includes the `recursive` flag, so add `recursive` to the queryKey to match cache
keys; update both qc.prefetchQuery occurrences (the ones referencing
ownerAccountId, artistAccountId, childPath) to use ["files", ownerAccountId,
artistAccountId, childPath, recursive] so prefetched results are reusable by the
main query.
In `@lib/files/fetchFiles.ts`:
- Around line 28-46: The client is calling GET /api/files from
lib/files/fetchFiles.ts but there is no server handler; add a new route handler
file that implements the root GET endpoint (app/api/files/route.ts) which reads
the query params artist_account_id, path, and recursive, validates the
Authorization header, fetches and returns the file listing as JSON (proper
200/401/4xx error responses), and matches the shape expected by fetchFiles.ts;
ensure the handler exports an async GET function that parses req.nextUrl or
Request.query, performs authorization and error handling, and returns a JSON
Response with the file list.
---
Nitpick comments:
In `@hooks/useFilesManager.ts`:
- Around line 37-40: Extract the duplicated access-token check into a small
helper (e.g., ensureAccessToken or getRequiredAccessToken) and use it wherever
getAccessToken + throw is repeated in this hook; the helper should call
getAccessToken(), throw the same Error("Please sign in to view files") when
falsy, and return the token otherwise so callers (the functions currently
containing the duplicated blocks around getAccessToken) simply call the helper
and proceed. Ensure the helper is defined near the top of useFilesManager.ts (or
moved to a shared hook util if used elsewhere) and replace both occurrences (the
blocks around getAccessToken at the current duplicated locations) with a single
call to the new helper.
In `@lib/files/fetchFiles.ts`:
- Line 53: The call that returns response.json() should first parse and
minimally validate the runtime shape against the expected FetchFilesResponse
before trusting it; update the fetchFiles function (the place that does return
response.json()) to await and store const payload = await response.json(), check
simple invariants (e.g., payload is an object/array and contains the expected
keys or types used downstream — reference the FetchFilesResponse shape), and if
validation fails throw a clear Error describing the unexpected response
(including response.status or a short payload snippet) so callers don't receive
malformed data.
🪄 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: 765bf83b-5dfe-4dfd-b130-e1c674e032f0
📒 Files selected for processing (4)
app/api/files/list/route.tshooks/useArtistFilesForMentions.tshooks/useFilesManager.tslib/files/fetchFiles.ts
💤 Files with no reviewable changes (1)
- app/api/files/list/route.ts
Change Request@arpitgupta1214 — after reviewing this PR and the related docs PR (recoupable/docs#122), I think we should take a different approach here. Problem with the current approachThe What we already haveThe Files page ( Requested changeInstead of migrating to a new
Let me know if you have questions or see any reason the sandbox data wouldn't work for the |
There was a problem hiding this comment.
2 issues found across 4 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="hooks/useFilesManager.ts">
<violation number="1">
P1: Prefetch sends the full storage path (e.g. `files/owner123/artist456/subfolder/`) as the `path` query param, but the main query strips the `files/{owner}/{artist}/` prefix first. The old relative-path computation was removed, so every prefetch request sends the wrong path to the API, making all prefetched results incorrect (likely empty).</violation>
</file>
<file name="app/api/files/list/route.ts">
<violation number="1">
P2: Custom agent: **Flag AI Slop and Fabricated Changes**
Narrating comment restates the next line. `listFilesByArtist` is self-documenting; remove the comment or replace it with *why* this specific helper is appropriate (e.g., the authorization/filtering guarantee the caller relies on).</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
There was a problem hiding this comment.
2 issues found across 6 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="hooks/useVercelChat.ts">
<violation number="1" location="hooks/useVercelChat.ts:111">
P1: Removing the signed-URL cache without a replacement causes `getFileContents` to fire a network request for every mentioned file **on every keystroke** while a mention is present in the input. `selectedFileIds` is a new array reference whenever `input` changes (even if the actual IDs are identical), which re-triggers this effect.
Consider adding a cache (e.g. a `useRef<Map>` keyed by file path) or stabilising `selectedFileIds` so the effect only re-runs when the set of IDs actually changes.</violation>
</file>
<file name="hooks/useBatchSignedUrls.ts">
<violation number="1" location="hooks/useBatchSignedUrls.ts:6">
P2: This hook is dead code — it's exported but never imported anywhere in the codebase. Per the project's YAGNI principle ("delete unused code; don't anticipate needs"), delete the file entirely rather than maintaining a no-op stub. The `void suggestions` trick to silence the unused-param warning is a clear signal the function serves no purpose.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
| throw new Error("Please sign in to attach mentioned files"); | ||
| } | ||
|
|
||
| const results = await Promise.all( |
There was a problem hiding this comment.
P1: Removing the signed-URL cache without a replacement causes getFileContents to fire a network request for every mentioned file on every keystroke while a mention is present in the input. selectedFileIds is a new array reference whenever input changes (even if the actual IDs are identical), which re-triggers this effect.
Consider adding a cache (e.g. a useRef<Map> keyed by file path) or stabilising selectedFileIds so the effect only re-runs when the set of IDs actually changes.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At hooks/useVercelChat.ts, line 111:
<comment>Removing the signed-URL cache without a replacement causes `getFileContents` to fire a network request for every mentioned file **on every keystroke** while a mention is present in the input. `selectedFileIds` is a new array reference whenever `input` changes (even if the actual IDs are identical), which re-triggers this effect.
Consider adding a cache (e.g. a `useRef<Map>` keyed by file path) or stabilising `selectedFileIds` so the effect only re-runs when the set of IDs actually changes.</comment>
<file context>
@@ -78,91 +76,90 @@ export function useVercelChat({
+ throw new Error("Please sign in to attach mentioned files");
+ }
+
+ const results = await Promise.all(
+ selected.map(async (f) => {
+ const fileResult = await getFileContents(accessToken, f.path);
</file context>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
hooks/useVercelChat.ts (3)
88-89: Simplify state updates — the conditional checks add noise without benefit.The pattern
setX((prev) => (prev.length ? [] : prev))prevents an update only when state is already falsy/empty. React already bails out on identical state values for primitive comparisons, but for arrays/strings this micro-optimization adds cognitive overhead. Consider straightforward updates since the effect already guards against unnecessary runs.♻️ Cleaner state resets
if (!selectedFileIds.length) { - if (!cancelled) setMentionAttachments((prev) => (prev.length ? [] : prev)); - if (!cancelled) setMentionTextContext((prev) => (prev ? "" : prev)); + if (!cancelled) setMentionAttachments([]); + if (!cancelled) setMentionTextContext(""); if (!cancelled) setIsLoadingSignedUrls((prev) => (prev ? false : prev)); return; }Also applies to: 97-98, 147-149, 153-154
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/useVercelChat.ts` around lines 88 - 89, Remove the unnecessary prev-based conditional updates and directly reset the state inside the cancelled guard: where you currently call setMentionAttachments((prev) => (prev.length ? [] : prev)) and setMentionTextContext((prev) => (prev ? "" : prev)) (and the similar occurrences at the other locations referenced), replace them with direct setters setMentionAttachments([]) and setMentionTextContext("") while keeping the existing if (!cancelled) guards; this simplifies the logic and relies on React's state equality behavior to avoid redundant re-renders.
111-122: Consider handling partial failures when fetching multiple files.
Promise.allwill reject entirely if any single file fetch fails, leaving the user with no attachments. UsingPromise.allSettledwould allow graceful degradation — successfully fetched files can still be attached while failures are logged.♻️ Resilient file fetching with partial success
- const results = await Promise.all( - selected.map(async (f) => { - const fileResult = await getFileContents(accessToken, f.path); - const mimeType = f.mime_type || getMimeFromPath(f.path); - return { - path: f.path, - mimeType, - content: fileResult.content, - dataUrl: fileResult.imageUrl, - }; - }), - ); + const settled = await Promise.allSettled( + selected.map(async (f) => { + const fileResult = await getFileContents(accessToken, f.path); + const mimeType = f.mime_type || getMimeFromPath(f.path); + return { + path: f.path, + mimeType, + content: fileResult.content, + dataUrl: fileResult.imageUrl, + }; + }), + ); + + const results = settled + .filter((r): r is PromiseFulfilledResult<typeof r extends PromiseFulfilledResult<infer T> ? T : never> => r.status === "fulfilled") + .map((r) => r.value); + + const failures = settled.filter((r) => r.status === "rejected"); + if (failures.length > 0) { + console.warn(`Failed to fetch ${failures.length} mentioned file(s)`); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/useVercelChat.ts` around lines 111 - 122, The current batch fetch uses Promise.all which fails entirely if any getFileContents call rejects; change the selected map call in useVercelChat.ts to use Promise.allSettled over the array of getFileContents promises, then filter the settled results for status === "fulfilled" and build the results array from result.value (ensuring you still compute mimeType via getMimeFromPath and include content and dataUrl), while logging or reporting any entries with status === "rejected" (include f.path and the error) so partial successes are preserved and failures are visible.
39-380: Hook is approaching complexity threshold — consider extraction.At ~340 lines with multiple concerns (message loading, file mention handling, form submission, model selection), this hook could benefit from extracting the mention-file resolution logic into a dedicated
useMentionAttachmentshook. This would improve testability and align with the single-responsibility guideline for hooks.As per coding guidelines: "Single responsibility per hook" and "Extract custom hooks for complex logic".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/useVercelChat.ts` around lines 39 - 380, The hook is doing too much — extract the mention-file resolution logic (selectedFileIds computation, the useEffect that fetches getFileContents/getMimeFromPath, and the state vars mentionAttachments, mentionTextContext, isLoadingSignedUrls) into a new custom hook (e.g., useMentionAttachments) that accepts input, allArtistFiles and getAccessToken and returns { mentionAttachments, mentionTextContext, isLoadingSignedUrls }; move helper logic around building results and error handling into that hook and keep useVercelChat only responsible for chat flow (replace the local selectedFileIds, setMentionAttachments, setMentionTextContext, setIsLoadingSignedUrls and the long useEffect with a call to useMentionAttachments and update imports/usages accordingly).components/VercelChat/SuggestionItem.tsx (1)
23-39: Consider adding accessibility attributes for screen reader support.This element is interactive (cursor-pointer, part of a selectable list). Adding
role="option"andaria-selected={focused}would improve screen reader compatibility within the suggestion dropdown.♿ Accessibility enhancement
<div + role="option" + aria-selected={focused} className={cn( "px-3 py-2 text-[13px] cursor-pointer select-none", "flex items-center gap-2 rounded-md", focused ? "bg-accent text-foreground" : "text-muted-foreground hover:bg-accent/50" )} >As per coding guidelines: "Provide proper ARIA roles/states".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/VercelChat/SuggestionItem.tsx` around lines 23 - 39, The outer interactive div in SuggestionItem.tsx (the returned element in the component that uses focused, isImage, ImageIcon, highlightedDisplay, and entry.display) needs ARIA attributes for screen readers: add role="option" and set aria-selected equal to the focused boolean (aria-selected={focused}) on that same div so assistive tech can treat it as a selectable list option; ensure these attributes are applied where the className and focus logic currently live.hooks/useFileMentionSuggestions.ts (1)
26-37: Consider removing the type assertion.The
as GroupedSuggestionassertion on line 37 can be eliminated by ensuring the mapped object satisfies the interface directly. This improves type safety and makes the compiler catch mismatches.♻️ Suggested improvement
- return { - id: f.id, - display: name, - group, - mime_type: f.mime_type, - path: f.path, - } as GroupedSuggestion; + const suggestion: GroupedSuggestion = { + id: f.id, + display: name, + group, + mime_type: f.mime_type, + path: f.path, + }; + return suggestion;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/useFileMentionSuggestions.ts` around lines 26 - 37, Remove the "as GroupedSuggestion" type assertion on the mapped return and make the object conform to the GroupedSuggestion type explicitly: annotate the map callback return type (or set the array.map generic) so the compiler enforces fields, ensure the returned object from the map includes all required GroupedSuggestion properties (e.g., id, display, group, mime_type, path) with correct names and types, and adjust the computed variables (rel, lastSlash, group, name) if needed to match expected types; this will allow you to delete the trailing type assertion in useFileMentionSuggestions.ts.hooks/useBatchSignedUrls.ts (1)
5-9: Delete this hook entirely — it serves no purpose and is unused elsewhere.The hook returns an empty object and ignores all inputs. No callers reference it in the codebase, so removing it eliminates dead code without breaking anything. This aligns with clean code principles and reduces maintenance burden.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/useBatchSignedUrls.ts` around lines 5 - 9, Remove the dead hook useBatchSignedUrls (exported function useBatchSignedUrls and its file hooks/useBatchSignedUrls.ts) since it always returns an empty Record, ignores its GroupedSuggestion[] parameter, and has no callers; delete the file and any associated export from index barrels if present to eliminate unused code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@hooks/useVercelChat.ts`:
- Line 162: The effect that fetches file contents (the useEffect covering lines
~84-162) is re-running because the Privy getAccessToken function reference is
unstable; update the code so the effect no longer depends on a non-memoized
function reference: either wrap getAccessToken with useCallback in the parent
(so the getAccessToken reference passed into useVercelChat is stable) or change
the effect to fetch the token once outside the effect (or store the token in a
ref/state) and replace getAccessToken from the dependency array with the stable
token/ref; target the dependency list that currently uses [selectedFileIds,
allArtistFiles, getAccessToken] and ensure only stable values remain so
fetchFiles logic (inside the effect) does not run every render.
---
Nitpick comments:
In `@components/VercelChat/SuggestionItem.tsx`:
- Around line 23-39: The outer interactive div in SuggestionItem.tsx (the
returned element in the component that uses focused, isImage, ImageIcon,
highlightedDisplay, and entry.display) needs ARIA attributes for screen readers:
add role="option" and set aria-selected equal to the focused boolean
(aria-selected={focused}) on that same div so assistive tech can treat it as a
selectable list option; ensure these attributes are applied where the className
and focus logic currently live.
In `@hooks/useBatchSignedUrls.ts`:
- Around line 5-9: Remove the dead hook useBatchSignedUrls (exported function
useBatchSignedUrls and its file hooks/useBatchSignedUrls.ts) since it always
returns an empty Record, ignores its GroupedSuggestion[] parameter, and has no
callers; delete the file and any associated export from index barrels if present
to eliminate unused code.
In `@hooks/useFileMentionSuggestions.ts`:
- Around line 26-37: Remove the "as GroupedSuggestion" type assertion on the
mapped return and make the object conform to the GroupedSuggestion type
explicitly: annotate the map callback return type (or set the array.map generic)
so the compiler enforces fields, ensure the returned object from the map
includes all required GroupedSuggestion properties (e.g., id, display, group,
mime_type, path) with correct names and types, and adjust the computed variables
(rel, lastSlash, group, name) if needed to match expected types; this will allow
you to delete the trailing type assertion in useFileMentionSuggestions.ts.
In `@hooks/useVercelChat.ts`:
- Around line 88-89: Remove the unnecessary prev-based conditional updates and
directly reset the state inside the cancelled guard: where you currently call
setMentionAttachments((prev) => (prev.length ? [] : prev)) and
setMentionTextContext((prev) => (prev ? "" : prev)) (and the similar occurrences
at the other locations referenced), replace them with direct setters
setMentionAttachments([]) and setMentionTextContext("") while keeping the
existing if (!cancelled) guards; this simplifies the logic and relies on React's
state equality behavior to avoid redundant re-renders.
- Around line 111-122: The current batch fetch uses Promise.all which fails
entirely if any getFileContents call rejects; change the selected map call in
useVercelChat.ts to use Promise.allSettled over the array of getFileContents
promises, then filter the settled results for status === "fulfilled" and build
the results array from result.value (ensuring you still compute mimeType via
getMimeFromPath and include content and dataUrl), while logging or reporting any
entries with status === "rejected" (include f.path and the error) so partial
successes are preserved and failures are visible.
- Around line 39-380: The hook is doing too much — extract the mention-file
resolution logic (selectedFileIds computation, the useEffect that fetches
getFileContents/getMimeFromPath, and the state vars mentionAttachments,
mentionTextContext, isLoadingSignedUrls) into a new custom hook (e.g.,
useMentionAttachments) that accepts input, allArtistFiles and getAccessToken and
returns { mentionAttachments, mentionTextContext, isLoadingSignedUrls }; move
helper logic around building results and error handling into that hook and keep
useVercelChat only responsible for chat flow (replace the local selectedFileIds,
setMentionAttachments, setMentionTextContext, setIsLoadingSignedUrls and the
long useEffect with a call to useMentionAttachments and update imports/usages
accordingly).
🪄 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: 419f9739-39c7-42a8-b1c8-22d952b9d43e
📒 Files selected for processing (6)
components/VercelChat/FileMentionsInput.tsxcomponents/VercelChat/SuggestionItem.tsxhooks/useArtistFilesForMentions.tshooks/useBatchSignedUrls.tshooks/useFileMentionSuggestions.tshooks/useVercelChat.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- hooks/useArtistFilesForMentions.ts
| } | ||
| return outputs; | ||
| }, [knowledgeFiles]); | ||
| }, [selectedFileIds, allArtistFiles, getAccessToken]); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does usePrivy getAccessToken return a stable function reference across renders?
💡 Result:
No, usePrivy getAccessToken does not return a stable function reference across renders. In standard React custom hooks like usePrivy, methods such as getAccessToken are typically recreated on each render unless explicitly memoized with useCallback, which official Privy documentation does not indicate. Official docs show usage as const { getAccessToken } = usePrivy; without stability guarantees, and general React behavior for hook returns supports new references per render (sources 1,2,5). To use stably, wrap in useCallback: const stableGetAccessToken = useCallback(getAccessToken, [getAccessToken]); but native return changes reference.
Citations:
- 1: https://docs.privy.io/guide/react/authorization
- 2: https://docs.privy.io/authentication/user-authentication/access-tokens
- 3: https://medium.com/@julianaguero/understanding-function-references-in-react-a-deep-dive-into-dependencies-and-render-behavior-a599fa87dd14
- 4: https://www.npmjs.com/package/@privy-io/react-auth
🏁 Script executed:
find . -name "useVercelChat.ts" -type fRepository: recoupable/chat
Length of output: 83
🏁 Script executed:
cat -n ./hooks/useVercelChat.ts | head -180 | tail -50Repository: recoupable/chat
Length of output: 1959
🏁 Script executed:
cat -n ./hooks/useVercelChat.ts | head -130 | tail -50Repository: recoupable/chat
Length of output: 2286
Fix unnecessary effect re-runs caused by unstable getAccessToken reference.
The effect that fetches file contents (lines 84-162) will re-run on every render because getAccessToken from Privy is recreated on each render without explicit memoization. This triggers unnecessary async file fetches. Wrap getAccessToken with useCallback in the parent component, or restructure the dependency to avoid passing the function reference directly. Consider extracting the token fetching logic outside the effect or using a ref to cache the last token to prevent excessive API calls on every render.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@hooks/useVercelChat.ts` at line 162, The effect that fetches file contents
(the useEffect covering lines ~84-162) is re-running because the Privy
getAccessToken function reference is unstable; update the code so the effect no
longer depends on a non-memoized function reference: either wrap getAccessToken
with useCallback in the parent (so the getAccessToken reference passed into
useVercelChat is stable) or change the effect to fetch the token once outside
the effect (or store the token in a ref/state) and replace getAccessToken from
the dependency array with the stable token/ref; target the dependency list that
currently uses [selectedFileIds, allArtistFiles, getAccessToken] and ensure only
stable values remain so fetchFiles logic (inside the effect) does not run every
render.
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="hooks/useMentionImagePreviews.ts">
<violation number="1" location="hooks/useMentionImagePreviews.ts:82">
P2: Including `previewUrls` in the dependency array while also calling `setPreviewUrls` inside the effect creates an unnecessary re-execution cycle after every batch fetch. Use a `useRef<Set<string>>` to track already-fetched paths instead, and remove `previewUrls` from the dependency array. This avoids the wasted re-run and the redundant `getAccessToken()` call it triggers.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="components/VercelChat/SuggestionItem.tsx">
<violation number="1" location="components/VercelChat/SuggestionItem.tsx:16">
P2: Custom agent: **Code Structure and Size Limits for Readability and Single Responsibility**
File is 104 lines (limit: 100). Extract the image preview loading logic (cache maps, in-flight deduplication, and the `useEffect`) into a dedicated `useImagePreview(path, isImage)` hook to keep this component focused on rendering.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
arpitgupta1214
changed the title
|
You're iterating quickly on this pull request. To help protect your rate limits, cubic has paused automatic reviews on new pushes for now—when you're ready for another review, comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
hooks/useSandboxFileContent.ts (1)
24-28: State initialization pattern works but may cause confusion.The
useStateinitializer on line 24-26 only captures the initial value ofoptions?.path. If a caller later changesoptions.path, the internalselectedPathwon't sync. This is technically fine becauseeffectivePath(line 27) prioritizesoptions.pathoverselectedPath, making the controlled path always win.However, this creates a subtle contract: callers using the
select()method and callers passingoptions.pathare effectively two different usage patterns that don't interact cleanly. Consider documenting this distinction or simplifying by removing the initial state assignment fromoptions?.pathsinceeffectivePathhandles the fallback anyway.🔧 Optional: Simplify state initialization
- const [selectedPath, setSelectedPath] = useState<string | undefined>( - options?.path, - ); + const [selectedPath, setSelectedPath] = useState<string | undefined>();This makes the two modes clearer: controlled via
options.pathOR uncontrolled viaselect().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/useSandboxFileContent.ts` around lines 24 - 28, The state initializer should not capture options?.path to avoid the mixed controlled/uncontrolled contract: change the useState for selectedPath to start as undefined (useState<string | undefined>(undefined)) and keep effectivePath = options?.path ?? selectedPath so callers either control via options.path or use select() which calls setSelectedPath; update any references to selectedPath/setSelectedPath and the select() method to rely on this uncontrolled initial state and adjust docs/tests to state the two exclusive usage modes.hooks/useArtistFilesForMentions.ts (1)
10-17: Consider documenting or removingis_directory.The
is_directoryfield is alwaysfalsesince the flatten logic explicitly filters out folders (line 25-27). If folder mentions are never intended, this field adds noise to the type. If they might be supported later, a brief comment would clarify the current constraint.Not blocking—the implementation is correct.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/useArtistFilesForMentions.ts` around lines 10 - 17, The MentionableFile type includes is_directory but the flatten logic that filters out folders ensures it's always false; either remove is_directory from the MentionableFile type to avoid dead/noisy fields or keep it but add a brief comment on the type (next to is_directory) stating that folder entries are filtered out by the flatten logic and is_directory will always be false; update any consumers of MentionableFile accordingly (search for MentionableFile and the flatten/filter implementation that excludes directories).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/VercelChat/SuggestionItem.tsx`:
- Around line 19-25: Compute an explicit effectivePath (e.g., entry.path ||
entry.display || "") and use that when calling useSandboxFileContent instead of
passing entry.path directly; also make the enabled flag depend on both isImage
and Boolean(effectivePath) so the hook is only invoked when there is a real
path. This touches the isImage computation and the useSandboxFileContent
call—ensure you pass effectivePath to useSandboxFileContent({ path:
effectivePath, enabled: isImage && Boolean(effectivePath) }).
---
Nitpick comments:
In `@hooks/useArtistFilesForMentions.ts`:
- Around line 10-17: The MentionableFile type includes is_directory but the
flatten logic that filters out folders ensures it's always false; either remove
is_directory from the MentionableFile type to avoid dead/noisy fields or keep it
but add a brief comment on the type (next to is_directory) stating that folder
entries are filtered out by the flatten logic and is_directory will always be
false; update any consumers of MentionableFile accordingly (search for
MentionableFile and the flatten/filter implementation that excludes
directories).
In `@hooks/useSandboxFileContent.ts`:
- Around line 24-28: The state initializer should not capture options?.path to
avoid the mixed controlled/uncontrolled contract: change the useState for
selectedPath to start as undefined (useState<string | undefined>(undefined)) and
keep effectivePath = options?.path ?? selectedPath so callers either control via
options.path or use select() which calls setSelectedPath; update any references
to selectedPath/setSelectedPath and the select() method to rely on this
uncontrolled initial state and adjust docs/tests to state the two exclusive
usage modes.
🪄 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: 146249f3-afb8-47c5-adb5-07a2092d36e5
📒 Files selected for processing (6)
app/api/files/list/route.tscomponents/VercelChat/SuggestionItem.tsxhooks/useArtistFilesForMentions.tshooks/useBatchSignedUrls.tshooks/useFilesManager.tshooks/useSandboxFileContent.ts
💤 Files with no reviewable changes (3)
- hooks/useBatchSignedUrls.ts
- app/api/files/list/route.ts
- hooks/useFilesManager.ts
| const isImage = | ||
| entry.mime_type?.startsWith("image/") || | ||
| /\.(png|jpg|jpeg|gif|webp|svg)$/i.test(entry.path || entry.display || ""); | ||
| const { imageUrl } = useSandboxFileContent({ | ||
| path: entry.path, | ||
| enabled: isImage, | ||
| }); |
There was a problem hiding this comment.
Potential undefined path passed to hook.
The regex fallback on line 21 suggests entry.path might be undefined (entry.path || entry.display || ""), yet line 23 passes entry.path directly to the hook without a fallback. If path is undefined and isImage is true (matched via regex on display), the hook will receive undefined, causing the query to be disabled anyway (due to the Boolean(enabled && effectivePath) check in the hook).
This works but is fragile—the safety depends on the hook's internal guard rather than explicit intent here.
🛡️ Suggested defensive approach
- const isImage =
- entry.mime_type?.startsWith("image/") ||
- /\.(png|jpg|jpeg|gif|webp|svg)$/i.test(entry.path || entry.display || "");
- const { imageUrl } = useSandboxFileContent({
- path: entry.path,
- enabled: isImage,
- });
+ const pathForContent = entry.path;
+ const isImage =
+ entry.mime_type?.startsWith("image/") ||
+ /\.(png|jpg|jpeg|gif|webp|svg)$/i.test(pathForContent || entry.display || "");
+ const { imageUrl } = useSandboxFileContent({
+ path: pathForContent,
+ enabled: isImage && Boolean(pathForContent),
+ });This makes the enabled condition explicitly guard against undefined paths at the call site.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/VercelChat/SuggestionItem.tsx` around lines 19 - 25, Compute an
explicit effectivePath (e.g., entry.path || entry.display || "") and use that
when calling useSandboxFileContent instead of passing entry.path directly; also
make the enabled flag depend on both isImage and Boolean(effectivePath) so the
hook is only invoked when there is a real path. This touches the isImage
computation and the useSandboxFileContent call—ensure you pass effectivePath to
useSandboxFileContent({ path: effectivePath, enabled: isImage &&
Boolean(effectivePath) }).
| import getMimeFromPath from "@/lib/files/getMimeFromPath"; | ||
| import type { FileNode } from "@/lib/sandboxes/parseFileTree"; | ||
|
|
||
| const WORKSPACE_ORGS_PREFIX = ".openclaw/workspace/orgs/"; |
There was a problem hiding this comment.
DRY principle
- actual: this prefix is duplicated across
/filespage andfile mentionsin the chat. - required: shared const imported across both usages.
The .openclaw/workspace/orgs prefix was duplicated in useArtistFilesForMentions (file-mention picker) and getSubtreeAtPath (/files page filetree). Share a single const so both stay in lockstep if the sandbox layout changes. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
| * stay in lockstep if the sandbox layout changes. | ||
| */ | ||
| export const WORKSPACE_ORGS_PATH = ".openclaw/workspace/orgs"; | ||
| export const WORKSPACE_ORGS_PREFIX = `${WORKSPACE_ORGS_PATH}/`; |
There was a problem hiding this comment.
DRY
- actual: exporting WORKSPACE_ORGS_PATH and WORKSPACE_ORGS_PREFIX
- required: only export WORKSPACE_ORGS_PATH. If trailing
/is needed, update the caller to include it.
Drop the WORKSPACE_ORGS_PREFIX export. The only caller that needs the trailing `/` constructs it locally, keeping the shared module minimal. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@hooks/useArtistFilesForMentions.ts`:
- Around line 34-42: The mention IDs currently use raw file paths in
useArtistFilesForMentions (id: path), which breaks the @[label](id) parser for
names with characters like ')'; change the id to an encoded opaque value (e.g.,
encodeURIComponent(path) or another reversible encoder) when building the file
objects in useArtistFilesForMentions, and update the mention resolver in
useVercelChat to decode the incoming mention id (e.g., decodeURIComponent from
the parser match) before looking up files so filenames like "Q1 (final).pdf"
resolve correctly.
In `@hooks/useVercelChat.ts`:
- Around line 108-146: The current flow eagerly fetches full contents via
getFileContents for every item in selected and may inline huge text or base64
blobs; update the logic in the selected.map / Promise.all block to first check
file size (use f.size if available or call a lightweight metadata
endpoint/getFileMetadata) and skip or switch to a server-side reference/summary
for files over a safe threshold (e.g., configurable MAX_INLINE_SIZE), only
calling getFileContents for files below that threshold; when skipping, ensure
setMentionAttachments and setMentionTextContext receive either a placeholder
reference (e.g., "[File omitted due to size: path]") or a server-side
URL/summary instead of inlining full content.
- Around line 91-98: The current block only aborts when no ids resolve; change
it to treat any partial resolution as failure by comparing selected.length
against selectedFileIds.length (i.e., if selected.length !==
selectedFileIds.length) and bail out in that case; preserve the existing
cancelled checks and the same cleanup calls to setMentionAttachments,
setMentionTextContext, and setIsLoadingSignedUrls, and ensure you derive
selection by resolving ids in mention order (use selectedFileIds order rather
than unordered Set filtering) so that any mismatch triggers the early return.
🪄 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: 038d2c09-1006-44de-a53b-64196780fd56
📒 Files selected for processing (5)
components/VercelChat/FileMentionsInput.tsxhooks/useArtistFilesForMentions.tshooks/useVercelChat.tslib/sandboxes/getSubtreeAtPath.tslib/sandboxes/workspaceOrgsPath.ts
✅ Files skipped from review due to trivial changes (1)
- lib/sandboxes/workspaceOrgsPath.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- components/VercelChat/FileMentionsInput.tsx
| return [ | ||
| { | ||
| id: path, | ||
| file_name: fileName, | ||
| path, | ||
| mime_type: getMimeFromPath(path), | ||
| is_directory: false, | ||
| relative_path: relativePath, | ||
| }, |
There was a problem hiding this comment.
Avoid raw paths as mention IDs.
id: path is not safe with the current @[label](id) parser in hooks/useVercelChat.ts; a filename like Q1 (final).pdf gets truncated at the first ), so the mention stops resolving. Use an encoded or opaque id here and decode it when resolving mentions.
Possible direction
- id: path,
+ id: encodeURIComponent(path),// hooks/useVercelChat.ts
if (match[1]) ids.add(decodeURIComponent(match[1]));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@hooks/useArtistFilesForMentions.ts` around lines 34 - 42, The mention IDs
currently use raw file paths in useArtistFilesForMentions (id: path), which
breaks the @[label](id) parser for names with characters like ')'; change the id
to an encoded opaque value (e.g., encodeURIComponent(path) or another reversible
encoder) when building the file objects in useArtistFilesForMentions, and update
the mention resolver in useVercelChat to decode the incoming mention id (e.g.,
decodeURIComponent from the parser match) before looking up files so filenames
like "Q1 (final).pdf" resolve correctly.
| const idSet = new Set(selectedFileIds); | ||
| const selected = allArtistFiles.filter((f) => idSet.has(f.id)); | ||
| if (selected.length === 0) { | ||
| if (!cancelled) setKnowledgeFiles((prev) => (prev.length ? [] : prev)); | ||
| if (!cancelled) setMentionAttachments((prev) => (prev.length ? [] : prev)); | ||
| if (!cancelled) setMentionTextContext((prev) => (prev ? "" : prev)); | ||
| if (!cancelled) setIsLoadingSignedUrls((prev) => (prev ? false : prev)); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Abort on partial mention resolution.
This only bails when zero ids resolve. If one mentioned file is stale or malformed, the hook still sends a partial file/text set, which makes the prompt diverge from what the composer shows. Resolve ids in mention order and treat any selected.length !== selectedFileIds.length case as a failure.
Suggested fix
- const idSet = new Set(selectedFileIds);
- const selected = allArtistFiles.filter((f) => idSet.has(f.id));
- if (selected.length === 0) {
+ const filesById = new Map(allArtistFiles.map((file) => [file.id, file]));
+ const selected = selectedFileIds.flatMap((id) => {
+ const file = filesById.get(id);
+ return file ? [file] : [];
+ });
+ if (selected.length !== selectedFileIds.length) {📝 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.
| const idSet = new Set(selectedFileIds); | |
| const selected = allArtistFiles.filter((f) => idSet.has(f.id)); | |
| if (selected.length === 0) { | |
| if (!cancelled) setKnowledgeFiles((prev) => (prev.length ? [] : prev)); | |
| if (!cancelled) setMentionAttachments((prev) => (prev.length ? [] : prev)); | |
| if (!cancelled) setMentionTextContext((prev) => (prev ? "" : prev)); | |
| if (!cancelled) setIsLoadingSignedUrls((prev) => (prev ? false : prev)); | |
| return; | |
| } | |
| const filesById = new Map(allArtistFiles.map((file) => [file.id, file])); | |
| const selected = selectedFileIds.flatMap((id) => { | |
| const file = filesById.get(id); | |
| return file ? [file] : []; | |
| }); | |
| if (selected.length !== selectedFileIds.length) { | |
| if (!cancelled) setMentionAttachments((prev) => (prev.length ? [] : prev)); | |
| if (!cancelled) setMentionTextContext((prev) => (prev ? "" : prev)); | |
| if (!cancelled) setIsLoadingSignedUrls((prev) => (prev ? false : prev)); | |
| return; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@hooks/useVercelChat.ts` around lines 91 - 98, The current block only aborts
when no ids resolve; change it to treat any partial resolution as failure by
comparing selected.length against selectedFileIds.length (i.e., if
selected.length !== selectedFileIds.length) and bail out in that case; preserve
the existing cancelled checks and the same cleanup calls to
setMentionAttachments, setMentionTextContext, and setIsLoadingSignedUrls, and
ensure you derive selection by resolving ids in mention order (use
selectedFileIds order rather than unordered Set filtering) so that any mismatch
triggers the early return.
| const results = await Promise.all( | ||
| selected.map(async (f) => { | ||
| const fileResult = await getFileContents(accessToken, f.path); | ||
| const mimeType = f.mime_type || getMimeFromPath(f.path); | ||
| return { | ||
| path: f.path, | ||
| mimeType, | ||
| content: fileResult.content, | ||
| dataUrl: fileResult.imageUrl, | ||
| }; | ||
| cache.set(f.storage_key, entry); | ||
| }), | ||
| ); | ||
|
|
||
| // Compose final entries in the order of selection | ||
| const entries = selected | ||
| .map((f) => signedUrlCacheRef.current.get(f.storage_key)) | ||
| .filter((e): e is KnowledgeBaseEntry => Boolean(e)); | ||
| const nextAttachments: FileUIPart[] = []; | ||
| const textBlocks: string[] = []; | ||
| for (const result of results) { | ||
| if ( | ||
| result.dataUrl && | ||
| (result.mimeType === "application/pdf" || | ||
| result.mimeType.startsWith("image/")) | ||
| ) { | ||
| nextAttachments.push({ | ||
| type: "file", | ||
| url: result.dataUrl, | ||
| mediaType: result.mimeType, | ||
| }); | ||
| continue; | ||
| } | ||
|
|
||
| if (result.content) { | ||
| textBlocks.push( | ||
| `[Mentioned File: ${result.path}]\n${result.content}`, | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| if (!cancelled) setKnowledgeFiles(entries); | ||
| if (!cancelled) setMentionAttachments(nextAttachments); | ||
| if (!cancelled) | ||
| setMentionTextContext(textBlocks.length ? textBlocks.join("\n\n") : ""); |
There was a problem hiding this comment.
Add size caps before inlining mentioned file contents.
This eagerly fetches every mentioned file and then inlines either the raw text or the full base64 data URL into state and the outgoing chat payload. Large text files will explode prompt size, and large PDFs/images will blow browser/request limits. Gate on file size before fetching, or switch this flow back to server-side references/summarization.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@hooks/useVercelChat.ts` around lines 108 - 146, The current flow eagerly
fetches full contents via getFileContents for every item in selected and may
inline huge text or base64 blobs; update the logic in the selected.map /
Promise.all block to first check file size (use f.size if available or call a
lightweight metadata endpoint/getFileMetadata) and skip or switch to a
server-side reference/summary for files over a safe threshold (e.g.,
configurable MAX_INLINE_SIZE), only calling getFileContents for files below that
threshold; when skipping, ensure setMentionAttachments and setMentionTextContext
receive either a placeholder reference (e.g., "[File omitted due to size:
path]") or a server-side URL/summary instead of inlining full content.
refactor: align file mentions with sandbox files flow (#1664)
2 participants
Summary
This PR has been repurposed after the dedicated files-list API migration was rolled back.
/api/sandboxes+/api/sandboxes/file)/api/files/listWhy this changed
The original plan introduced a separate files-list endpoint for mentions, but the Files section already relies on sandbox APIs. To avoid parallel data paths and closed API/docs workstreams, mentions now reuse the existing sandbox file flow.
Key Changes
useArtistFilesForMentionsnow reads from sandbox filetree (useSandboxes) rather than files table listinggetFileContents)SuggestionItemuseSandboxFileContentrefactored to support both:useSandboxFilePreview/app/api/files/list/route.tshooks/useFilesManager.tshooks/useBatchSignedUrls.tsVerification
lib/emails/__tests__/extractSendEmailResults.test.ts.Notes
/api/sandboxes/filereturns{ status, content, encoding? }.imageUrlis derived client-side ingetFileContentswhenencoding === "base64".Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores