feat(frontend): @agenta/entity-ui/drive — the drive explorer, tree and file surfaces leave the app - #5877
feat(frontend): @agenta/entity-ui/drive — the drive explorer, tree and file surfaces leave the app#5877ardaerzin 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 components under shared Agenta packages. It adds session and renderer APIs, updates storage and messaging flows, introduces ChangesDrive UI centralization
Estimated code review effort: 4 (Complex) | ~45 minutes 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 |
6ac5beb to
e2d62a8
Compare
9519345 to
892272f
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/packages/agenta-entity-ui/src/drive/StorageSection.tsx (1)
87-106: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winIsolate config Files drawer state by session.
AgentOperationsSectionsforwards the sameStorageSectionelement forrevisionIdacross agents, but that component now selects a differentsessionIddrive viauseConfigDrive. Its drawer atom is still keyed only torevisionId, so staging andinitialPathcan persist when the passed session changes. Include the active session in the drawer state or reset the drawer whensessionIdchanges.
🧹 Nitpick comments (4)
web/packages/agenta-entity-ui/src/drive/VirtualTileGrid.tsx (3)
1-60: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTrim the comment volume to match the repository comment rule.
The guidelines allow long comments only for genuinely surprising constraints. Several blocks here explain routine layout math and rejected alternatives (for example lines 18-19 and 43-48). Keep the setState-in-render and spring-window notes, which are genuine ordering constraints. Move the design rationale and history to the PR description or an ADR, and reduce the rest to one short line each.
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
277-293: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winGuard the focus retry loop against overlap and unmount.
Every
focusCellcall starts an independentrequestAnimationFramechain. A held arrow key starts several chains that compete for focus, and the last one to resolve wins. No chain is cancelled when the component unmounts.Store the frame id in a ref, cancel the previous chain before starting a new one, and cancel on unmount.
♻️ Proposed refactor
+ const focusFrame = useRef<number | null>(null) + useEffect(() => () => { + if (focusFrame.current !== null) cancelAnimationFrame(focusFrame.current) + }, []) const focusCell = (index: number) => { if (!items.length) return const target = Math.min(Math.max(index, 0), items.length - 1) virtualizer.scrollToIndex(Math.floor(target / cols), {align: "auto"}) + if (focusFrame.current !== null) cancelAnimationFrame(focusFrame.current) let tries = 0 const run = () => { const cell = parentRef.current?.querySelector<HTMLElement>( `[data-grid-cell="${target}"]`, ) const focusable = cell?.querySelector<HTMLElement>( "button, [href], [tabindex]:not([tabindex='-1'])", ) if (focusable) focusable.focus() - else if (tries++ < 4) requestAnimationFrame(run) + else if (tries++ < 4) focusFrame.current = requestAnimationFrame(run) } - requestAnimationFrame(run) + focusFrame.current = requestAnimationFrame(run) }
355-406: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider exposing grid semantics to assistive technology.
The container is focusable and implements 2D roving focus, but the DOM carries no grid semantics.
data-grid-cellis a query hook only. Screen readers announce a plain scrollable region, so the row and column position is not available.Add
role="grid"on the scroll container, androle="gridcell"plusaria-colindexandaria-rowindexon eachmotion.div. Also setaria-rowcount={rowCount}andaria-colcount={cols}. Verify with a screen reader before merge, because the absolutely positioned tiles have no intermediaterole="row"element and some readers require one.web/packages/agenta-entity-ui/src/drive/ContextRail.tsx (1)
35-45: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared secondary-text component.
Texthas the same implementation in three drive modules. Move it to one shared drive UI module and import it at each site.
web/packages/agenta-entity-ui/src/drive/ContextRail.tsx#L35-L45: replace the localTextdeclaration with the shared component.web/packages/agenta-entity-ui/src/drive/DriveFileCard.tsx#L23-L33: replace the localTextdeclaration with the shared component.web/packages/agenta-entity-ui/src/drive/DriveTreeList.tsx#L19-L29: replace the localTextdeclaration with the shared component.As per coding guidelines, “Extract reusable components only when used in three or more places.”
Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 3dc61029-fd15-46ef-b950-9814652ca9b2
📒 Files selected for processing (44)
web/oss/src/components/AgentChatSlice/components/AttachmentViewerDrawer.tsxweb/oss/src/components/Drives/chatFileRefs.tsxweb/oss/src/components/Drives/useChatScopeSessionId.tsweb/oss/src/components/pages/overview/agent/AgentFilesCard.tsxweb/packages/agenta-entity-ui/src/drive/ContextRail.tsxweb/packages/agenta-entity-ui/src/drive/DriveBreadcrumb.tsxweb/packages/agenta-entity-ui/src/drive/DriveExplorer.tsxweb/packages/agenta-entity-ui/src/drive/DriveExplorerSkeleton.tsxweb/packages/agenta-entity-ui/src/drive/DriveExplorerStates.tsxweb/packages/agenta-entity-ui/src/drive/DriveFileCard.tsxweb/packages/agenta-entity-ui/src/drive/DriveFileContentViewer.tsxweb/packages/agenta-entity-ui/src/drive/DriveFilePreview.tsxweb/packages/agenta-entity-ui/src/drive/DriveFileRow.tsxweb/packages/agenta-entity-ui/src/drive/DriveHeader.tsxweb/packages/agenta-entity-ui/src/drive/DriveItemContextMenu.tsxweb/packages/agenta-entity-ui/src/drive/DrivePendingTiles.tsxweb/packages/agenta-entity-ui/src/drive/DriveToolbar.tsxweb/packages/agenta-entity-ui/src/drive/DriveTreeList.tsxweb/packages/agenta-entity-ui/src/drive/DriveTreePane.tsxweb/packages/agenta-entity-ui/src/drive/DriveTreeRow.tsxweb/packages/agenta-entity-ui/src/drive/FileThumb.tsxweb/packages/agenta-entity-ui/src/drive/FilesDrawer.tsxweb/packages/agenta-entity-ui/src/drive/FolderTile.tsxweb/packages/agenta-entity-ui/src/drive/FolderView.tsxweb/packages/agenta-entity-ui/src/drive/OriginTag.tsxweb/packages/agenta-entity-ui/src/drive/SessionFilesDrawer.tsxweb/packages/agenta-entity-ui/src/drive/StorageFilesHeader.tsxweb/packages/agenta-entity-ui/src/drive/StorageSection.tsxweb/packages/agenta-entity-ui/src/drive/VirtualTileGrid.tsxweb/packages/agenta-entity-ui/src/drive/driveFileSource.tsxweb/packages/agenta-entity-ui/src/drive/driveIcons.tsxweb/packages/agenta-entity-ui/src/drive/driveMarkdown.tsxweb/packages/agenta-entity-ui/src/drive/driveSessionContext.tsxweb/packages/agenta-entity-ui/src/drive/fileMeta.tsxweb/packages/agenta-entity-ui/src/drive/index.tsweb/packages/agenta-entity-ui/src/drive/quickLook.tsxweb/packages/agenta-entity-ui/src/drive/renderers.tsxweb/packages/agenta-entity-ui/src/drive/repoMeta.tsxweb/packages/agenta-entity-ui/src/drive/useDriveDownloadAll.tsweb/packages/agenta-entity-ui/src/drive/useDriveFileDownload.tsweb/packages/agenta-entity-ui/src/drive/useDriveTreeData.tsweb/packages/agenta-entity-ui/src/drive/useLazyDriveTree.tsxweb/packages/agenta-entity-ui/src/drive/useUploadReveal.tsweb/packages/agenta-entity-ui/tests/unit/useUploadReveal.test.ts
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/packages/agenta-entity-ui/src/drive/StorageSection.tsx (1)
87-106: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winIsolate config Files drawer state by session.
AgentOperationsSectionsforwards the sameStorageSectionelement forrevisionIdacross agents, but that component now selects a differentsessionIddrive viauseConfigDrive. Its drawer atom is still keyed only torevisionId, so staging andinitialPathcan persist when the passed session changes. Include the active session in the drawer state or reset the drawer whensessionIdchanges.
🧹 Nitpick comments (4)
web/packages/agenta-entity-ui/src/drive/VirtualTileGrid.tsx (3)
1-60: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTrim the comment volume to match the repository comment rule.
The guidelines allow long comments only for genuinely surprising constraints. Several blocks here explain routine layout math and rejected alternatives (for example lines 18-19 and 43-48). Keep the setState-in-render and spring-window notes, which are genuine ordering constraints. Move the design rationale and history to the PR description or an ADR, and reduce the rest to one short line each.
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
277-293: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winGuard the focus retry loop against overlap and unmount.
Every
focusCellcall starts an independentrequestAnimationFramechain. A held arrow key starts several chains that compete for focus, and the last one to resolve wins. No chain is cancelled when the component unmounts.Store the frame id in a ref, cancel the previous chain before starting a new one, and cancel on unmount.
♻️ Proposed refactor
+ const focusFrame = useRef<number | null>(null) + useEffect(() => () => { + if (focusFrame.current !== null) cancelAnimationFrame(focusFrame.current) + }, []) const focusCell = (index: number) => { if (!items.length) return const target = Math.min(Math.max(index, 0), items.length - 1) virtualizer.scrollToIndex(Math.floor(target / cols), {align: "auto"}) + if (focusFrame.current !== null) cancelAnimationFrame(focusFrame.current) let tries = 0 const run = () => { const cell = parentRef.current?.querySelector<HTMLElement>( `[data-grid-cell="${target}"]`, ) const focusable = cell?.querySelector<HTMLElement>( "button, [href], [tabindex]:not([tabindex='-1'])", ) if (focusable) focusable.focus() - else if (tries++ < 4) requestAnimationFrame(run) + else if (tries++ < 4) focusFrame.current = requestAnimationFrame(run) } - requestAnimationFrame(run) + focusFrame.current = requestAnimationFrame(run) }
355-406: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider exposing grid semantics to assistive technology.
The container is focusable and implements 2D roving focus, but the DOM carries no grid semantics.
data-grid-cellis a query hook only. Screen readers announce a plain scrollable region, so the row and column position is not available.Add
role="grid"on the scroll container, androle="gridcell"plusaria-colindexandaria-rowindexon eachmotion.div. Also setaria-rowcount={rowCount}andaria-colcount={cols}. Verify with a screen reader before merge, because the absolutely positioned tiles have no intermediaterole="row"element and some readers require one.web/packages/agenta-entity-ui/src/drive/ContextRail.tsx (1)
35-45: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared secondary-text component.
Texthas the same implementation in three drive modules. Move it to one shared drive UI module and import it at each site.
web/packages/agenta-entity-ui/src/drive/ContextRail.tsx#L35-L45: replace the localTextdeclaration with the shared component.web/packages/agenta-entity-ui/src/drive/DriveFileCard.tsx#L23-L33: replace the localTextdeclaration with the shared component.web/packages/agenta-entity-ui/src/drive/DriveTreeList.tsx#L19-L29: replace the localTextdeclaration with the shared component.As per coding guidelines, “Extract reusable components only when used in three or more places.”
Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 3dc61029-fd15-46ef-b950-9814652ca9b2
📒 Files selected for processing (44)
web/oss/src/components/AgentChatSlice/components/AttachmentViewerDrawer.tsxweb/oss/src/components/Drives/chatFileRefs.tsxweb/oss/src/components/Drives/useChatScopeSessionId.tsweb/oss/src/components/pages/overview/agent/AgentFilesCard.tsxweb/packages/agenta-entity-ui/src/drive/ContextRail.tsxweb/packages/agenta-entity-ui/src/drive/DriveBreadcrumb.tsxweb/packages/agenta-entity-ui/src/drive/DriveExplorer.tsxweb/packages/agenta-entity-ui/src/drive/DriveExplorerSkeleton.tsxweb/packages/agenta-entity-ui/src/drive/DriveExplorerStates.tsxweb/packages/agenta-entity-ui/src/drive/DriveFileCard.tsxweb/packages/agenta-entity-ui/src/drive/DriveFileContentViewer.tsxweb/packages/agenta-entity-ui/src/drive/DriveFilePreview.tsxweb/packages/agenta-entity-ui/src/drive/DriveFileRow.tsxweb/packages/agenta-entity-ui/src/drive/DriveHeader.tsxweb/packages/agenta-entity-ui/src/drive/DriveItemContextMenu.tsxweb/packages/agenta-entity-ui/src/drive/DrivePendingTiles.tsxweb/packages/agenta-entity-ui/src/drive/DriveToolbar.tsxweb/packages/agenta-entity-ui/src/drive/DriveTreeList.tsxweb/packages/agenta-entity-ui/src/drive/DriveTreePane.tsxweb/packages/agenta-entity-ui/src/drive/DriveTreeRow.tsxweb/packages/agenta-entity-ui/src/drive/FileThumb.tsxweb/packages/agenta-entity-ui/src/drive/FilesDrawer.tsxweb/packages/agenta-entity-ui/src/drive/FolderTile.tsxweb/packages/agenta-entity-ui/src/drive/FolderView.tsxweb/packages/agenta-entity-ui/src/drive/OriginTag.tsxweb/packages/agenta-entity-ui/src/drive/SessionFilesDrawer.tsxweb/packages/agenta-entity-ui/src/drive/StorageFilesHeader.tsxweb/packages/agenta-entity-ui/src/drive/StorageSection.tsxweb/packages/agenta-entity-ui/src/drive/VirtualTileGrid.tsxweb/packages/agenta-entity-ui/src/drive/driveFileSource.tsxweb/packages/agenta-entity-ui/src/drive/driveIcons.tsxweb/packages/agenta-entity-ui/src/drive/driveMarkdown.tsxweb/packages/agenta-entity-ui/src/drive/driveSessionContext.tsxweb/packages/agenta-entity-ui/src/drive/fileMeta.tsxweb/packages/agenta-entity-ui/src/drive/index.tsweb/packages/agenta-entity-ui/src/drive/quickLook.tsxweb/packages/agenta-entity-ui/src/drive/renderers.tsxweb/packages/agenta-entity-ui/src/drive/repoMeta.tsxweb/packages/agenta-entity-ui/src/drive/useDriveDownloadAll.tsweb/packages/agenta-entity-ui/src/drive/useDriveFileDownload.tsweb/packages/agenta-entity-ui/src/drive/useDriveTreeData.tsweb/packages/agenta-entity-ui/src/drive/useLazyDriveTree.tsxweb/packages/agenta-entity-ui/src/drive/useUploadReveal.tsweb/packages/agenta-entity-ui/tests/unit/useUploadReveal.test.ts
🛑 Comments failed to post (2)
web/packages/agenta-entity-ui/src/drive/VirtualTileGrid.tsx (2)
153-159: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
An announced shift can be dropped silently.
Two paths advance
shiftSeqwithout freezing the layout:
- If
widthis still0(first render, before measurement), line 157 skips the freeze. The announcement is consumed and never applied.shiftSeqinitializes fromanticipateShift?.seq ?? 0. If the host mounts the grid withanticipateShift === nulland then announces{delta, seq: 0}, the comparison at line 155 sees equal values and ignores the shift.In both cases the grid derives columns from the sweeping live width, which is the behavior the prop exists to prevent. Seed the sequence with a sentinel, and defer the pending shift until a width exists.
🐛 Proposed fix
const [frozenWidth, setFrozenWidth] = useState<number | null>(null) -const [shiftSeq, setShiftSeq] = useState(anticipateShift?.seq ?? 0) -if (anticipateShift && anticipateShift.seq !== shiftSeq) { +const [shiftSeq, setShiftSeq] = useState<number | null>(null) +if (anticipateShift && anticipateShift.seq !== shiftSeq && width > 0) { setShiftSeq(anticipateShift.seq) - if (width > 0) setFrozenWidth((frozenWidth ?? width) + anticipateShift.delta) + setFrozenWidth((frozenWidth ?? width) + anticipateShift.delta) }📝 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 [frozenWidth, setFrozenWidth] = useState<number | null>(null) const [shiftSeq, setShiftSeq] = useState<number | null>(null) if (anticipateShift && anticipateShift.seq !== shiftSeq && width > 0) { setShiftSeq(anticipateShift.seq) setFrozenWidth((frozenWidth ?? width) + anticipateShift.delta) } const arrived = frozenWidth !== null && width > 0 && Math.abs(width - frozenWidth) < 2
308-343: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
The handler steals arrow keys, Home, and End from nested text fields.
handleKeyDownsits on the scroll container, so it receives bubbled events from every descendant. If a tile renders aninputortextarea(inline rename, in-tile filter), pressing ArrowLeft, ArrowRight, Home, or End moves grid focus ande.preventDefault()at line 328 blocks caret movement. The same applies to Cmd/Ctrl+ArrowDown at line 310.The autoFocus effect already bails on an active text field at line 303. Apply the same guard here.
🐛 Proposed fix
const handleKeyDown = (e: KeyboardEvent<HTMLDivElement>) => { + // Never hijack keys from an editable descendant (inline rename, in-tile search). + const t = e.target as HTMLElement | null + if (t && (/^(input|textarea|select)$/i.test(t.tagName) || t.isContentEditable)) return // Finder-style step in/out (Cmd on macOS, Ctrl elsewhere). if ((e.metaKey || e.ctrlKey) && (e.key === "ArrowDown" || e.key === "ArrowUp")) {📝 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 handleKeyDown = (e: KeyboardEvent<HTMLDivElement>) => { // Never hijack keys from an editable descendant. const t = e.target as HTMLElement | null if (t && (/^(input|textarea|select)$/i.test(t.tagName) || t.isContentEditable)) return // Finder-style step in/out (Cmd on macOS, Ctrl elsewhere). if ((e.metaKey || e.ctrlKey) && (e.key === "ArrowDown" || e.key === "ArrowUp")) { if (e.key === "ArrowDown") { const cur = focusedIndex() if (cur >= 0 && onMetaActivate) { e.preventDefault() onMetaActivate(items[cur], cur) } } else if (onMetaBack) { e.preventDefault() onMetaBack() } return } if (!NAV_KEYS.includes(e.key)) { onKeyDown?.(e) return } const cur = focusedIndex() e.preventDefault() switch (e.key) { case "Home": return focusCell(0) case "End": return focusCell(items.length - 1) case "ArrowLeft": return focusCell(cur < 0 ? 0 : cur - 1) case "ArrowRight": return focusCell(cur < 0 ? 0 : cur + 1) case "ArrowUp": return focusCell(cur < 0 ? 0 : cur - cols) case "ArrowDown": return focusCell(cur < 0 ? 0 : cur + cols) } }
…d file surfaces leave the app
e2d62a8 to
c185e3e
Compare
892272f to
6f21584
Compare
1 participant
The rendered half of the drive: the explorer, the tree and the file surfaces, on top of the
headless layer from the lane below.
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/entities-drive; review only this lane's diff.