[7418] External browse/upload dialogs#4826
Merged
sumerjabri merged 16 commits intocraftercms:developfrom Apr 22, 2026
Merged
Conversation
…o feature/7418-browse-s3-dialog # Conflicts: # ui/app/src/utils/constants.ts
|
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 44 minutes and 47 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 (1)
WalkthroughAdds new dialogs and utilities for browsing and uploading external assets (AWS S3 / WebDAV), extracts shared viewModes constant into a utils export, and makes SingleFileUpload HTTP method configurable. (47 words) Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant BrowseDialog as BrowseExternalAssetDialog
participant FolderTree as ExternalAssetFoldersTreeView
participant Service as AWS/WebDAV Service
participant State as Selection State
User->>BrowseDialog: Open dialog / click folder
BrowseDialog->>FolderTree: onFolderClick(path)
FolderTree->>Service: listAws/listWebdav(path, profileId)
Service-->>BrowseDialog: return items
BrowseDialog->>State: partition folders/files, update foldersByPath
BrowseDialog->>BrowseDialog: Render items (MediaCards) / apply viewModes
User->>BrowseDialog: Select item(s)
BrowseDialog->>State: Update selectedLookup / selectedCard
User->>BrowseDialog: Confirm
BrowseDialog-->>User: onSuccess(selected items)
sequenceDiagram
participant User
participant UploadDialog as ExternalAssetUploadDialog
participant SingleFileUpload
participant Uppy
participant Service as AWS S3/WebDAV
User->>UploadDialog: Open upload dialog (path, profileType)
UploadDialog->>UploadDialog: Compute upload URL (s3UploadUri / webDAVUploadUri)
User->>SingleFileUpload: Add file & start upload
SingleFileUpload->>UploadDialog: onUploadStart callback
SingleFileUpload->>Uppy: Configure XHRUpload (url, method, hidden fields)
Uppy->>Service: Upload file (PUT/POST)
Service-->>Uppy: Response
Uppy->>SingleFileUpload: onComplete / onError
SingleFileUpload->>UploadDialog: onUploadComplete / onUploadError callbacks
UploadDialog-->>User: Notify complete/error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 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}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ui/app/src/components/SingleFileUpload/SingleFileUpload.tsx (1)
196-229:⚠️ Potential issue | 🟡 MinorInclude
methodin the uploader setup effect dependencies.Line 211 configures
XHRUploadwith themethodprop, but line 228 omits it from the dependency array. If the component receives a differentmethodprop value (e.g., switching fromPUTtoPOST), the effect won't re-run, leaving the upload configured with the stale method.🔧 Proposed fix
- }, [uppy, formTarget, url, upload.timeout, path, site, formatMessage]); + }, [uppy, formTarget, url, method, upload.timeout, path, site, formatMessage]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/src/components/SingleFileUpload/SingleFileUpload.tsx` around lines 196 - 229, The useEffect that configures uppy (calling uppy.use(XHRUpload, { endpoint: url, method, ... })) is missing the method prop in its dependency array, so changes to method won't reconfigure the uploader; update the dependency array for that useEffect to include method (alongside uppy, formTarget, url, upload.timeout, path, site, formatMessage) so the effect re-runs when method changes and the XHRUpload instance is recreated with the new HTTP method.
🧹 Nitpick comments (2)
ui/app/src/components/ExternalAssetUploadDialog/ExternalAssetUploadDialog.tsx (2)
41-42:onCloseis included inExternalAssetUploadDialogBodyPropsbut never used.The
Pick<ExternalAssetUploadDialogProps, 'onClose'>addsonCloseto the body props interface, but it's never destructured or used inExternalAssetUploadDialogBody. Either remove it from the interface or use it if intended.♻️ If not needed, remove from interface
export interface ExternalAssetUploadDialogBodyProps - extends ExternalAssetUploadDialogBaseProps, Pick<ExternalAssetUploadDialogProps, 'onClose'> {} + extends ExternalAssetUploadDialogBaseProps {}And remove the
onClose={onClose}prop at line 119.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/src/components/ExternalAssetUploadDialog/ExternalAssetUploadDialog.tsx` around lines 41 - 42, The ExternalAssetUploadDialogBodyProps interface currently extends Pick<ExternalAssetUploadDialogProps, 'onClose'> but ExternalAssetUploadDialogBody does not use or destructure onClose; either remove the unnecessary onClose from ExternalAssetUploadDialogBodyProps (and delete the corresponding onClose={onClose} prop passed into ExternalAssetUploadDialogBody) or, if intended, add onClose to the component props for ExternalAssetUploadDialogBody and use it where the dialog should close; update the interface and the ExternalAssetUploadDialogBody function signature (and remove the redundant onClose prop where it's passed) so the types and usage stay consistent.
34-35: Consider adding explicit types for callback parameters.The
resultparameter usesany, andonUploadErrorhas an implicitly typed destructured object. Adding explicit types improves clarity and IDE support.🔧 Suggested type improvement
- onUploadComplete?(result: any): void; - onUploadError?({ file, error, response }): void; + onUploadComplete?(result: FileUploadResult): void; + onUploadError?(params: { file: UppyFile<Meta, Body>; error: Error; response: unknown }): void;Import
FileUploadResultfromSingleFileUploadif available, or define a suitable type.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/src/components/ExternalAssetUploadDialog/ExternalAssetUploadDialog.tsx` around lines 34 - 35, The callbacks onUploadComplete and onUploadError use implicit/any types; update their prop signatures in ExternalAssetUploadDialog to use explicit types (e.g., replace onUploadComplete?(result: any): void with onUploadComplete?(result: FileUploadResult): void by importing FileUploadResult from SingleFileUpload if it exists) and type the onUploadError parameter instead of an implicit destructure (e.g., onUploadError?(args: { file: File; error: Error; response: unknown }): void), or define a local interface (e.g., UploadErrorPayload) and use that to improve IDE support and type safety for the handlers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ui/app/src/components/BrowseS3Dialog/BrowseExternalAssetDialog.tsx`:
- Around line 127-129: The code skips updating an existing foldersByPath entry
and refresh always uses the initial path, causing stale folder nodes; change the
update at refs.current.foldersByPath?.[path] / setFoldersByPath to always
overwrite the entry for the currently opened folder (do not bail out when an
entry exists) and change the refresh logic that references the initial path to
use the current/open folder state (the runtime path variable used for
navigation, e.g., the component's currentPath or the runtime `path` that
reflects the opened folder). Ensure you update the setFoldersByPath((prev) => ({
...prev, [path]: folders })) call to run unconditionally for the active path and
replace the hardcoded/initial-path reference at the refresh site to the
current/open folder identifier so refreshes update the correct tree node.
- Around line 389-395: The recursive rendering of children in
ExternalAssetFoldersTreeView is missing a stable key, causing React
reconciliation warnings; update the JSX where foldersByPath[path]?.map(...)
renders <ExternalAssetFoldersTreeView ... /> to pass a stable key prop (e.g.,
key={folder.path} or key={folder.id} if available) so each recursive child has a
unique, consistent identifier; ensure you choose a property on folder that is
globally unique and stable across renders and add it to the component
instantiation alongside foldersByPath, path, folder, and onFolderClick.
- Around line 140-143: The render currently updates state and triggers
fetchItems when profileId changes (using profileId, prevProfileId,
setPrevProfileId, fetchItems) which causes side effects during render; move that
logic into a useEffect inside the BrowseExternalAssetDialog component that
watches profileId (e.g. useEffect(() => { setPrevProfileId(profileId);
fetchItems(path); }, [profileId])), remove the conditional from the render body,
and ensure fetchItems and setPrevProfileId are stable or included in the effect
dependencies to avoid stale closures.
- Around line 92-137: fetchItems currently subscribes to listAws/listWebdav
without canceling or ignoring prior in-flight responses, causing stale responses
to overwrite state (setItems, setSelectedCard, setSelectedLookup). Fix by adding
request cancellation/race protection: keep a ref (e.g., fetchSubscriptionRef or
fetchRequestIdRef) for the current request inside the component, unsubscribe the
previous subscription from listAws/listWebdav before starting a new one (or
increment a requestId and check it inside next/error handlers) and only call
setItems, setSelectedCard, setSelectedLookup, setFoldersByPath,
setIsFetchingItems, setError if the response matches the current active request;
ensure you unsubscribe in cleanup to avoid leaks and reference fetchItems,
listAws, listWebdav, refs.current.foldersByPath, setItems, setSelectedLookup,
setSelectedCard when locating where to apply the change.
In `@ui/app/src/components/BrowseS3Dialog/utils.ts`:
- Around line 112-117: The function getExternalItemType currently returns
lowercase 'pdf' and sometimes null while its signature claims to return string;
update the return value for PDFs to the canonical 'PDF' to match backend/Search
components and change the function signature return type to allow null (e.g.,
string | null) so callers see the nullable possibility; locate
getExternalItemType in utils.ts along with helpers isImage/isVideo/isPdfDocument
to implement these two small fixes.
In
`@ui/app/src/components/ExternalAssetUploadDialog/ExternalAssetUploadDialog.tsx`:
- Around line 44-45: ExternalAssetUploadDialogBody is not forwarding the
fileTypes prop from ExternalAssetUploadDialogBaseProps into SingleFileUpload, so
file filtering is broken; update ExternalAssetUploadDialogBody to destructure
fileTypes from props (alongside path, profileId, profileType, onUploadStart,
onUploadComplete, onUploadError, onFileAdded) and pass that fileTypes value into
the SingleFileUpload component instance(s) used in this file (also ensure the
same fix is applied in the other occurrence around the 80-89 block) so
SingleFileUpload receives and uses props.fileTypes.
- Around line 96-107: The ExternalAssetUploadDialog component is not forwarding
the fileTypes prop to the body; update the props destructuring in
ExternalAssetUploadDialog to include fileTypes (alongside path, profileId,
profileType, etc.) and pass fileTypes through to the
ExternalAssetUploadDialogBody invocation (same pattern used for
onClose/onUploadStart/etc.), and also ensure the same change is applied where
ExternalAssetUploadDialog is rendering the body in the alternate block that
mirrors lines 115-124 so fileTypes is consistently forwarded.
---
Outside diff comments:
In `@ui/app/src/components/SingleFileUpload/SingleFileUpload.tsx`:
- Around line 196-229: The useEffect that configures uppy (calling
uppy.use(XHRUpload, { endpoint: url, method, ... })) is missing the method prop
in its dependency array, so changes to method won't reconfigure the uploader;
update the dependency array for that useEffect to include method (alongside
uppy, formTarget, url, upload.timeout, path, site, formatMessage) so the effect
re-runs when method changes and the XHRUpload instance is recreated with the new
HTTP method.
---
Nitpick comments:
In
`@ui/app/src/components/ExternalAssetUploadDialog/ExternalAssetUploadDialog.tsx`:
- Around line 41-42: The ExternalAssetUploadDialogBodyProps interface currently
extends Pick<ExternalAssetUploadDialogProps, 'onClose'> but
ExternalAssetUploadDialogBody does not use or destructure onClose; either remove
the unnecessary onClose from ExternalAssetUploadDialogBodyProps (and delete the
corresponding onClose={onClose} prop passed into ExternalAssetUploadDialogBody)
or, if intended, add onClose to the component props for
ExternalAssetUploadDialogBody and use it where the dialog should close; update
the interface and the ExternalAssetUploadDialogBody function signature (and
remove the redundant onClose prop where it's passed) so the types and usage stay
consistent.
- Around line 34-35: The callbacks onUploadComplete and onUploadError use
implicit/any types; update their prop signatures in ExternalAssetUploadDialog to
use explicit types (e.g., replace onUploadComplete?(result: any): void with
onUploadComplete?(result: FileUploadResult): void by importing FileUploadResult
from SingleFileUpload if it exists) and type the onUploadError parameter instead
of an implicit destructure (e.g., onUploadError?(args: { file: File; error:
Error; response: unknown }): void), or define a local interface (e.g.,
UploadErrorPayload) and use that to improve IDE support and type safety for the
handlers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 51ddc276-a428-425a-bd9b-69fed24735ea
📒 Files selected for processing (9)
ui/app/src/components/BrowseFilesDialog/BrowseFilesDialogContainer.tsxui/app/src/components/BrowseFilesDialog/utils.tsui/app/src/components/BrowseS3Dialog/BrowseExternalAssetDialog.tsxui/app/src/components/BrowseS3Dialog/index.tsui/app/src/components/BrowseS3Dialog/utils.tsui/app/src/components/ExternalAssetUploadDialog/ExternalAssetUploadDialog.tsxui/app/src/components/ExternalAssetUploadDialog/index.tsui/app/src/components/SingleFileUpload/SingleFileUpload.tsxui/app/src/utils/constants.ts
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
ui/app/src/components/BrowseS3Dialog/BrowseExternalAssetDialog.tsx (3)
92-93:⚠️ Potential issue | 🟠 MajorAdd unmount cleanup for the active subscription.
Previous requests are canceled when a new fetch starts, but the active request is not canceled when the dialog unmounts.
🔧 Proposed fix
+ useEffect(() => { + return () => currentSub.current?.unsubscribe?.(); + }, []);Also applies to: 99-104, 142-145
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/src/components/BrowseS3Dialog/BrowseExternalAssetDialog.tsx` around lines 92 - 93, The component doesn't cancel the active subscription when it unmounts; add a cleanup that calls the unsubscribe on currentSub.current if present. Implement a useEffect(() => { return () => { currentSub.current?.unsubscribe?.(); }; }, []) (or add the same cleanup to any existing effect that manages subscriptions) so requestSeq and currentSub are left clean and any in-flight fetch handled by currentSub (the object referenced by currentSub and created in your fetch/start methods) is cancelled on unmount; apply the same pattern wherever you manage subscriptions (the code paths that set currentSub.current in the fetch/start functions referenced around requestSeq/currentSub usage).
17-17:⚠️ Potential issue | 🟠 MajorMove fetch/init side effects out of render.
The render path currently triggers state updates and network fetches. This should be in
useEffectto avoid render-phase side effects and duplicate calls.🔧 Proposed fix
-import React, { useCallback, useRef, useState } from 'react'; +import React, { useCallback, useEffect, useRef, useState } from 'react'; @@ - const [prevProfileId, setPrevProfileId] = useState(undefined); @@ - if (profileId !== prevProfileId) { - setPrevProfileId(profileId); - fetchItems(path); - } + useEffect(() => { + fetchItems(path); + }, [fetchItems, path]);Also applies to: 87-87, 147-150
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/src/components/BrowseS3Dialog/BrowseExternalAssetDialog.tsx` at line 17, The component BrowseExternalAssetDialog currently performs fetches and state updates during render; move those side effects into a useEffect hook to prevent render-phase network calls and duplicate executions. Identify the render-time calls (e.g., any calls to loadS3Buckets/fetchObjects or direct setBuckets/setObjects/setLoading in the component body) and wrap them in a useEffect with an appropriate dependency array (or empty array if it should run once), keep async logic inside the effect (use an inner async function and call it), and add cleanup/abort handling if needed; ensure refs remain via useRef and only set state inside the effect, and update any handlers that assumed immediate results accordingly (symbols to locate: BrowseExternalAssetDialog, loadS3Buckets, fetchObjects, setBuckets, setObjects, setLoading).
50-51:⚠️ Potential issue | 🟠 MajorFolder cache write should not be guarded by
nou(...).Skipping updates for an already-known path keeps stale folder nodes after subsequent fetches of the same folder.
🔧 Proposed fix
-import useUpdateRefs from '../../hooks/useUpdateRefs'; -import { nou } from '../../utils/object'; @@ - const refs = useUpdateRefs({ foldersByPath }); @@ - if (nou(refs.current.foldersByPath?.[path])) { - setFoldersByPath((prev) => ({ ...prev, [path]: folders })); - } + setFoldersByPath((prev) => ({ ...prev, [path]: folders })); @@ - [profileId, siteId, refs, setSelectedLookup, setSelectedCard, multiSelect, preselectedPaths, profileType] + [profileId, siteId, setSelectedLookup, setSelectedCard, multiSelect, preselectedPaths, profileType] );Also applies to: 88-89, 133-135, 144-145
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/src/components/BrowseS3Dialog/BrowseExternalAssetDialog.tsx` around lines 50 - 51, The folder-cache write is currently skipped when wrapped with nou(...), which allows stale folder nodes to persist; remove the nou(...) guard so the cache write always runs when updating the folder cache (replace uses like nou(folderCacheWrite(...)) with a direct call to the folder cache write function), ensuring updates for already-known paths update data rather than being skipped; apply the same removal of nou(...) guards in the other occurrences referenced (the other uses around the folder cache write in this file).
🧹 Nitpick comments (1)
ui/app/src/components/ExternalAssetUploadDialog/ExternalAssetUploadDialog.tsx (1)
41-43: Trim unusedonClosefrom body props.
ExternalAssetUploadDialogBodydoesn’t useonClose, so requiring/passing it adds unnecessary coupling.♻️ Proposed cleanup
-export interface ExternalAssetUploadDialogBodyProps - extends ExternalAssetUploadDialogBaseProps, Pick<ExternalAssetUploadDialogProps, 'onClose'> {} +export interface ExternalAssetUploadDialogBodyProps extends ExternalAssetUploadDialogBaseProps {}<ExternalAssetUploadDialogBody path={path} profileId={profileId} profileType={profileType} fileTypes={fileTypes} - onClose={onClose} onUploadStart={onUploadStart} onUploadComplete={onUploadComplete} onUploadError={onUploadError} onFileAdded={onFileAdded} />Also applies to: 126-132
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/src/components/ExternalAssetUploadDialog/ExternalAssetUploadDialog.tsx` around lines 41 - 43, ExternalAssetUploadDialogBodyProps unnecessarily requires onClose via Pick<ExternalAssetUploadDialogProps, 'onClose'> even though ExternalAssetUploadDialogBody doesn't use it; remove the Pick<'onClose'> from the ExternalAssetUploadDialogBodyProps declaration (leave it extending ExternalAssetUploadDialogBaseProps only) and update any call sites that were passing onClose into ExternalAssetUploadDialogBody (see the ExternalAssetUploadDialogBody component and the other occurrence around where props are assembled) so they no longer pass or type that prop.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@ui/app/src/components/BrowseS3Dialog/BrowseExternalAssetDialog.tsx`:
- Around line 92-93: The component doesn't cancel the active subscription when
it unmounts; add a cleanup that calls the unsubscribe on currentSub.current if
present. Implement a useEffect(() => { return () => {
currentSub.current?.unsubscribe?.(); }; }, []) (or add the same cleanup to any
existing effect that manages subscriptions) so requestSeq and currentSub are
left clean and any in-flight fetch handled by currentSub (the object referenced
by currentSub and created in your fetch/start methods) is cancelled on unmount;
apply the same pattern wherever you manage subscriptions (the code paths that
set currentSub.current in the fetch/start functions referenced around
requestSeq/currentSub usage).
- Line 17: The component BrowseExternalAssetDialog currently performs fetches
and state updates during render; move those side effects into a useEffect hook
to prevent render-phase network calls and duplicate executions. Identify the
render-time calls (e.g., any calls to loadS3Buckets/fetchObjects or direct
setBuckets/setObjects/setLoading in the component body) and wrap them in a
useEffect with an appropriate dependency array (or empty array if it should run
once), keep async logic inside the effect (use an inner async function and call
it), and add cleanup/abort handling if needed; ensure refs remain via useRef and
only set state inside the effect, and update any handlers that assumed immediate
results accordingly (symbols to locate: BrowseExternalAssetDialog,
loadS3Buckets, fetchObjects, setBuckets, setObjects, setLoading).
- Around line 50-51: The folder-cache write is currently skipped when wrapped
with nou(...), which allows stale folder nodes to persist; remove the nou(...)
guard so the cache write always runs when updating the folder cache (replace
uses like nou(folderCacheWrite(...)) with a direct call to the folder cache
write function), ensuring updates for already-known paths update data rather
than being skipped; apply the same removal of nou(...) guards in the other
occurrences referenced (the other uses around the folder cache write in this
file).
---
Nitpick comments:
In
`@ui/app/src/components/ExternalAssetUploadDialog/ExternalAssetUploadDialog.tsx`:
- Around line 41-43: ExternalAssetUploadDialogBodyProps unnecessarily requires
onClose via Pick<ExternalAssetUploadDialogProps, 'onClose'> even though
ExternalAssetUploadDialogBody doesn't use it; remove the Pick<'onClose'> from
the ExternalAssetUploadDialogBodyProps declaration (leave it extending
ExternalAssetUploadDialogBaseProps only) and update any call sites that were
passing onClose into ExternalAssetUploadDialogBody (see the
ExternalAssetUploadDialogBody component and the other occurrence around where
props are assembled) so they no longer pass or type that prop.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1b55f765-1a2a-43be-8094-9733adecbc9b
📒 Files selected for processing (4)
ui/app/src/components/BrowseS3Dialog/BrowseExternalAssetDialog.tsxui/app/src/components/BrowseS3Dialog/utils.tsui/app/src/components/ExternalAssetUploadDialog/ExternalAssetUploadDialog.tsxui/app/src/components/SingleFileUpload/SingleFileUpload.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- ui/app/src/components/SingleFileUpload/SingleFileUpload.tsx
- ui/app/src/components/BrowseS3Dialog/utils.ts
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
ui/app/src/components/BrowseS3Dialog/BrowseExternalAssetDialog.tsx (2)
148-150:⚠️ Potential issue | 🟠 MajorReset refresh back to the initial path.
onRefreshnow reloads the last clicked folder viacurrentPath; the intended behavior was to refresh from the initialpath. ResetcurrentPathand clear cached folders so the tree is rebuilt from that root.Proposed fix
useEffect(() => { + setCurrentPath(path); fnRefs.current.fetchItems(path, profileId); }, [profileId, path, fnRefs]); @@ - const onRefresh = () => fetchItems(currentPath, profileId); + const onRefresh = () => { + setCurrentPath(path); + setFoldersByPath({}); + fetchItems(path, profileId); + };Also applies to: 185-185
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/src/components/BrowseS3Dialog/BrowseExternalAssetDialog.tsx` around lines 148 - 150, The refresh handler now reloads from the last-clicked folder (currentPath) instead of the original root path; update the onRefresh implementation to reset currentPath back to the incoming prop path, clear any cached folder/tree state (e.g., foldersCache, cachedFolders or similar variables used to memoize tree nodes), and then call fnRefs.current.fetchItems(path, profileId) so the tree is rebuilt from the initial path; also ensure the useEffect that calls fnRefs.current.fetchItems uses [profileId, path] and does not depend on mutable refs like fnRefs in a way that prevents the root refresh behavior.
92-150:⚠️ Potential issue | 🟠 MajorKeep fetches aligned with
typechanges and clean up the active subscription.
typeis part of the request payload but not in the callback/effect dependencies, so a changed filter can keep using the previous value. Also unsubscribe the active request on unmount to avoid late state updates after the dialog closes.Proposed fix
const fetchItems = useCallback( (path, profileId) => { const fetchService = profileType === 'aws' ? listAws : listWebdav; @@ }); }, - [siteId, setSelectedLookup, setSelectedCard, multiSelect, preselectedPaths, profileType] + [siteId, setSelectedLookup, setSelectedCard, multiSelect, preselectedPaths, profileType, type] ); const fnRefs = useUpdateRefs({ fetchItems }); useEffect(() => { fnRefs.current.fetchItems(path, profileId); - }, [profileId, path, fnRefs]); + }, [profileId, path, type, fnRefs]); + + useEffect(() => { + return () => { + requestSeq.current += 1; + currentSub.current?.unsubscribe?.(); + }; + }, []);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/src/components/BrowseS3Dialog/BrowseExternalAssetDialog.tsx` around lines 92 - 150, The fetchItems callback and the useEffect that calls it must include the request filter `type` in their dependency lists and the component must unsubscribe the active subscription on unmount to avoid late state updates; update the useCallback dependency array for fetchItems to include `type`, update the useEffect dependencies to include `type` (or keep using fnRefs but ensure it reflects `type`), and add a cleanup (return) in the effect that calls currentSub.current?.unsubscribe() (and optionally increments/invalidates requestSeq.current) so any in-flight subscription is cancelled when the dialog unmounts or `type` changes; reference the existing symbols fetchItems, fnRefs, currentSub, requestSeq, and the useEffect that calls fnRefs.current.fetchItems.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ui/app/src/components/BrowseS3Dialog/BrowseExternalAssetDialog.tsx`:
- Around line 137-140: The error handler in the fetch flow (the arrow function
using error => { if (seq !== requestSeq.current) return;
setIsFetchingItems(false); setError(error); }) leaves previous items in state
and the setError value isn't surfaced; update the error path to (1) clear or set
items to an empty array via setItems([]) (or null if UI expects that) so stale
results aren't shown, (2) ensure the error state setError(error) is the same
error shape the component renders (or map it to a userFacingError message), and
(3) apply the same change to the other mirrored fetch block (lines ~301-340) so
both request handlers check seq vs requestSeq.current,
setIsFetchingItems(false), clear items with setItems(...), and setError(...)
consistently.
- Around line 313-318: Replace uses of previewUrl for identity and card path
with the stable external path when rendering filteredItems: in the
filteredItems.map that renders MediaCard, use a stable key and pass path by
preferring item.path (the external asset path) and falling back to
item.previewUrl only if path is null/undefined; update the key from
key={item.previewUrl} to key={item.path ?? item.previewUrl} and set the
MediaCard prop item to use path: item.path ?? item.previewUrl so MediaCard and
the list keys use the external asset path when available.
- Around line 119-124: The multi-select flow rebuilds selectedLookup for each
fetch and discards prior selections; instead merge new folder pre-selections
into the existing selectedLookup so earlier user choices persist: when
multiSelect is true, compute the per-folder preSelectedLookup from files (using
file.path and preselectedPaths) and then call setSelectedLookup with a merge of
the previous selectedLookup and the new preSelectedLookup (e.g., use the
functional updater to spread prev and the new entries), leaving single-select
behavior unchanged.
---
Duplicate comments:
In `@ui/app/src/components/BrowseS3Dialog/BrowseExternalAssetDialog.tsx`:
- Around line 148-150: The refresh handler now reloads from the last-clicked
folder (currentPath) instead of the original root path; update the onRefresh
implementation to reset currentPath back to the incoming prop path, clear any
cached folder/tree state (e.g., foldersCache, cachedFolders or similar variables
used to memoize tree nodes), and then call fnRefs.current.fetchItems(path,
profileId) so the tree is rebuilt from the initial path; also ensure the
useEffect that calls fnRefs.current.fetchItems uses [profileId, path] and does
not depend on mutable refs like fnRefs in a way that prevents the root refresh
behavior.
- Around line 92-150: The fetchItems callback and the useEffect that calls it
must include the request filter `type` in their dependency lists and the
component must unsubscribe the active subscription on unmount to avoid late
state updates; update the useCallback dependency array for fetchItems to include
`type`, update the useEffect dependencies to include `type` (or keep using
fnRefs but ensure it reflects `type`), and add a cleanup (return) in the effect
that calls currentSub.current?.unsubscribe() (and optionally
increments/invalidates requestSeq.current) so any in-flight subscription is
cancelled when the dialog unmounts or `type` changes; reference the existing
symbols fetchItems, fnRefs, currentSub, requestSeq, and the useEffect that calls
fnRefs.current.fetchItems.
🪄 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: 9c1281d8-0a17-4f37-9026-15ae01eb0c37
📒 Files selected for processing (1)
ui/app/src/components/BrowseS3Dialog/BrowseExternalAssetDialog.tsx
rart
approved these changes
Apr 22, 2026
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
3 participants
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.
craftercms/craftercms#7418
Summary by CodeRabbit
New Features
Refactor
Chores