[fix] Give the config Files drawer its own drive scope [#6388] - #6393
Conversation
The drive it browses arrives as a prop, but the per-file actions inside it read the session and artifact ids from DriveSessionContext — and the only providers were the chat surfaces (AgentChatPanel, AgentConversation, mobile's SessionWorkspace). On a configuration page with no conversation open there was no provider in the tree at all, so every file resolved to no mount and the section did nothing (#6388). StorageSection already resolves both ids through useConfigDrive, and the artifact one does not need a session to exist, so it now provides them around its own drawer instead of hoping an ancestor supplies them.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 SummarySummary by CodeRabbit
Walkthrough
ChangesConfiguration file access
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: 🔵 Low · up to The change enables file previews and downloads from an agent configuration page without an active session. It is mergeable with explicit owner awareness: update the drawer metadata to use the resolved session ID, and confirm that existing backend ownership checks cover the newly reachable file operations. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: e8a891a7-9297-4c2e-94ab-a1712e67ae5d
📒 Files selected for processing (1)
web/packages/agenta-entity-ui/src/drive/StorageSection.tsx
Included review availability: Your plan provides up to 8 included reviews per hour; 6 remain after this review.
| scope: string | ||
| }) { | ||
| const {drive} = useConfigDrive(revisionId, sessionId) | ||
| const {drive, sessionId: resolvedSessionId, artifactId} = useConfigDrive(revisionId, sessionId) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use resolvedSessionId for driveIds.
The provider uses resolvedSessionId, but driveIds still uses sessionId at Line 130. Without an open conversation, the drawer header can omit the resolved session owner ID. Build driveIds from resolvedSessionId and update the useMemo dependency.
Proposed fix
- sessionId ? {key: "owner", label: "Session ID", value: sessionId} : null,
+ resolvedSessionId
+ ? {key: "owner", label: "Session ID", value: resolvedSessionId}
+ : null,
...
- [drive.mount?.id, sessionId],
+ [drive.mount?.id, resolvedSessionId],| Its own DriveSessionProvider because the drive it browses is THIS section's, not an | ||
| ancestor's. The listing arrives as a prop, but the per-file actions inside read the | ||
| ids from context — and the only providers were the chat surfaces, so on a | ||
| configuration page with no conversation open there was no context at all and the | ||
| files resolved to no mount (#6388). `useConfigDrive` already resolved both ids from | ||
| the edited revision; the artifact one does not need a session to exist. */} |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Shorten the added JSX comment to one line.
Lines 279-284 add a multi-line comment. Replace it with one short line.
As per coding guidelines: “Hard rule. At most ONE short line per comment.”
Source: Coding guidelines
2 participants
Fixes #6388.
Context
Open an agent's configuration page when no session is running, go to the Files section, and nothing works.
The listing itself is fine.
StorageSectionresolves the drive throughuseConfigDriveand hands it to the drawer as a prop, anduseSessionDriveSummaryalready models the no-session case: with no cwd mount it treats the agent's durable folder as the whole drive.The per-file actions are the problem. Inside the drawer,
DriveFileCardand friends read the session and artifact ids fromDriveSessionContext, and the only components that ever provided it are the chat surfaces:AgentChatPanel,AgentConversation, and mobile'sSessionWorkspace. On a configuration page with no conversation open there is no provider anywhere in the tree, so every file resolved to no mount.Changes
StorageSectionnow provides the drive scope around its own drawer instead of hoping an ancestor supplies it. It already had both ids fromuseConfigDrive, and the artifact id does not need a session to exist.The surface that resolves the drive is now the one that declares it.
Tests / notes
@agenta/entity-uiunit suite passes (598), lint and both app typechecks clean.StorageSectionon that page./mthis path already had an ancestor provider, so the bug never reproduced there. The nested provider resolves the same two ids, so it should be a no-op on mobile. Worth a click-through on/m's config Files section before merging, since that is the case this could regress.What to QA
/m, open the config pane's Files section, open a file. It behaves as before.