Skip to content

[chore] Split DriveExplorer into hooks and views - #5570

Open
ardaerzin wants to merge 3 commits into
release/v0.106.2from
fe-chore/drive-explorer-split
Open

[chore] Split DriveExplorer into hooks and views#5570
ardaerzin wants to merge 3 commits into
release/v0.106.2from
fe-chore/drive-explorer-split

Conversation

@ardaerzin

Copy link
Copy Markdown
Contributor

Context

DriveExplorer.tsx had grown to 2587 lines holding 14 inline components plus the whole browsing body: the lazy tree pipeline, selection, search, the per-parent horizontal scroll groups, pane drag, keyboard navigation, the row virtualizer, and the upload staging flow.

Changes

The presentational half moves to sibling modules, then the remaining body splits into hooks. The Drives/ folder is flat by convention, so everything joins it as siblings rather than introducing subdirectories.

views    DriveBreadcrumb, DriveTreeRow, DriveFileContentViewer,
         DriveFilePreview, FolderTile, FolderView, DriveHeader,
         DriveTreeList, DriveTreePane, DriveToolbar, DriveExplorerStates,
         DrivePendingTiles
hooks    useDriveFilters, useDriveSelection, useDriveTreeData,
         useDriveTreePane, useTreeGroupScroll, useDriveTreeViewport,
         useDriveTreeReveal, useDriveTreeKeyboard, useDriveDownloadAll,
         useDriveUploads
helpers  driveTypes, driveTreeView, useDelayedTrue; motion tokens append
         to the existing driveMotion.ts

DriveExplorer.tsx is 409 lines.

Shared refs stay inside the hook that owns them. The tree scroll element never leaves useDriveTreeViewport (group scroll attaches through attachTreeWheel(el)), and the group maps never leave useTreeGroupScroll (rows read scrollXFor(parent)).

Ordering matters for uploads and is preserved. useDriveUploads is called above the tree so its uploadFiles feed buildDriveTree. That is what makes an in-flight upload render as a real row under its destination folder instead of a separate pinned list. pendingUploadByPath then decorates the resulting rows.

currentFolder and commitStaged stay in the root on purpose. They read the selection, which derives from the tree, which consumes useDriveUploads. Moving them into the hook would be circular, and keeping them out preserved their dependency arrays exactly.

Tests

  • pnpm --filter @agenta/oss exec tsc --noEmit passes with zero errors, as do @agenta/entity-ui and @agenta/ee.
  • pnpm lint-fix across the repo makes no changes. Prettier clean.
  • Effect bodies, dependency arrays and lazy initializers moved verbatim, comments included.
  • Rebuilt against current release/v0.106.1 rather than rebased, because the upload and staging feature (512 added lines) landed inside the regions this refactor deletes. Verified line by line: every behavioural line survives, and NEXT_PUBLIC_AGENT_FILE_UPLOADS still reaches the drop targets, the header upload button and the staged inbox.

Not runtime verified. The gates above are static.

One follow-up left in place: the module doc still says "Phase 1 is read-only", which uploads made false. It was kept verbatim rather than silently edited in a structural refactor.

What to QA

  • Open the files drawer and expand a deep folder. Rows appear, and a slow level shows shimmer placeholders that swap in place with no height jump.
  • Scroll a nested folder's rows sideways. Siblings move together, other folders stay put.
  • Drag a file onto a folder row and onto the tree background. Both upload to the right destination, and the in-flight item shows as a row under that folder.
  • Fail an upload (drop something oversized). The tile offers Dismiss, and Retry works.
  • Drag the tree pane divider, then collapse and expand it. The content pane tracks the width in one pass with no second snap.
  • Arrow-key around the tree and the tile grid. Cmd/Ctrl+Down opens, Cmd/Ctrl+Up steps out.
  • Regression: Download all still produces a zip, and a partial mount failure still shows the inline warning with Try again.

DriveExplorer.tsx: 2587 -> 409 lines, across 26 flat siblings (the Drives
folder has no subdirectories; new modules join it as siblings).

  views    DriveBreadcrumb, DriveTreeRow, DriveFileContentViewer,
           DriveFilePreview, FolderTile, FolderView, DriveHeader,
           DriveTreeList, DriveTreePane, DriveToolbar, DriveExplorerStates,
           DrivePendingTiles
  hooks    useDriveFilters, useDriveSelection, useDriveTreeData,
           useDriveTreePane, useTreeGroupScroll, useDriveTreeViewport,
           useDriveTreeReveal, useDriveTreeKeyboard, useDriveDownloadAll,
           useDriveUploads
  helpers  driveTypes, driveTreeView, useDelayedTrue; motion tokens append
           to the existing driveMotion.ts

Refs stay inside their owning hook: the scroll element never leaves
useDriveTreeViewport (group scroll attaches via attachTreeWheel) and the
group maps never leave useTreeGroupScroll (rows read scrollXFor(parent)).

Split against current content so the upload/staging feature survives.
useDriveUploads is called ABOVE the tree so its uploadFiles feed
buildDriveTree — that is what makes an in-flight upload render as a real
row under its destination folder rather than a pinned list — and
pendingUploadByPath then decorates the resulting rows. retry/dismiss are
exposed as named accessors rather than leaking the upload object.

currentFolder and commitStaged stay in the root: they read the selection,
which derives from the tree, which consumes useDriveUploads — moving them
into the hook would be circular. Verified line-by-line against the upstream
diff.
@dosubot dosubot Bot added the size:XXL This PR changes 1000+ lines, ignoring generated files. label Jul 30, 2026
@vercel

vercel Bot commented Jul 30, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
agenta-documentation Ready Ready Preview Jul 30, 2026 10:53pm

Request Review

@dosubot dosubot Bot added Frontend refactoring A code change that neither fixes a bug nor adds a feature labels Jul 30, 2026
@coderabbitai

coderabbitai Bot commented Jul 30, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Summary by CodeRabbit

  • New Features
    • Redesigned drive explorer with a resizable, searchable, virtualized file tree and keyboard navigation.
    • Added breadcrumb navigation, folder tiles, file previews, metadata, copy-path, and download actions.
    • Added upload staging with progress, retry, dismiss, drag-and-drop, and folder-specific upload options.
    • Added “Download all” for drive contents.
    • Added filters for origins, hidden files, and Git-ignored files.
  • Bug Fixes
    • Improved loading, empty, error, and upload-status feedback, including download status notifications.

Walkthrough

DriveExplorer is refactored into a composition root. New hooks and components provide filtering, selection, tree navigation, uploads, downloads, folder views, file previews, and terminal states.

Changes

Drive explorer modularization

Layer / File(s) Summary
Shared contracts and tree utilities
web/oss/src/components/Drives/driveTypes.ts, driveMotion.ts, driveTreeView.ts, useDelayedTrue.ts, useTreeGroupScroll.ts
Adds shared types, animation settings, tree shaping, delayed state, and horizontal group scrolling.
Filters, selection, tree data, and pane state
web/oss/src/components/Drives/useDriveFilters.ts, useDriveSelection.ts, useDriveTreeData.ts, useDriveTreeViewport.ts, useDriveTreeKeyboard.ts, useDriveTreeReveal.ts, useDriveTreePane.ts
Adds filtering, persisted selection, lazy tree data, virtualization, keyboard navigation, reveal behavior, and pane resizing.
Tree and navigation components
web/oss/src/components/Drives/DriveTreeRow.tsx, DriveTreeList.tsx, DriveTreePane.tsx, DriveBreadcrumb.tsx, DriveToolbar.tsx
Adds virtualized tree rendering, drop and context-menu wiring, pane layout, breadcrumbs, and filter controls.
Upload and download flows
web/oss/src/components/Drives/useDriveUploads.ts, DrivePendingTiles.tsx, useDriveDownloadAll.ts, driveMedia.ts, DriveFileCard.tsx, DriveItemContextMenu.tsx, driveFileSource.tsx
Centralizes upload staging and retry handling. File and archive downloads use shared hooks and feedback.
Folder, file, and terminal views
web/oss/src/components/Drives/FolderView.tsx, FolderTile.tsx, DriveFileContentViewer.tsx, DriveFilePreview.tsx, DriveHeader.tsx, DriveExplorerStates.tsx
Adds folder tile views, file previews, metadata controls, header actions, and empty or error states.
DriveExplorer composition wiring
web/oss/src/components/Drives/DriveExplorer.tsx
Replaces inline explorer logic with the new hooks and components.

Estimated code review effort: 5 (Critical) | ~120 minutes

Sequence Diagram(s)

sequenceDiagram
  participant DriveExplorer
  participant useDriveTreeData
  participant useDriveUploads
  participant DriveTreePane
  participant DriveTreeList
  participant FolderView

  DriveExplorer->>useDriveTreeData: derive filtered flatRows
  DriveExplorer->>useDriveUploads: provide upload state and handlers
  DriveExplorer->>DriveTreePane: render tree and content panes
  DriveTreePane->>DriveTreeList: pass virtualized rows and drop props
  DriveTreeList->>DriveExplorer: emit selection and expansion changes
  DriveExplorer->>FolderView: render selected folder entries
Loading

Possibly related PRs

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 27.78% which is insufficient. The required threshold is 60.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the primary change: splitting DriveExplorer into hooks and views.
Description check ✅ Passed The description is directly related and explains the refactor, preserved upload behavior, validation, and QA scope.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches 💡 2
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🛠️ Fix failing CI checks 💡
  • Fix failing CI checks
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fe-chore/drive-explorer-split

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

github-actions Bot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Railway Preview Environment

Preview URL https://gateway-production-90a0.up.railway.app/w
Project agenta-oss-pr-5570
Image tag pr-5570-7304e25
Status Deployed
Railway logs Open logs
Workflow logs View workflow run
Updated at 2026-07-30T23:04:20.059Z

@ardaerzin
ardaerzin changed the base branch from release/v0.106.1 to release/v0.106.2 July 30, 2026 18:08
@ardaerzin

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 30, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 6

🧹 Nitpick comments (7)
web/oss/src/components/Drives/useDriveSelection.ts (1)

51-53: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add an explicit eslint-disable-next-line react-hooks/exhaustive-deps.

select is intentionally omitted so the effect only reacts to prop changes; without the disable comment pnpm lint-fix will either flag it or "fix" it by adding select, silently changing behavior.

♻️ Suggested change
     useEffect(() => {
         if (initialPath != null) select(initialPath)
+        // eslint-disable-next-line react-hooks/exhaustive-deps -- prop-change only, must not refire on select identity
     }, [initialPath])
web/oss/src/components/Drives/useDriveUploads.ts (1)

50-56: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Avoid the as unknown as MountFile double cast.

This silently accepts any future required field on MountFile. Build the object typed (or widen the tree-build input to Pick<MountFile, "path" | "size">) so the compiler keeps verifying the synthetic nodes.

web/oss/src/components/Drives/DrivePendingTiles.tsx (1)

56-86: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Prefer theme tokens over hard-coded rgba(0,0,0,…) overlays.

The retry pill, progress track, and remove button use raw color literals. Swapping to antd semantic token variables (e.g. var(--ant-color-bg-mask)) keeps light/dark consistent with the rest of the drive UI.

As per coding guidelines, "Consume theme colors through Ant Design semantic tokens, Tailwind color utilities, or supported var(--ag-color*) variables; do not use raw hex colors or --ag-c-* literals."

Source: Coding guidelines

web/oss/src/components/Drives/FolderView.tsx (1)

254-300: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

Deduplicate the staged/upload tile wrapper and stabilize renderTile.

The two motion.div wrappers (Lines 260-274 and 281-299) are identical except for the child; extract one <PendingTileMotion> wrapper. Also, renderTile is a fresh closure on every render, so VirtualTileGrid re-renders every cell on any parent state change (e.g. repoExpanded) — wrap it in useCallback or lift the tile into a memoized component.

As per coding guidelines, "Minimize React re-renders with useMemo, useCallback, and React.memo where appropriate; avoid unstable inline functions and objects, especially in lists."

Source: Coding guidelines

web/oss/src/components/Drives/DriveTreeRow.tsx (1)

73-81: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Raw Ant Design CSS variable used for the primary-bg tint in two places. Both sites hardcode bg-[var(--ant-color-primary-bg)] instead of a sanctioned theme channel (Ant Design semantic token, Tailwind color utility, or var(--ag-color*)), so the tint bypasses the project's token pipeline and its light/dark verification.

  • web/oss/src/components/Drives/DriveTreeRow.tsx#L73-L81: replace the pending-upload tint with the semantic Tailwind utility (e.g. bg-colorPrimaryBg).
  • web/oss/src/components/Drives/DriveTreeList.tsx#L100-L108: replace the drop-hover tint with the same utility so both row states stay in sync.

As per coding guidelines: "Consume theme colors through Ant Design semantic tokens, Tailwind color utilities, or supported var(--ag-color*) variables; do not use raw hex colors or --ag-c-* literals."

Source: Coding guidelines

web/oss/src/components/Drives/DriveTreeList.tsx (1)

148-155: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value

Hoist the toggle closure out of the row map.

A fresh onToggle (and the setExpanded updater) is allocated per visible row on every render, defeating any future React.memo on TreeRow. A single useCallback((path: string) => setExpanded(...)) above the return would be reused by all rows.

As per coding guidelines: "Minimize React re-renders with useMemo, useCallback, and React.memo where appropriate; avoid unstable inline functions and objects, especially in lists."

Source: Coding guidelines

web/oss/src/components/Drives/DriveTreePane.tsx (1)

77-91: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Separator is mouse-only.

role="separator" with pointer handlers but no tabIndex={0}, arrow-key handling, or aria-valuenow/aria-valuemin/aria-valuemax makes the resize unreachable by keyboard and unannounced. The toolbar toggle keeps the pane usable, so this is an enhancement rather than a blocker.


ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 4e680dee-ce45-4f37-ac4b-a92f69f7a51e

📥 Commits

Reviewing files that changed from the base of the PR and between a0ef2cc and bbb64a8.

📒 Files selected for processing (27)
  • web/oss/src/components/Drives/DriveBreadcrumb.tsx
  • web/oss/src/components/Drives/DriveExplorer.tsx
  • web/oss/src/components/Drives/DriveExplorerStates.tsx
  • web/oss/src/components/Drives/DriveFileContentViewer.tsx
  • web/oss/src/components/Drives/DriveFilePreview.tsx
  • web/oss/src/components/Drives/DriveHeader.tsx
  • web/oss/src/components/Drives/DrivePendingTiles.tsx
  • web/oss/src/components/Drives/DriveToolbar.tsx
  • web/oss/src/components/Drives/DriveTreeList.tsx
  • web/oss/src/components/Drives/DriveTreePane.tsx
  • web/oss/src/components/Drives/DriveTreeRow.tsx
  • web/oss/src/components/Drives/FolderTile.tsx
  • web/oss/src/components/Drives/FolderView.tsx
  • web/oss/src/components/Drives/driveMotion.ts
  • web/oss/src/components/Drives/driveTreeView.ts
  • web/oss/src/components/Drives/driveTypes.ts
  • web/oss/src/components/Drives/useDelayedTrue.ts
  • web/oss/src/components/Drives/useDriveDownloadAll.ts
  • web/oss/src/components/Drives/useDriveFilters.ts
  • web/oss/src/components/Drives/useDriveSelection.ts
  • web/oss/src/components/Drives/useDriveTreeData.ts
  • web/oss/src/components/Drives/useDriveTreeKeyboard.ts
  • web/oss/src/components/Drives/useDriveTreePane.ts
  • web/oss/src/components/Drives/useDriveTreeReveal.ts
  • web/oss/src/components/Drives/useDriveTreeViewport.ts
  • web/oss/src/components/Drives/useDriveUploads.ts
  • web/oss/src/components/Drives/useTreeGroupScroll.ts

Comment thread web/oss/src/components/Drives/DriveFileContentViewer.tsx
Comment thread web/oss/src/components/Drives/DriveHeader.tsx
Comment thread web/oss/src/components/Drives/DriveToolbar.tsx
Comment on lines +34 to +48
const handleDownloadAll = useCallback(async () => {
if (!archiveMounts.length || downloadingAll) return
setDownloadingAll(true)
const key = "drive-download-all"
message.open({type: "loading", key, content: "Preparing download…", duration: 0})
const result = await downloadMountArchive({
mounts: archiveMounts,
projectId,
filename: `${driveRootLabel(drive.mount)}-files.zip`,
})
if (result.cancelled) message.destroy(key)
else if (result.ok) message.open({type: "success", key, content: "Download ready"})
else message.open({type: "error", key, content: result.error ?? "Download failed"})
setDownloadingAll(false)
}, [archiveMounts, drive.mount, projectId, downloadingAll, message])

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Wrap the await in try/finally — a throw permanently wedges the button and leaves the toast up.

downloadMountArchive is not exception-free: in the streaming path await handle.createWritable() sits outside its inner try (web/oss/src/components/Drives/driveMedia.ts lines 225-308), so a rejection propagates here. downloadingAll then stays true forever (guard at Line 35 blocks all retries) and the duration: 0 loading toast never closes.

🛡️ Proposed fix
-        const result = await downloadMountArchive({
-            mounts: archiveMounts,
-            projectId,
-            filename: `${driveRootLabel(drive.mount)}-files.zip`,
-        })
-        if (result.cancelled) message.destroy(key)
-        else if (result.ok) message.open({type: "success", key, content: "Download ready"})
-        else message.open({type: "error", key, content: result.error ?? "Download failed"})
-        setDownloadingAll(false)
+        try {
+            const result = await downloadMountArchive({
+                mounts: archiveMounts,
+                projectId,
+                filename: `${driveRootLabel(drive.mount)}-files.zip`,
+            })
+            if (result.cancelled) message.destroy(key)
+            else if (result.ok) message.open({type: "success", key, content: "Download ready"})
+            else message.open({type: "error", key, content: result.error ?? "Download failed"})
+        } catch {
+            message.open({type: "error", key, content: "Download failed"})
+        } finally {
+            setDownloadingAll(false)
+        }
📝 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.

Suggested change
const handleDownloadAll = useCallback(async () => {
if (!archiveMounts.length || downloadingAll) return
setDownloadingAll(true)
const key = "drive-download-all"
message.open({type: "loading", key, content: "Preparing download…", duration: 0})
const result = await downloadMountArchive({
mounts: archiveMounts,
projectId,
filename: `${driveRootLabel(drive.mount)}-files.zip`,
})
if (result.cancelled) message.destroy(key)
else if (result.ok) message.open({type: "success", key, content: "Download ready"})
else message.open({type: "error", key, content: result.error ?? "Download failed"})
setDownloadingAll(false)
}, [archiveMounts, drive.mount, projectId, downloadingAll, message])
const handleDownloadAll = useCallback(async () => {
if (!archiveMounts.length || downloadingAll) return
setDownloadingAll(true)
const key = "drive-download-all"
message.open({type: "loading", key, content: "Preparing download…", duration: 0})
try {
const result = await downloadMountArchive({
mounts: archiveMounts,
projectId,
filename: `${driveRootLabel(drive.mount)}-files.zip`,
})
if (result.cancelled) message.destroy(key)
else if (result.ok) message.open({type: "success", key, content: "Download ready"})
else message.open({type: "error", key, content: result.error ?? "Download failed"})
} catch {
message.open({type: "error", key, content: "Download failed"})
} finally {
setDownloadingAll(false)
}
}, [archiveMounts, drive.mount, projectId, downloadingAll, message])

Comment thread web/oss/src/components/Drives/useDriveSelection.ts Outdated
Comment thread web/oss/src/components/Drives/useDriveTreeData.ts Outdated
Report single-file download failures. Four call sites dropped
downloadMountFile's `false`, so a failed click looked identical to a
successful one. Lift the one correct implementation (the context menu's)
into useDriveFileDownload, route every site through it, give the shared
DriveFileDownloadButton a loading state, drop DriveHeader's duplicate
button, and un-export downloadMountFile so the pattern can't return.

Stop downloadMountArchive from throwing past its result contract:
handle.createWritable() sat outside the try, so a refused write
permission escaped every caller's result handling — stranding the
download-all button (its in-flight flag gates retries) and the folder-zip
toast. Also reset the flag in `finally`.

Key drive selection persistence on a real mount id. drive.mount resolves
async, so early renders keyed the selection atom on "" — a slot shared by
every mountless drive — and the root-landing effect wrote to it. Read ""
as empty, never write it, adopt the drive's persisted selection (or flush
a pre-mount pick) once the id lands, and stop auto-landing on root from
overwriting persistence. The local-file drive has no mount at all, so the
gate is on persistence, not on selection.

Derive the selection's folder-ness from the backend's is_folder instead
of its name. The name guess made extensionless files (LICENSE,
Dockerfile) issue a needless directory query; it now covers only the cold
window before any level lands, corrected during render so a wrong guess
never mounts a subscriber. The heuristic itself is one shared helper —
DriveExplorer's skeleton held a second, divergent copy.

Fix the inverted aria-pressed on the hidden-files toggle and align all
three toolbar toggles on one convention: a static label naming the
control, aria-pressed carrying the state, dynamic wording in the tooltip.
Drop the overflow menu's leading divider when there are no drive ids.
@ardaerzin

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 31, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (3)
web/oss/src/components/Drives/DriveExplorer.tsx (3)

238-239: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Include local and in-flight content in the empty-state decision.

FolderView receives explicitFiles, stagedItems, and pending-upload data, but this branch returns DriveEmptyState solely from drive.fileCount. An empty backend drive with staged or pending files therefore hides the grid and the upload controls. Base this branch on effective content, not only persisted drive.fileCount.


334-335: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Expose partial-error retry and staged-upload actions in embedded mode.

DriveExplorer.render only renders FolderView directly when onClose is absent, so drive.partialErrored/retry and staged commit (onUploadStaged) stay hidden in headerless/embedded use. Keep retry/staged actions available in headerless mode, for example by rendering them in FolderView.


161-172: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Pass an explicit kind for unloaded Quick Look targets.

DriveExplorer.initialPath only carries the target path; selectedIsFolder falls back to false until nodeByPath loads it, so an unloaded folder selection enters DriveFilePreview and staged uploads default to the parent folder. Store/provide the file/folder kind in the drawer state, or make Quick Look callers pass folder paths explicitly as browse targets.

🧹 Nitpick comments (1)
web/oss/src/components/Drives/DriveFileContentViewer.tsx (1)

56-57: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Shorten the exported component comment to one line.

This JSDoc describes normal behavior and spans two source lines. Keep it concise.

Proposed change
-/** Download button for one file — raw bytes, so every type round-trips (not just text). Shared by
- * the drawer header and the file preview, so both report a failure the same way. */
+/** Downloads raw file bytes through the shared drive download hook. */

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


ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: f1bd1e72-c039-4760-bb30-25f8e55f57af

📥 Commits

Reviewing files that changed from the base of the PR and between bbb64a8 and 6c98cc5.

📒 Files selected for processing (12)
  • web/oss/src/components/Drives/DriveExplorer.tsx
  • web/oss/src/components/Drives/DriveFileCard.tsx
  • web/oss/src/components/Drives/DriveFileContentViewer.tsx
  • web/oss/src/components/Drives/DriveHeader.tsx
  • web/oss/src/components/Drives/DriveItemContextMenu.tsx
  • web/oss/src/components/Drives/DriveToolbar.tsx
  • web/oss/src/components/Drives/driveFileSource.tsx
  • web/oss/src/components/Drives/driveMedia.ts
  • web/oss/src/components/Drives/driveTreeView.ts
  • web/oss/src/components/Drives/useDriveDownloadAll.ts
  • web/oss/src/components/Drives/useDriveSelection.ts
  • web/oss/src/components/Drives/useDriveTreeData.ts
🚧 Files skipped from review as they are similar to previous changes (3)
  • web/oss/src/components/Drives/DriveToolbar.tsx
  • web/oss/src/components/Drives/useDriveDownloadAll.ts
  • web/oss/src/components/Drives/DriveHeader.tsx

Comment on lines +220 to +242
/** `download(mount, path)` for ONE drive file, reporting the outcome through the themed toast
* (App.useApp, so it renders correctly in dark mode). THE way to trigger a single-file download:
* {@link downloadMountFile} resolves `false` on failure, so a bare fire-and-forget call left a failed
* click looking exactly like a successful one. Stable — safe to pass down to list items. */
export function useDriveFileDownload(): (mount: Mount | null, path: string) => Promise<boolean> {
const {message} = App.useApp()
const projectId = useAtomValue(projectIdAtom)
return useCallback(
async (mount: Mount | null, path: string) => {
const key = `drive-download:${path}`
message.open({type: "loading", key, content: "Downloading…", duration: 0})
const ok = await downloadMountFile({mount, path, projectId})
message.open(
ok
? {type: "success", key, content: "Downloaded"}
: {type: "error", key, content: "Download failed"},
)
return ok
},
[message, projectId],
)
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Include the mount identity in the notification key.

The key uses only path, but the callback also accepts mount. The cwd mount and the agent-files mount can contain the same relative path. Concurrent downloads can then replace each other’s loading and completion messages. Include mount?.id in the key.

Proposed fix
-            const key = `drive-download:${path}`
+            const key = `drive-download:${mount?.id ?? "none"}:${path}`
📝 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.

Suggested change
/** `download(mount, path)` for ONE drive file, reporting the outcome through the themed toast
* (App.useApp, so it renders correctly in dark mode). THE way to trigger a single-file download:
* {@link downloadMountFile} resolves `false` on failure, so a bare fire-and-forget call left a failed
* click looking exactly like a successful one. Stable safe to pass down to list items. */
export function useDriveFileDownload(): (mount: Mount | null, path: string) => Promise<boolean> {
const {message} = App.useApp()
const projectId = useAtomValue(projectIdAtom)
return useCallback(
async (mount: Mount | null, path: string) => {
const key = `drive-download:${path}`
message.open({type: "loading", key, content: "Downloading…", duration: 0})
const ok = await downloadMountFile({mount, path, projectId})
message.open(
ok
? {type: "success", key, content: "Downloaded"}
: {type: "error", key, content: "Download failed"},
)
return ok
},
[message, projectId],
)
}
/** `download(mount, path)` for ONE drive file, reporting the outcome through the themed toast
* (App.useApp, so it renders correctly in dark mode). THE way to trigger a single-file download:
* {`@link` downloadMountFile} resolves `false` on failure, so a bare fire-and-forget call left a failed
* click looking exactly like a successful one. Stable safe to pass down to list items. */
export function useDriveFileDownload(): (mount: Mount | null, path: string) => Promise<boolean> {
const {message} = App.useApp()
const projectId = useAtomValue(projectIdAtom)
return useCallback(
async (mount: Mount | null, path: string) => {
const key = `drive-download:${mount?.id ?? "none"}:${path}`
message.open({type: "loading", key, content: "Downloading…", duration: 0})
const ok = await downloadMountFile({mount, path, projectId})
message.open(
ok
? {type: "success", key, content: "Downloaded"}
: {type: "error", key, content: "Download failed"},
)
return ok
},
[message, projectId],
)
}

Comment on lines +65 to +79
// Once the drive's id resolves: adopt its persisted selection (the lazy initializers above ran
// before mount discovery finished, so they couldn't see it) — or, if a pick was already made
// against no mount, flush that pick into the now-real slot. Runs once.
const adoptedRef = useRef(hasMount)
useEffect(() => {
if (!hasMount || adoptedRef.current) return
adoptedRef.current = true
if (chosenRef.current) {
setPersistedSelection(selectedPath)
return
}
if (!persistedSelection) return
setSelectedPath(persistedSelection)
expand(persistedSelection)
}, [hasMount, selectedPath, persistedSelection, setPersistedSelection, expand])

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

ast-grep outline web/oss/src/components/Drives/DriveExplorer.tsx --view expanded
rg -n -C 5 --glob '*.ts' --glob '*.tsx' '<DriveExplorer|DriveExplorer\(' web

Repository: Agenta-AI/agenta

Length of output: 4125


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== FilesDrawer relevant section =="
sed -n '60,110p' web/oss/src/components/Drives/FilesDrawer.tsx

echo
echo "== DriveExplorer mount/key/projection around DriveExplorer and usesDriveSelection call =="
sed -n '220,330p' web/oss/src/components/Drives/DriveExplorer.tsx

echo
echo "== useDriveSelection source =="
sed -n '1,130p' web/oss/src/components/Drives/useDriveSelection.ts

Repository: Agenta-AI/agenta

Length of output: 12221


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== DriveExplorer hook call site and props =="
rg -n -C 8 --glob '*.tsx' --glob '*.ts' 'useDriveSelection\(|Key=|key=|mountId|drive=' web/oss/src/components/Drives/DriveExplorer.tsx web/oss/src/components/Drives/FilesDrawer.tsx

echo
echo "== DriveExplorer full outlines around useState/useDriveSelection/dynamic mount key =="
ast-grep outline web/oss/src/components/Drives/DriveExplorer.tsx --view expanded

Repository: Agenta-AI/agenta

Length of output: 7167


Reset mount-scoped state when mountId changes.

After one discovery completes, adoptedRef.current stays true, so a later render with a different non-empty mount reuses the previous mount’s persisted selection and expanded folders with the new mount’s tree. Reset or remount selectedPath, expanded, the Jotai selection state, and persist any former pick into the old mount slot before adopting the new mount.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Frontend refactoring A code change that neither fixes a bug nor adds a feature size:XXL This PR changes 1000+ lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants