feat(frontend): browse and preview mount files in the session inspector - #5204
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI 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:
📝 WalkthroughWalkthroughThe SessionInspector Mounts tab now browses mount files through new frontend fetchers, derives one-level folder/file rows, previews supported text and image files, and downloads unsupported or oversized files. Documentation records the design, implementation status, research, tests, QA steps, and deferred issues. ChangesMount file viewer
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant MountsTab
participant SessionInspectorAPI
participant MountsRouter
User->>MountsTab: expand mount or select file
MountsTab->>SessionInspectorAPI: fetch listing or preview data
SessionInspectorAPI->>MountsRouter: call mount file endpoint
MountsRouter-->>SessionInspectorAPI: return entries, text, or bytes
SessionInspectorAPI-->>MountsTab: provide data
MountsTab-->>User: render rows, preview, or download
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
mmabrouk
left a comment
mmabrouk
left a comment
There was a problem hiding this comment.
Author notes on the non-obvious hunks, for reviewers. Live QA evidence: all 8 acceptance checks pass on the dev box (root listing shape, folder nav + breadcrumb, text preview, image preview, 2 MB cap on a 3.3 MB avi, download fallback for mp3, console clean, dark mode), verified on a real session whose cwd mount holds nested test assets.
| * `is_folder` marker rows for a direct child folder are merged into the same synthetic set | ||
| * so a folder never appears twice. | ||
| */ | ||
| export const deriveRows = (files: MountFileEntry[], currentPath: string): BrowserRow[] => { |
There was a problem hiding this comment.
Why this function exists at all: the backend has no one-level listing mode. GET /mounts/{id}/files walks the prefix recursively (list_objects(recursive=True), no delimiter) and returns a flat list with every path relative to the mount root. Folder rows only appear for explicit marker objects, and agent-written files (geesefs) mostly have none. All three bugs found during review and live QA lived in this derivation, and each is pinned as a regression test in mountBrowser.test.ts.
| if (!relative) continue // marker entry for the current path itself | ||
|
|
||
| const isMarker = entry.is_folder || relative.endsWith("/") | ||
| const cleanRelative = relative.endsWith("/") ? relative.slice(0, -1) : relative |
There was a problem hiding this comment.
This line pair is the subtle part. is_folder: true entries arrive WITHOUT a trailing slash, so folder detection must read the flag (an earlier cut keyed it on the slash alone: empty folders rendered as files and 404'd on click). The slice, though, must stay conditional on an actual trailing slash — a cut that sliced whenever isMarker was true chopped the last character off every folder name and produced a phantom file_explorer_test_asset row next to file_explorer_test_assets in live QA. Keep the two conditions distinct.
| projectId: string | null | undefined, | ||
| path: string, | ||
| ): Promise<Blob> { | ||
| const res = await axios.get<Blob>(`${getAgentaApiUrl()}/mounts/${mountId}/files/download`, { |
There was a problem hiding this comment.
Deliberate Fern bypass, scoped to this one fetcher. The generated downloadMountFile never sets a responseType, so the shared fetcher's getResponseBody falls through to response.text() + JSON.parse on every body — binary bytes come back mangled, and .withRawResponse() can't recover them because the stream is already consumed. fetchMountFiles and fetchMountFileText stay on Fern per web/AGENTS.md; the axios interceptor already special-cases Blob responses, so no extra wiring.
| "css", | ||
| ]) | ||
| const IMAGE_EXTENSIONS = new Set(["png", "jpg", "jpeg", "gif", "svg", "webp"]) | ||
| const MAX_PREVIEW_BYTES = 2 * 1024 * 1024 |
There was a problem hiding this comment.
Cap rationale: the ?read= endpoint loads the whole object into API memory and utf-8-decodes it, so previews should stay small. The cap gates both preview queries via enabled (checked against the listing's size, which the backend always populates), and bigger files fall back to Download, which streams bytes instead. 2 MB is a judgment call; happy to tune.
| const [objectUrl, setObjectUrl] = useState<string | null>(null) | ||
| // Create the object URL as an effect (not in useMemo) so StrictMode's double-render | ||
| // can't orphan one; revoke the previous URL on every re-run and on unmount. | ||
| useEffect(() => { |
There was a problem hiding this comment.
Object-URL lifecycle: created in an effect (not useMemo) so StrictMode's double-invoked render can't orphan a URL, and the cleanup revokes on file switch, preview close, and unmount. The download path (triggerDownload above) defers its revoke by a tick instead, because a synchronous revoke after a synthetic click can cancel the save in Firefox.
| expect(rows.map((row) => row.name)).toEqual(["notes", "src", "tests"]) | ||
| }) | ||
|
|
||
| it("collapses a geesefs mkdir marker plus its contents into one full-name folder row", () => { |
There was a problem hiding this comment.
This test and the two after it are the regression pins for the three real bugs found during review + live QA: recursive-listing flattening, is_folder markers without trailing slashes rendering as files, and the truncated-name phantom folder row. Note the colocated oss/src vitest files (this one included) are not yet wired into the CI harness — tracked in docs/design/mount-file-viewer/open-issues.md; they run locally via the command in the PR body.
|
@coderabbitai review |
✅ Action performedReview finished.
|
a31dce2 to
e2a2de7
Compare
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
web/oss/src/components/SessionInspector/api.ts (1)
49-68: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd runtime validation before returning these Fern payloads.
fetchMountFilesandfetchMountFileTextstill cast the response directly to local types;safeParseWithLoggingis already available from@agenta/entities/sharedand should be used here with local Zod schemas so backend drift fails loudly instead of leaking bad data into the UI.Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7f9515c3-3cb4-4d13-b6f4-e575398935cf
📒 Files selected for processing (11)
docs/design/mount-file-viewer/README.mddocs/design/mount-file-viewer/context.mddocs/design/mount-file-viewer/open-issues.mddocs/design/mount-file-viewer/plan.mddocs/design/mount-file-viewer/pr-body.mddocs/design/mount-file-viewer/research.mddocs/design/mount-file-viewer/status.mdweb/oss/src/components/SessionInspector/api.tsweb/oss/src/components/SessionInspector/assets/mountBrowser.test.tsweb/oss/src/components/SessionInspector/assets/mountBrowser.tsweb/oss/src/components/SessionInspector/tabs/MountsTab.tsx
The SessionInspector Mounts tab showed only a mount's name, slug, and id.
It now lists the mount's files (one-level view derived client-side from
the flat recursive listing, folders first, breadcrumb navigation) with
click-to-preview: text inline via the read endpoint, images via blob
object URLs, and a Download fallback for other types and files over 2 MB.
Frontend-only; uses the existing /mounts/{id}/files endpoints. The blob
fetch uses the shared axios instance because the generated Fern client
JSON-parses binary bodies. deriveRows lives in assets/mountBrowser.ts
with a colocated vitest suite (9 tests).
Design workspace: docs/design/mount-file-viewer/
Claude-Session: https://claude.ai/code/session_018MaXPNpvzN22kngHno3VMj
e2a2de7 to
e2b5d74
Compare
ffc70be to
0bdb6ec
Compare
|
🤖 The AI agent says: Addressed the remaining review feedback before merge.
Focused Vitest: 9/9 passed. Frontend lint and Prettier passed. |
dosubot
Bot
added
size:XL
0bdb6ec to
daae705
Compare
|
|
daae705 to
b6c4947
Compare
2 participants
Context
The playground's SessionInspector drawer has a Mounts tab. For a session with a durable
cwd mount full of files, the tab showed only each mount's
name / slug / id. Users couldsee that a mount existed but not what the agent wrote into it, let alone read a file. The
backend already exposed everything needed (
GET /mounts/{id}/filesfor listing,?read=for text,
/files/downloadfor raw bytes), so the tab just never called it.Before: expanding a mount showed three lines of metadata.
After: expanding a mount shows its files, folders first, with breadcrumb navigation.
Clicking a file previews it inline (text or image) or offers a Download button.
Changes
api.tsgained three fetchers:fetchMountFiles,fetchMountFileText(both through theFern
mountsclient), andfetchMountFileBlob. The blob fetcher goes through the sharedaxios instance instead of Fern, because the generated
downloadMountFilealwaystext/JSON-parses the response body, so a
Blobis unreachable through it. The functioncarries a one-line comment explaining why.
MountsTab.tsxreplaced the flat metadataListwith aCollapse, one panel per mount.Expanding a panel queries the root listing; clicking a folder row re-queries with that
folder's path. Clicking a file opens an inline preview: text extensions render through
read=, image extensions fetch a blob and render it via an object URL, everything else(and anything over 2 MB) shows a Download button instead.
The listing endpoint is recursive and flat: it returns every file under a path in one
response, with paths relative to the mount root, and folders only show up as
is_foldermarker entries with no trailing slash.
deriveRows(new file,assets/mountBrowser.ts) derives the one-level view the UI needs: it groups entries bytheir first path segment past the current folder into synthetic folder rows, merges those
with any explicit folder markers so a folder never appears twice, and returns folders
before files, both sorted alphabetically. It is unit-tested (9 tests,
assets/mountBrowser.test.ts, colocated per repo precedent likeTemplateStrip/assets/pagerMath.test.ts), including regression tests for the two bugsreview caught (see below).
Scope / risk
Frontend only. Files touched:
SessionInspector/api.ts,SessionInspector/tabs/MountsTab.tsx,and the new
SessionInspector/assets/mountBrowser.ts+mountBrowser.test.ts. No backendchanges, no Fern regen, no new endpoints.
dump.ts(the markdown export of a session's mounts) is untouched and still shows metadataonly; it does not gain a file listing in this PR. Nothing outside the Mounts tab changes,
so the only realistic regression surface is that tab itself: mounts with zero files, mounts
that fail to load, and sessions with zero mounts all need to keep working (all three are
covered in QA below).
The blob-fetch axios exception is intentional, not a drift from the Fern convention:
web/AGENTS.mdrequires Fern for new endpoint calls, andfetchMountFiles/fetchMountFileTextfollow that.fetchMountFileBlobcalls the same endpoint Fern alreadywraps; it bypasses Fern only because Fern's generated method parses every response body as
JSON before handing it back, which makes a binary
Blobunreachable no matter how thecall is made. The listing and text fetchers stay on Fern.
One known v1 limit, tracked in
open-issues.md: the listing endpoint has no one-level ordelimiter mode, so a very large mount's root view transfers its full recursive tree in one
response. The frontend derivation bounds what renders, not what is fetched.
Tests / notes
npx --yes vitest@4.1.10 run src/components/SessionInspector/assets/mountBrowser.test.tsfrom
web/oss: 9/9 passing.tscclean on the touched files.web/oss/src/**/*.test.tsvitest files, this one included, are not wired intothe CI unit-test harness (it runs package
test:<layer>scripts only). Logged as adeferred item in
open-issues.md; same status as the existing colocated tests.the mount root and duplicated them inside folders (the listing is recursive, not
one-level); a second cut keyed folder detection off a trailing slash the backend never
sends, so empty folders rendered as files and 404'd on preview, and a later one-line fix
for that unconditionally sliced a marker entry's last character, producing a phantom
folder row. All three are pinned as regression tests in
mountBrowser.test.ts.How to QA
Prerequisites: local dev stack (
run.shwith your usual OSS or EE flags), and asession whose cwd mount has nested files. If you don't have one, run a playground agent
that writes a few files into its cwd, including a subfolder, a text file, an image, and a
file over 2 MB.
Steps:
SessionInspector drawer.
.mdor.txtfile..pngor other image file..mp3or.pdf).Expected result:
mounts.
Automated tests:
(run from
web/oss)Edge cases: a mount with an explicit empty-folder marker must show a folder row, not a
file row and not a phantom row with a truncated name (both were real bugs, now pinned in
mountBrowser.test.ts). A mount with a same-named file and folder at the same path (forexample a file
aand a foldera/) must show both as separate rows. Re-check dark modeon the preview panel specifically, not just the file list.
https://claude.ai/code/session_018MaXPNpvzN22kngHno3VMj