feat(frontend): @agenta/entities/drive — the drive's headless layer leaves the app - #5876
feat(frontend): @agenta/entities/drive — the drive's headless layer leaves the app#5876ardaerzin wants to merge 1 commit into
Conversation
|
@coderabbitai review |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR centralizes Drive utilities in ChangesDrive package centralization
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Browser
participant useDriveDrop
participant dropEntries
participant DriveUpload
Browser->>useDriveDrop: drop files or directories
useDriveDrop->>dropEntries: readDroppedFiles(dataTransfer)
dropEntries-->>useDriveDrop: return relative-path files
useDriveDrop->>DriveUpload: upload non-empty file list
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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 |
|
e211095 to
26c16d8
Compare
6ac5beb to
e2d62a8
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (12)
web/packages/agenta-entities/src/drive/useDelayedTrue.ts (1)
3-4: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReduce this to one short comment line.
This comment documents normal hook behavior. Keep a longer comment only for a surprising constraint.
As per coding guidelines, “Keep in-code comments to at most one short line; use longer comments only for genuinely surprising constraints such as bugs, races, or ordering requirements.”
Source: Coding guidelines
web/packages/agenta-entities/src/drive/dropEntries.ts (1)
74-76: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider traversing children concurrently.
The loop awaits each child before it starts the next one. For a deep folder drop, each file also costs one
entry.filecallback round-trip, so total latency grows with file count.Promise.allkeeps the same output order and removes the serialization.♻️ Optional refactor
- const collected: DroppedFile[] = [] - for (const child of children) collected.push(...(await collectDropEntry(child, dirPrefix))) - return collected + const lists = await Promise.all(children.map((child) => collectDropEntry(child, dirPrefix))) + return lists.flat()web/packages/agenta-entities/src/drive/useDriveDrop.ts (1)
183-204: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winStabilize the returned
dropPropsobject and handlers.
useStageDropbuilds a newdropPropsobject with new inline handlers on every render. Hosts that spread these props onto a memoized child re-render on every parent render. The coding guidelines require memoized handlers and objects for props.Wrap the handlers in
useCallbackand the returned object inuseMemo. Keep the hook call order stable by computing the disabled result after the hooks run.♻️ Proposed refactor sketch
export function useStageDrop( onFiles: ((files: DroppedFile[]) => void) | false | null | undefined, ): { dropActive: boolean dropProps: FileDropProps } { const [dropActive, setDropActive] = useState(false) - if (!onFiles) return {dropActive: false, dropProps: {}} - return { - dropActive, - dropProps: { - onDragOver: (e) => { - if (!isFileDrag(e)) return - e.preventDefault() - setDropActive(true) - }, - onDragLeave: () => setDropActive(false), - onDrop: (e) => { - if (!isFileDrag(e)) return - e.preventDefault() - setDropActive(false) - void readDroppedFiles(e.dataTransfer).then((files) => { - if (files.length) onFiles(files) - }) - }, - }, - } + const onDragOver = useCallback((e: React.DragEvent<HTMLElement>) => { + if (!isFileDrag(e)) return + e.preventDefault() + setDropActive(true) + }, []) + const onDragLeave = useCallback(() => setDropActive(false), []) + const onDrop = useCallback( + (e: React.DragEvent<HTMLElement>) => { + if (!isFileDrag(e)) return + e.preventDefault() + setDropActive(false) + if (!onFiles) return + void readDroppedFiles(e.dataTransfer).then((files) => { + if (files.length) onFiles(files) + }) + }, + [onFiles], + ) + return useMemo( + () => + onFiles + ? {dropActive, dropProps: {onDragOver, onDragLeave, onDrop}} + : {dropActive: false, dropProps: {}}, + [onFiles, dropActive, onDragOver, onDragLeave, onDrop], + ) }As per coding guidelines: "Minimize React re-renders with
useMemo,useCallback, andReact.memowhere appropriate; avoid unstable inline functions and objects".Source: Coding guidelines
web/packages/agenta-entities/src/drive/useImagePreviews.ts (1)
27-46: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider a
[files]dependency for the reconcile effect.Both call sites pass a memoized array (
useDriveUploads.tsline 101 anduseMountUpload.tsline 217). With[files]the reconcile runs only when the input array identity changes, and the behavior stays identical for callers that pass a fresh array each render. Today the filter, Set build, and Map scan run after every commit of the host.♻️ Optional refactor
if (changed) setSnapshot(new Map(map)) - }) + }, [files])web/packages/agenta-entities/src/drive/pdfThumb.ts (1)
14-24: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winReset the cached promise when the import fails.
pdfjsPromisekeeps a rejected promise. If the dynamic chunk load fails once, for example on a transient network error, every later call torenderPdfFirstPagerejects and returnsnullfor the rest of the session. Clear the cache on rejection so a later thumbnail request can retry.♻️ Proposed resiliency fix
async function loadPdfjs(): Promise<PdfjsModule> { if (!pdfjsPromise) { - pdfjsPromise = import("pdfjs-dist").then((pdfjs) => { - pdfjs.GlobalWorkerOptions.workerSrc = "/pdf.worker.min.mjs" - return pdfjs - }) + pdfjsPromise = import("pdfjs-dist") + .then((pdfjs) => { + pdfjs.GlobalWorkerOptions.workerSrc = "/pdf.worker.min.mjs" + return pdfjs + }) + .catch((error) => { + pdfjsPromise = null // a failed chunk load must not poison every later render + throw error + }) } return pdfjsPromise }web/packages/agenta-entities/src/drive/driveTree.ts (1)
190-198: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
applyLoadedCountscan understate a lazily loaded folder's item count.
applyLoadedCountsreplaces the backenditem_countwithchildren.lengthfor every folder that has at least one loaded child. If a level is only partially loaded, or if a filter pruned children, the row shows fewer items than the folder holds. The comment covers only theagent-filesfold point.Consider restricting the override to folders that are fully loaded, or to the fold-point mount only.
web/packages/agenta-entities/src/drive/useDriveSelection.ts (3)
88-90: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDeclare the effect dependencies or silence the rule explicitly.
The effect calls
selectbut lists onlyinitialPath.react-hooks/exhaustive-depsreports this. The current omission is intentional, so record it in code instead of leaving a lint error. Addselectto the deps with a ref guard on the previousinitialPath, or add a targetedeslint-disable-next-linewith the reason.♻️ Proposed fix
useEffect(() => { if (initialPath != null) select(initialPath) + // eslint-disable-next-line react-hooks/exhaustive-deps -- fire on initialPath changes only }, [initialPath])As per coding guidelines: "Before committing frontend changes, run
pnpm lint-fixfrom thewebdirectory."Source: Coding guidelines
18-21: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider
atomWithStoragefor the per-drive last selection.The last-viewed path per drive is a persisted UI preference. The current module-level atom family loses it on reload, and every visited
mountIdkeeps an entry for the page lifetime. If cross-reload restore is wanted, useatomWithStoragewith anagenta:-prefixed key andstringStoragefor the nullable string. If it must stay session-scoped, state that in the comment.As per coding guidelines: "Use
atomWithStoragefor persisted preferences, UI state, recently used items, and form drafts; prefix storage keys withagenta:. UsestringStoragefor nullable strings."Source: Coding guidelines
30-33: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueShorten the narrative block comments. These modules carry multi-line comments that explain design intent and history rather than a surprising constraint. The repository rule allows longer comments only for genuinely surprising constraints such as bugs, races, or ordering requirements. Keep the ones that record a real hazard, for example the ReDoS note in
driveTree.tsat L23-L25, and move the rest into the module doc block or a design note.
web/packages/agenta-entities/src/drive/useDriveSelection.ts#L30-L33: reduce the empty-mountIdrationale to one line.web/packages/agenta-entities/src/drive/driveTree.ts#L75-L83: reduce theisFolderEntryheuristic comment to one line and keep the inference rule only.web/packages/agenta-entities/src/drive/driveTreeView.ts#L16-L19: reduce theSKELETON_ROW_CAPexplanation to one line.web/packages/agenta-entities/src/drive/useDriveTreePane.ts#L63-L69: move the anticipated-shift explanation into the module doc block and keep one line here.web/packages/agenta-entities/src/drive/useTreeGroupScroll.ts#L17-L20: reduce the group-scroll model description to one line.As per coding guidelines: "Keep in-code comments to at most one short line; use longer comments only for genuinely surprising constraints such as bugs, races, or ordering requirements."
Source: Coding guidelines
web/packages/agenta-entities/src/drive/useDriveTreePane.ts (1)
29-48: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider keyboard support for the resize handle.
The pane width changes through pointer events only. Keyboard users cannot resize the pane. Add a
role="separator"handle witharia-valuenowand arrow-key stepping betweenTREE_MINandTREE_MAXwhen the UI lane lands.web/packages/agenta-entities/src/drive/useDriveTreeReveal.ts (1)
28-45: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing
setExpandeddependency.The expansion effect uses
setExpandedbut lists onlyselectedPathandselectedIsFolder.setExpandedarrives as a prop, soreact-hooks/exhaustive-depsreports it. Add it to the deps; the setter identity is stable, so behavior does not change.♻️ Proposed fix
- }, [selectedPath, selectedIsFolder]) + }, [selectedPath, selectedIsFolder, setExpanded])As per coding guidelines: "Before committing frontend changes, run
pnpm lint-fixfrom thewebdirectory."Source: Coding guidelines
web/packages/agenta-entities/src/drive/useTreeGroupScroll.ts (1)
66-69: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winThrottle the re-render bump to one per frame.
bumpGroupScroll()runs on every wheel event that changes an offset. A trackpad emits many events per frame, so the tree re-renders more often than it paints. Coalesce the bump withrequestAnimationFrame, and cancel the pending frame in the teardown.As per coding guidelines: "debounce or throttle search, filter, scroll, and resize handlers."
Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 10f53913-a885-4522-996b-8b030cad3afd
📒 Files selected for processing (31)
web/packages/agenta-entities/package.jsonweb/packages/agenta-entities/src/drive/agentDrive.tsweb/packages/agenta-entities/src/drive/configDrive.tsweb/packages/agenta-entities/src/drive/driveFlags.tsweb/packages/agenta-entities/src/drive/driveKeyboard.tsweb/packages/agenta-entities/src/drive/driveKinds.tsweb/packages/agenta-entities/src/drive/driveLabels.tsweb/packages/agenta-entities/src/drive/driveMedia.tsweb/packages/agenta-entities/src/drive/driveMotion.tsweb/packages/agenta-entities/src/drive/driveRepo.tsweb/packages/agenta-entities/src/drive/driveTree.tsweb/packages/agenta-entities/src/drive/driveTreeView.tsweb/packages/agenta-entities/src/drive/driveTypes.tsweb/packages/agenta-entities/src/drive/dropEntries.tsweb/packages/agenta-entities/src/drive/index.tsweb/packages/agenta-entities/src/drive/pdfThumb.tsweb/packages/agenta-entities/src/drive/recentChange.tsweb/packages/agenta-entities/src/drive/useDelayedTrue.tsweb/packages/agenta-entities/src/drive/useDriveDrop.tsweb/packages/agenta-entities/src/drive/useDriveFilters.tsweb/packages/agenta-entities/src/drive/useDriveSelection.tsweb/packages/agenta-entities/src/drive/useDriveTreeKeyboard.tsweb/packages/agenta-entities/src/drive/useDriveTreePane.tsweb/packages/agenta-entities/src/drive/useDriveTreeReveal.tsweb/packages/agenta-entities/src/drive/useDriveTreeViewport.tsweb/packages/agenta-entities/src/drive/useDriveUploads.tsweb/packages/agenta-entities/src/drive/useImagePreviews.tsweb/packages/agenta-entities/src/drive/useMountUpload.tsweb/packages/agenta-entities/src/drive/useSessionDrive.tsweb/packages/agenta-entities/src/drive/useTreeGroupScroll.tsweb/packages/agenta-entities/tests/unit/dropEntries.test.ts
| "motion": "^12.0.0", | ||
| "openapi-json-schema": "^2.0.0", | ||
| "pdfjs-dist": "^4.10.38", |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Commit the lockfile for the new runtime dependencies.
package.json now declares motion and pdfjs-dist, but the pipeline reports that pnpm install --frozen-lockfile fails because pnpm-lock.yaml lacks both entries. Regenerate and commit the lockfile before merging this change.
Source: Pipeline failures
|
|
||
| import {type Mount} from "@agenta/entities/session" | ||
| import {App} from "antd" | ||
| import {axios, getAgentaApiUrl, getAuthToken} from "@agenta/shared/api" |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 4 'mounts/.*/files/download|mounts/files/export|axios\.(get|post)|fetch\(' \
web/packages/agenta-entities/src/drive web/packages/agenta-sdkRepository: Agenta-AI/agenta
Length of output: 7798
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Resources file =="
wc -l web/packages/agenta-sdk/src/resources.ts
sed -n '1,220p' web/packages/agenta-sdk/src/resources.ts
echo "== Drive media relevant sections =="
sed -n '1,80p' web/packages/agenta-entities/src/drive/driveMedia.ts
sed -n '230,345p' web/packages/agenta-entities/src/drive/driveMedia.ts
echo "== API package exports/axios config snippets =="
sed -n '1,140p' web/packages/agenta-shared/src/api.ts
sed -n '1,100p' web/packages/agenta-sdk/src/config.ts
echo "== SDK config package file list =="
fd -a . web/packages/agenta-sdk/src -t f | sed -n '1,120p'
echo "== Search for raw axios/fetch and Fern resource accessor patterns in web packages =="
rg -n "(from \"`@agenta/sdk`\"|from '`@agenta/sdk`'|axios\.get|axios\.post|axios\.blob|fetch\(|queryParams:|responseType: \"blob\"|responseType: \"stream\")" web/packages -g '*.ts' -g '*.tsx' | sed -n '1,220p'Repository: Agenta-AI/agenta
Length of output: 14665
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Locate api/config files =="
fd -a '(^|/)api\.ts$|(^|/)config\.ts$|resources\.ts$' web/packages | sed -n '1,200p'
echo "== Shared api file =="
api_file="$(fd '^api\.ts$' web/packages -d 4 | grep shared | head -n 1)"
echo "${api_file:-NOT_FOUND}"
if [ -n "${api_file:-}" ]; then
wc -l "$api_file"
sed -n '1,160p' "$api_file"
fi
echo "== SDK config =="
config_file="$(fd 'config\.ts$' web/packages/agenta-sdk/src | head -n 1 || true)"
if [ -n "${config_file:-}" ]; then
wc -l "$config_file"
sed -n '1,140p' "$config_file"
fi
echo "== Mounts resource client definition/imports =="
rg -n "file.*download|files.*download|files/download|export|download|FetchBlobs|Blobs|Blob" \
-S web/packages -g '*resource*' -g '*.ts' | sed -n '1,240p'
echo "== SDK package package.json refs =="
for f in web/packages/agenta-entities/package.json web/packages/agenta-sdk/package.json; do
echo "--- $f"
cat "$f"
done
echo "== MountsClient method names if declaration exists =="
tmp="$(mktemp -d)"
cd "$tmp"
set +e
npm view `@agentaai/api-client` 2>/dev/null | sed -n '1,160p'
set -eRepository: Agenta-AI/agenta
Length of output: 1408
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== All repository files named api.ts =="
git ls-files '*api.ts' | sed -n '1,200p'
echo "== File under review imports =="
sed -n '1,20p' web/packages/agenta-entities/src/drive/driveMedia.ts
echo "== Repository-wide imports of `@agenta/shared/api` =="
rg -n '`@agenta/shared/api`|shared/api' -S . | sed -n '1,200p'
echo "== SDK build config package names =="
sed -n '1,220p' web/packages/agenta-sdk/src/config.ts
sed -n '1,120p' web/packages/agenta-sdk/package.json
echo "== Mount route and mount client route references =="
rg -n 'files/download|files/export|downloadMountArchive|fetchMountFileBlob|MountsClient' web -S | sed -n '1,260p'Repository: Agenta-AI/agenta
Length of output: 33288
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== generated MountsClient get methods around files/download and export =="
sed -n '520,1010p' web/packages/agenta-api-client/src/generated/api/resources/mounts/client/Client.ts
echo "== MountsClient declarations for raw response methods =="
sed -n '1,70p' web/packages/agenta-api-client/src/generated/api/resources/mounts/client/Client.ts
echo "== check source package.json dependency version/workspace =="
sed -n '1,220p' web/packages/agenta-api-client/package.jsonRepository: Agenta-AI/agenta
Length of output: 24664
Route mount binary downloads through @agenta/sdk/resources.
fetchMountFileBlob and downloadMountArchive still call raw axios/fetch paths at web/packages/agenta-entities/src/drive/driveMedia.ts:55, :277, and :304. The Fern MountsClient already exposes binary responses for downloadMountFile and exportMountFiles; add resource accessors in web/packages/agenta-sdk/src/routes.ts/resources.ts as needed and use them instead of bypassing the SDK.
Source: Coding guidelines
| /** Object URL for an image preview, else null (icon fallback). Owned by DriveExplorer. */ | ||
| previewUrl: string | null |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update the ownership note for previewUrl.
The object URL lifecycle now belongs to useImagePreviews, which mints and revokes the URL (web/packages/agenta-entities/src/drive/useImagePreviews.ts). useDriveUploads fills previewUrl from that hook. The reference to DriveExplorer points at the UI layer and is no longer accurate for this headless package.
📝 Proposed doc fix
- /** Object URL for an image preview, else null (icon fallback). Owned by DriveExplorer. */
+ /** Object URL for an image preview, else null (icon fallback). Owned by `useImagePreviews`. */📝 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.
| /** Object URL for an image preview, else null (icon fallback). Owned by DriveExplorer. */ | |
| previewUrl: string | null | |
| /** Object URL for an image preview, else null (icon fallback). Owned by `useImagePreviews`. */ | |
| previewUrl: string | null |
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 11
🧹 Nitpick comments (12)
web/packages/agenta-entities/src/drive/useDelayedTrue.ts (1)
3-4: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReduce this to one short comment line.
This comment documents normal hook behavior. Keep a longer comment only for a surprising constraint.
As per coding guidelines, “Keep in-code comments to at most one short line; use longer comments only for genuinely surprising constraints such as bugs, races, or ordering requirements.”
Source: Coding guidelines
web/packages/agenta-entities/src/drive/dropEntries.ts (1)
74-76: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider traversing children concurrently.
The loop awaits each child before it starts the next one. For a deep folder drop, each file also costs one
entry.filecallback round-trip, so total latency grows with file count.Promise.allkeeps the same output order and removes the serialization.♻️ Optional refactor
- const collected: DroppedFile[] = [] - for (const child of children) collected.push(...(await collectDropEntry(child, dirPrefix))) - return collected + const lists = await Promise.all(children.map((child) => collectDropEntry(child, dirPrefix))) + return lists.flat()web/packages/agenta-entities/src/drive/useDriveDrop.ts (1)
183-204: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winStabilize the returned
dropPropsobject and handlers.
useStageDropbuilds a newdropPropsobject with new inline handlers on every render. Hosts that spread these props onto a memoized child re-render on every parent render. The coding guidelines require memoized handlers and objects for props.Wrap the handlers in
useCallbackand the returned object inuseMemo. Keep the hook call order stable by computing the disabled result after the hooks run.♻️ Proposed refactor sketch
export function useStageDrop( onFiles: ((files: DroppedFile[]) => void) | false | null | undefined, ): { dropActive: boolean dropProps: FileDropProps } { const [dropActive, setDropActive] = useState(false) - if (!onFiles) return {dropActive: false, dropProps: {}} - return { - dropActive, - dropProps: { - onDragOver: (e) => { - if (!isFileDrag(e)) return - e.preventDefault() - setDropActive(true) - }, - onDragLeave: () => setDropActive(false), - onDrop: (e) => { - if (!isFileDrag(e)) return - e.preventDefault() - setDropActive(false) - void readDroppedFiles(e.dataTransfer).then((files) => { - if (files.length) onFiles(files) - }) - }, - }, - } + const onDragOver = useCallback((e: React.DragEvent<HTMLElement>) => { + if (!isFileDrag(e)) return + e.preventDefault() + setDropActive(true) + }, []) + const onDragLeave = useCallback(() => setDropActive(false), []) + const onDrop = useCallback( + (e: React.DragEvent<HTMLElement>) => { + if (!isFileDrag(e)) return + e.preventDefault() + setDropActive(false) + if (!onFiles) return + void readDroppedFiles(e.dataTransfer).then((files) => { + if (files.length) onFiles(files) + }) + }, + [onFiles], + ) + return useMemo( + () => + onFiles + ? {dropActive, dropProps: {onDragOver, onDragLeave, onDrop}} + : {dropActive: false, dropProps: {}}, + [onFiles, dropActive, onDragOver, onDragLeave, onDrop], + ) }As per coding guidelines: "Minimize React re-renders with
useMemo,useCallback, andReact.memowhere appropriate; avoid unstable inline functions and objects".Source: Coding guidelines
web/packages/agenta-entities/src/drive/useImagePreviews.ts (1)
27-46: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider a
[files]dependency for the reconcile effect.Both call sites pass a memoized array (
useDriveUploads.tsline 101 anduseMountUpload.tsline 217). With[files]the reconcile runs only when the input array identity changes, and the behavior stays identical for callers that pass a fresh array each render. Today the filter, Set build, and Map scan run after every commit of the host.♻️ Optional refactor
if (changed) setSnapshot(new Map(map)) - }) + }, [files])web/packages/agenta-entities/src/drive/pdfThumb.ts (1)
14-24: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winReset the cached promise when the import fails.
pdfjsPromisekeeps a rejected promise. If the dynamic chunk load fails once, for example on a transient network error, every later call torenderPdfFirstPagerejects and returnsnullfor the rest of the session. Clear the cache on rejection so a later thumbnail request can retry.♻️ Proposed resiliency fix
async function loadPdfjs(): Promise<PdfjsModule> { if (!pdfjsPromise) { - pdfjsPromise = import("pdfjs-dist").then((pdfjs) => { - pdfjs.GlobalWorkerOptions.workerSrc = "/pdf.worker.min.mjs" - return pdfjs - }) + pdfjsPromise = import("pdfjs-dist") + .then((pdfjs) => { + pdfjs.GlobalWorkerOptions.workerSrc = "/pdf.worker.min.mjs" + return pdfjs + }) + .catch((error) => { + pdfjsPromise = null // a failed chunk load must not poison every later render + throw error + }) } return pdfjsPromise }web/packages/agenta-entities/src/drive/driveTree.ts (1)
190-198: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
applyLoadedCountscan understate a lazily loaded folder's item count.
applyLoadedCountsreplaces the backenditem_countwithchildren.lengthfor every folder that has at least one loaded child. If a level is only partially loaded, or if a filter pruned children, the row shows fewer items than the folder holds. The comment covers only theagent-filesfold point.Consider restricting the override to folders that are fully loaded, or to the fold-point mount only.
web/packages/agenta-entities/src/drive/useDriveSelection.ts (3)
88-90: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDeclare the effect dependencies or silence the rule explicitly.
The effect calls
selectbut lists onlyinitialPath.react-hooks/exhaustive-depsreports this. The current omission is intentional, so record it in code instead of leaving a lint error. Addselectto the deps with a ref guard on the previousinitialPath, or add a targetedeslint-disable-next-linewith the reason.♻️ Proposed fix
useEffect(() => { if (initialPath != null) select(initialPath) + // eslint-disable-next-line react-hooks/exhaustive-deps -- fire on initialPath changes only }, [initialPath])As per coding guidelines: "Before committing frontend changes, run
pnpm lint-fixfrom thewebdirectory."Source: Coding guidelines
18-21: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider
atomWithStoragefor the per-drive last selection.The last-viewed path per drive is a persisted UI preference. The current module-level atom family loses it on reload, and every visited
mountIdkeeps an entry for the page lifetime. If cross-reload restore is wanted, useatomWithStoragewith anagenta:-prefixed key andstringStoragefor the nullable string. If it must stay session-scoped, state that in the comment.As per coding guidelines: "Use
atomWithStoragefor persisted preferences, UI state, recently used items, and form drafts; prefix storage keys withagenta:. UsestringStoragefor nullable strings."Source: Coding guidelines
30-33: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueShorten the narrative block comments. These modules carry multi-line comments that explain design intent and history rather than a surprising constraint. The repository rule allows longer comments only for genuinely surprising constraints such as bugs, races, or ordering requirements. Keep the ones that record a real hazard, for example the ReDoS note in
driveTree.tsat L23-L25, and move the rest into the module doc block or a design note.
web/packages/agenta-entities/src/drive/useDriveSelection.ts#L30-L33: reduce the empty-mountIdrationale to one line.web/packages/agenta-entities/src/drive/driveTree.ts#L75-L83: reduce theisFolderEntryheuristic comment to one line and keep the inference rule only.web/packages/agenta-entities/src/drive/driveTreeView.ts#L16-L19: reduce theSKELETON_ROW_CAPexplanation to one line.web/packages/agenta-entities/src/drive/useDriveTreePane.ts#L63-L69: move the anticipated-shift explanation into the module doc block and keep one line here.web/packages/agenta-entities/src/drive/useTreeGroupScroll.ts#L17-L20: reduce the group-scroll model description to one line.As per coding guidelines: "Keep in-code comments to at most one short line; use longer comments only for genuinely surprising constraints such as bugs, races, or ordering requirements."
Source: Coding guidelines
web/packages/agenta-entities/src/drive/useDriveTreePane.ts (1)
29-48: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider keyboard support for the resize handle.
The pane width changes through pointer events only. Keyboard users cannot resize the pane. Add a
role="separator"handle witharia-valuenowand arrow-key stepping betweenTREE_MINandTREE_MAXwhen the UI lane lands.web/packages/agenta-entities/src/drive/useDriveTreeReveal.ts (1)
28-45: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing
setExpandeddependency.The expansion effect uses
setExpandedbut lists onlyselectedPathandselectedIsFolder.setExpandedarrives as a prop, soreact-hooks/exhaustive-depsreports it. Add it to the deps; the setter identity is stable, so behavior does not change.♻️ Proposed fix
- }, [selectedPath, selectedIsFolder]) + }, [selectedPath, selectedIsFolder, setExpanded])As per coding guidelines: "Before committing frontend changes, run
pnpm lint-fixfrom thewebdirectory."Source: Coding guidelines
web/packages/agenta-entities/src/drive/useTreeGroupScroll.ts (1)
66-69: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winThrottle the re-render bump to one per frame.
bumpGroupScroll()runs on every wheel event that changes an offset. A trackpad emits many events per frame, so the tree re-renders more often than it paints. Coalesce the bump withrequestAnimationFrame, and cancel the pending frame in the teardown.As per coding guidelines: "debounce or throttle search, filter, scroll, and resize handlers."
Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 10f53913-a885-4522-996b-8b030cad3afd
📒 Files selected for processing (31)
web/packages/agenta-entities/package.jsonweb/packages/agenta-entities/src/drive/agentDrive.tsweb/packages/agenta-entities/src/drive/configDrive.tsweb/packages/agenta-entities/src/drive/driveFlags.tsweb/packages/agenta-entities/src/drive/driveKeyboard.tsweb/packages/agenta-entities/src/drive/driveKinds.tsweb/packages/agenta-entities/src/drive/driveLabels.tsweb/packages/agenta-entities/src/drive/driveMedia.tsweb/packages/agenta-entities/src/drive/driveMotion.tsweb/packages/agenta-entities/src/drive/driveRepo.tsweb/packages/agenta-entities/src/drive/driveTree.tsweb/packages/agenta-entities/src/drive/driveTreeView.tsweb/packages/agenta-entities/src/drive/driveTypes.tsweb/packages/agenta-entities/src/drive/dropEntries.tsweb/packages/agenta-entities/src/drive/index.tsweb/packages/agenta-entities/src/drive/pdfThumb.tsweb/packages/agenta-entities/src/drive/recentChange.tsweb/packages/agenta-entities/src/drive/useDelayedTrue.tsweb/packages/agenta-entities/src/drive/useDriveDrop.tsweb/packages/agenta-entities/src/drive/useDriveFilters.tsweb/packages/agenta-entities/src/drive/useDriveSelection.tsweb/packages/agenta-entities/src/drive/useDriveTreeKeyboard.tsweb/packages/agenta-entities/src/drive/useDriveTreePane.tsweb/packages/agenta-entities/src/drive/useDriveTreeReveal.tsweb/packages/agenta-entities/src/drive/useDriveTreeViewport.tsweb/packages/agenta-entities/src/drive/useDriveUploads.tsweb/packages/agenta-entities/src/drive/useImagePreviews.tsweb/packages/agenta-entities/src/drive/useMountUpload.tsweb/packages/agenta-entities/src/drive/useSessionDrive.tsweb/packages/agenta-entities/src/drive/useTreeGroupScroll.tsweb/packages/agenta-entities/tests/unit/dropEntries.test.ts
🛑 Comments failed to post (8)
web/packages/agenta-entities/src/drive/driveKeyboard.ts (1)
9-26: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Find bindings of listArrowKeyDown and the row buttons in those surfaces. rg -n -C5 'listArrowKeyDown' web rg -n 'data-tree-main|data-row-main' webRepository: Agenta-AI/agenta
Length of output: 6256
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== driveKeyboard ==" cat -n web/packages/agenta-entities/src/drive/driveKeyboard.ts echo "== DriveTreeRow relevant section ==" cat -n web/oss/src/components/Drives/DriveTreeRow.tsx | sed -n '80,140p' echo "== DriveFileRow buttons and exported JSX ==" rg -n -C5 'DriveFileRow|data-row-main|data-tree-main|button\|button' web/oss/src/components/Drives/DriveFileRow.tsx echo "== list handler usages near render ==" cat -n web/oss/src/components/Drives/ContextRail.tsx | sed -n '230,270p' cat -n web/oss/src/components/Drives/StorageSection.tsx | sed -n '136,165p' echo "== JS DOM probe: roving behavior with descendant/readonly/disabled buttons ==" node - <<'JS' const rows = [ '<button id="main-0" data-row-main>Main0</button>', '<button id="main-1" data-row-main>Main1<menu><button id="menu-a">Menu</button></menu></button>', '<button id="main-2" data-row-main disabled>Main2</button>', ].join(""); const temp = document.createElement("tbody"); temp.innerHTML = rows; const all = Array.from(temp.querySelectorAll("button")); const main = Array.from(temp.querySelectorAll("button[data-row-main]")).filter(r => !r.disabled); let current = main[0]; const keys = ["ArrowDown", "ArrowDown", "End", "ArrowUp", "ArrowUp"]; const events = []; for (const key of keys) { const e = new KeyboardEvent("keydown", {key, bubbles: true}); const target = current; target.dispatchEvent(e); if (key.startsWith("Arrow")) e.preventDefault(); const rowsList = e.currentTarget.querySelectorAll("button"); const idx = Array.from(rowsList).findIndex(r => r === document.activeElement); const next = idx < 0 ? 0 : Math.min(Math.max(idx + (key === "ArrowDown" ? 1 : -1), 0), rowsList.length - 1); rowsList[next]?.focus(); events.push({key, from: target.id, allRowIndex: idx, allNext: rowsList[next]?.id, activeElement: document.activeElement?.id}); current = rowsList[next]; } console.log(JSON.stringify({ allRowCount: all.length, allIndices: all.map(b => b.id), mainRowCount: main.length, main: main.map(b => ({id: b.id, text: b.textContent?.trim()})), events, nextButtonFromMainIndexViaAll: main.map(b => { const idx = all.indexOf(b); return {id: b.id, index: idx, allNext: all[idx + 1]?.id}; }) }, null, 2)); JSRepository: Agenta-AI/agenta
Length of output: 12735
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== DriveFileRow implementation ==" wc -l web/oss/src/components/Drives/DriveFileRow.tsx cat -n web/oss/src/components/Drives/DriveFileRow.tsx | sed -n '120,260p' echo "== DriveRetryButton implementation ==" cat -n web/oss/src/components/Drives/DriveFileRow.tsx | sed -n '1,80p' echo "== KeyboardEvent imports/usages in web ==" rg -n 'KeyboardEvent<|KeyboardEvent\(' web/oss web/packages | head -80 echo "== DriveFileRow button search ==" rg -n -C4 '<button|data-row|data-tree|disabled' web/oss/src/components/Drives/DriveFileRow.tsxRepository: Agenta-AI/agenta
Length of output: 19322
Restrict the row query to row controls and skip disabled buttons.
listArrowKeyDownis bound by the drive context rail and config Files list, and both renderDriveFileRow. Inside the row,DriveRetryButtonrenders a disabled button when busy, so the currentquerySelectorAll("button")query includes it as a row stop whilefocus()does nothing. Use a row selector and!r.disabledso arrow keys skip retry controls and secondary row actions if they are added.web/packages/agenta-entities/src/drive/driveTree.ts (2)
114-119: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Add a GB tier to
humanSize.Files of 1 GB or more render as large MB values, for example "2048.0 MB". Agent-written drives can contain such files.
♻️ Proposed fix
export const humanSize = (bytes?: number | null): string => { if (bytes == null) return "" if (bytes < 1024) return `${bytes} B` if (bytes < 1024 * 1024) return `${(bytes / 1024).toFixed(1)} KB` - return `${(bytes / (1024 * 1024)).toFixed(1)} MB` + if (bytes < 1024 * 1024 * 1024) return `${(bytes / (1024 * 1024)).toFixed(1)} MB` + return `${(bytes / (1024 * 1024 * 1024)).toFixed(1)} GB` }📝 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.export const humanSize = (bytes?: number | null): string => { if (bytes == null) return "" if (bytes < 1024) return `${bytes} B` if (bytes < 1024 * 1024) return `${(bytes / 1024).toFixed(1)} KB` if (bytes < 1024 * 1024 * 1024) return `${(bytes / (1024 * 1024)).toFixed(1)} MB` return `${(bytes / (1024 * 1024 * 1024)).toFixed(1)} GB` }
202-219: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
A folder that matches the query loses its contents.
In
filterDriveTree, a folder survives when its own path matches, but itschildrenare replaced by the filtered list. A folder whose name matches then renders as empty, because no descendant path contains the query string. Users normally expect the matched folder's contents.♻️ Proposed fix
const children = walk(node.children) - return children.length || node.path.toLowerCase().includes(q) - ? {...node, children} - : null + if (node.path.toLowerCase().includes(q)) return node + return children.length ? {...node, children} : null📝 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./** Case-insensitive path-substring filter; folders survive when any descendant matches. */ export function filterDriveTree(nodes: DriveTreeNode[], query: string): DriveTreeNode[] { const q = query.trim().toLowerCase() if (!q) return nodes const walk = (list: DriveTreeNode[]): DriveTreeNode[] => list .map((node) => { if (!node.isFolder) { return node.path.toLowerCase().includes(q) ? node : null } const children = walk(node.children) if (node.path.toLowerCase().includes(q)) return node return children.length ? {...node, children} : null }) .filter((n): n is DriveTreeNode => Boolean(n)) return walk(nodes) }web/packages/agenta-entities/src/drive/recentChange.ts (1)
30-30: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Refresh
nowbefore the early return.If
lastTouchedAtchanges to an expired timestamp while a prior timer is active, cleanup stops the timer and this branch does not updatenow. The stale value can keepisRecentlyChangedtrue until another prop change.Proposed fix
useEffect(() => { - if (lastTouchedAt == null || Date.now() - lastTouchedAt >= windowMs) return - setNow(Date.now()) + const currentNow = Date.now() + setNow(currentNow) + if (lastTouchedAt == null || currentNow - lastTouchedAt >= windowMs) return const id = setInterval(() => {📝 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 currentNow = Date.now() setNow(currentNow) if (lastTouchedAt == null || currentNow - lastTouchedAt >= windowMs) returnweb/packages/agenta-entities/src/drive/useDriveDrop.ts (1)
52-92: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Clear the spring timer on unmount.
springTimerholds a pendingwindow.setTimeout. No cleanup path cancels it when the component unmounts. If the host unmounts during the 700 ms hover countdown, the callback still runs and callsonNavigate(path)plussetHoverPath(null)after unmount. That produces an unexpected navigation and keeps the closure alive.Add an unmount effect that calls
clearSpring.🛡️ Proposed fix
const clearSpring = useCallback(() => { window.clearTimeout(springTimer.current) springTimer.current = undefined springPath.current = null }, []) + + // The spring timeout must not outlive the host — it would navigate after unmount. + useEffect(() => clearSpring, [clearSpring])📝 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 springTimer = useRef<number | undefined>(undefined) const springPath = useRef<string | null>(null) const clearSpring = useCallback(() => { window.clearTimeout(springTimer.current) springTimer.current = undefined springPath.current = null }, []) // The spring timeout must not outlive the host — it would navigate after unmount. useEffect(() => clearSpring, [clearSpring]) // Window-level drag tracking for the overall `dragging` flag (depth counter absorbs the // dragenter/leave flicker from moving across child elements). const depth = useRef(0) useEffect(() => { if (!enabled) return const has = (e: DragEvent) => Array.from(e.dataTransfer?.types ?? []).includes("Files") const onEnter = (e: DragEvent) => { if (has(e)) { depth.current += 1 setDragging(true) } } const onLeave = () => { depth.current = Math.max(0, depth.current - 1) if (depth.current === 0) setDragging(false) } const onEnd = () => { depth.current = 0 setDragging(false) setHoverPath(null) clearSpring() } window.addEventListener("dragenter", onEnter) window.addEventListener("dragleave", onLeave) window.addEventListener("drop", onEnd) window.addEventListener("dragend", onEnd) return () => { window.removeEventListener("dragenter", onEnter) window.removeEventListener("dragleave", onLeave) window.removeEventListener("drop", onEnd) window.removeEventListener("dragend", onEnd) } }, [enabled, clearSpring])web/packages/agenta-entities/src/drive/useDriveTreeKeyboard.ts (1)
36-39: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Resolve
activePathfrom the closest row element.The handler reads
data-pathfromdocument.activeElementonly. If focus sits on a control inside a row, for example a row action button,activePathisnullandidxis-1. ArrowDown and ArrowUp then jump to row 0 instead of the neighbour row, and ArrowLeft and ArrowRight do nothing useful.♻️ Proposed fix
- const activePath = (document.activeElement as HTMLElement | null)?.getAttribute( - "data-path", - ) + const activeEl = document.activeElement as HTMLElement | null + const activePath = + activeEl?.closest<HTMLElement>("[data-path]")?.getAttribute("data-path") ?? null📝 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 activeEl = document.activeElement as HTMLElement | null const activePath = activeEl?.closest<HTMLElement>("[data-path]")?.getAttribute("data-path") ?? null const idx = activePath != null ? (indexByPath.get(activePath) ?? -1) : -1web/packages/agenta-entities/src/drive/useDriveTreePane.ts (1)
49-58: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Handle a cancelled or lost pointer during the resize drag.
onTreeHandleUpruns on pointer up only. If the browser firespointercancel, or if pointer capture is lost,treeDrag.currentstays set andtreeDraggingstaystrue. The pane then keeps its dragging state and continues to follow later pointer moves over the handle.Export one end handler and bind it to
onPointerUp,onPointerCancel, andonLostPointerCapture.♻️ Proposed fix
const onTreeHandleUp = useCallback( - (e: React.PointerEvent<HTMLDivElement>) => { + (e: React.PointerEvent<HTMLDivElement>) => { if (!treeDrag.current) return treeDrag.current = null setTreeDragging(false) setTreeWidth(Math.round(paneW.get())) - e.currentTarget.releasePointerCapture?.(e.pointerId) + if (e.currentTarget.hasPointerCapture?.(e.pointerId)) + e.currentTarget.releasePointerCapture?.(e.pointerId) }, [paneW], )Bind it as
onPointerUp={onTreeHandleUp}andonPointerCancel={onTreeHandleUp}in the consuming component.web/packages/agenta-entities/src/drive/useTreeGroupScroll.ts (1)
59-65: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Handle
deltaModepage units and clamp per group width.Two edge cases in the offset math:
e.deltaMode === 2(DOM_DELTA_PAGE) falls into the1branch, so a page-unit wheel event scrolls a few pixels.maxScrollusesel.clientWidthof the whole scroll container. Each group's rows are indented by depth, so the usable width per group is smaller. The clamp then permits over-scroll for deep groups.♻️ Proposed fix for the delta unit
- const unit = e.deltaMode === 1 ? 16 : 1 + const unit = e.deltaMode === 1 ? 16 : e.deltaMode === 2 ? el.clientWidth : 1
26c16d8 to
ede9f4d
Compare
e2d62a8 to
c185e3e
Compare
1 participant
The drive's queries, mutations, selectors and types move to
@agenta/entities/drive. The UIfollows in the next lane; splitting them keeps each diff readable.
Not run in a browser — static gates only (
pnpm lint-fix24/24,tsc --noEmitcleanfor
@agenta/shared,ui,entities,entity-ui,settings-ui,oss,ee,mobile).Stacked on
pkg/observability; review only this lane's diff.