chore: add files route and UI to dev-playground#114
chore: add files route and UI to dev-playground#114atilafassina wants to merge 2 commits intoplugin/filesfrom
Conversation
atilafassina
force-pushed
the
plugin/files-playground
branch
2 times, most recently
from
0ba630f to
2862caa
Compare
atilafassina
force-pushed
the
plugin/files
branch
from
8fa2314 to
60b1bcd
Compare
|
@copilot let's fix the merge conflicts to base branch |
|
@atilafassina I've opened a new pull request, #122, to work on those changes. Once the pull request is ready, I'll request review from you. |
atilafassina
force-pushed
the
plugin/files-playground
branch
2 times, most recently
from
8f34b36 to
0279ba2
Compare
There was a problem hiding this comment.
Pull request overview
Adds a new Files experience to the Dev Playground so developers can browse Databricks Volumes via the AppKit Files plugin.
Changes:
- Add
/filesroute with a file browser UI (list directories/files, preview, upload, download, delete, mkdir). - Add navigation entry points to the Files route from the home cards and the top nav.
- Update generated route tree and AppKit SQL type declarations.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| apps/dev-playground/client/src/routes/index.tsx | Adds a new “File Browser” card that navigates to /files. |
| apps/dev-playground/client/src/routes/files.route.tsx | Introduces the Files route UI and all client-side interactions with /api/files/*. |
| apps/dev-playground/client/src/routes/__root.tsx | Adds “Files” to the global navigation bar. |
| apps/dev-playground/client/src/routeTree.gen.ts | Registers the new /files route in the generated router tree. |
| apps/dev-playground/client/src/appKitTypes.d.ts | Updates declared query result shapes (typegen update). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export const Route = createFileRoute("/files")({ | ||
| component: FilesRoute, | ||
| search: { | ||
| middlewares: [retainSearchParams(true)], | ||
| }, | ||
| }); |
There was a problem hiding this comment.
There are Playwright integration tests for other routes under apps/dev-playground/tests, but none covering the new /files route. Add at least a smoke test that loads the page and mocks the /api/files/* endpoints (similar to setupMockAPI) to prevent regressions in navigation, listing, and preview rendering.
| <div className="flex items-center gap-2"> | ||
| <Button | ||
| variant="outline" | ||
| size="sm" | ||
| onClick={() => { | ||
| setShowNewDirInput(true); | ||
| setTimeout(() => newDirInputRef.current?.focus(), 0); | ||
| }} | ||
| > | ||
| <FolderPlus className="h-4 w-4 mr-2" /> | ||
| New Folder | ||
| </Button> | ||
| <input | ||
| ref={fileInputRef} | ||
| type="file" | ||
| className="hidden" | ||
| onChange={handleUpload} | ||
| /> | ||
| <Button | ||
| variant="outline" | ||
| size="sm" | ||
| disabled={uploading} | ||
| onClick={() => fileInputRef.current?.click()} | ||
| > | ||
| {uploading ? ( | ||
| <Loader2 className="h-4 w-4 mr-2 animate-spin" /> | ||
| ) : ( | ||
| <Upload className="h-4 w-4 mr-2" /> | ||
| )} | ||
| {uploading ? "Uploading..." : "Upload"} | ||
| </Button> | ||
| </div> |
There was a problem hiding this comment.
When no default volume is configured (UI indicates this state), the Upload/New Folder actions remain enabled and will attempt requests with relative paths. Consider disabling these actions (or showing a blocking prompt) until volumeRoot is available to avoid confusing errors and unintended requests.
| const loadDirectory = useCallback(async (path?: string) => { | ||
| setLoading(true); | ||
| setError(null); | ||
| setSelectedFile(null); | ||
| setPreview(null); | ||
|
|
||
| try { | ||
| const url = path | ||
| ? `/api/files/list?path=${encodeURIComponent(path)}` | ||
| : "/api/files/list"; | ||
| const response = await fetch(url); | ||
|
|
||
| if (!response.ok) { | ||
| const data = await response.json().catch(() => ({})); | ||
| throw new Error( | ||
| data.error ?? `HTTP ${response.status}: ${response.statusText}`, | ||
| ); | ||
| } | ||
|
|
||
| const data: DirectoryEntry[] = await response.json(); | ||
| data.sort((a, b) => { | ||
| if (a.is_directory && !b.is_directory) return -1; | ||
| if (!a.is_directory && b.is_directory) return 1; | ||
| return (a.name ?? "").localeCompare(b.name ?? ""); | ||
| }); | ||
| setEntries(data); | ||
| setCurrentPath(path ?? ""); | ||
| } catch (err) { | ||
| setError(err instanceof Error ? err.message : String(err)); | ||
| setEntries([]); | ||
| } finally { | ||
| setLoading(false); | ||
| } |
There was a problem hiding this comment.
loadDirectory can race if users navigate folders quickly (or if React StrictMode triggers extra effects): an older list response may set entries/currentPath after a newer navigation. Consider aborting in-flight requests or guarding state updates with a requestId so only the latest call wins.
| if ( | ||
| volumeRoot && | ||
| normalize(parentPath).length <= normalize(volumeRoot).length | ||
| ) { | ||
| loadDirectory(volumeRoot); | ||
| return; |
There was a problem hiding this comment.
navigateToParent uses string length comparison to decide when to clamp to volumeRoot (normalize(parentPath).length <= normalize(volumeRoot).length). This is not a reliable way to determine path ancestry and can behave incorrectly for different-length sibling paths. Compare normalized paths by prefix/segment relationship instead (e.g., ensure parentPathNormalized starts with volumeRootNormalized + '/' or use segment arrays).
| if ( | |
| volumeRoot && | |
| normalize(parentPath).length <= normalize(volumeRoot).length | |
| ) { | |
| loadDirectory(volumeRoot); | |
| return; | |
| if (volumeRoot) { | |
| const parentNorm = normalize(parentPath); | |
| const rootNorm = normalize(volumeRoot); | |
| const rootWithSlash = rootNorm.endsWith("/") ? rootNorm : `${rootNorm}/`; | |
| const isWithinRoot = | |
| parentNorm === rootNorm || parentNorm.startsWith(rootWithSlash); | |
| if (!isWithinRoot) { | |
| loadDirectory(volumeRoot); | |
| return; | |
| } |
| {breadcrumbSegments.map((segment, index) => ( | ||
| <span key={segment} className="contents"> | ||
| <BreadcrumbSeparator /> | ||
| <BreadcrumbItem> | ||
| {index === breadcrumbSegments.length - 1 ? ( | ||
| <BreadcrumbPage>{segment}</BreadcrumbPage> | ||
| ) : ( | ||
| <BreadcrumbLink | ||
| className="cursor-pointer" | ||
| onClick={() => navigateToBreadcrumb(index)} | ||
| > | ||
| {segment} | ||
| </BreadcrumbLink> | ||
| )} | ||
| </BreadcrumbItem> | ||
| </span> | ||
| ))} |
There was a problem hiding this comment.
The breadcrumb items use key={segment}; repeated folder names at different levels will produce duplicate keys and React warnings. Use a stable unique key (e.g., include index or build the accumulated path up to that crumb).
| setShowNewDirInput(false); | ||
| setNewDirName(""); | ||
| }} | ||
| className="text-muted-foreground hover:text-foreground" |
There was a problem hiding this comment.
Icon-only buttons need accessible names. The close (X) control should have an aria-label (or include visually hidden text) so screen readers can announce its purpose.
| className="text-muted-foreground hover:text-foreground" | |
| className="text-muted-foreground hover:text-foreground" | |
| aria-label="Cancel creating folder" |
| <Button | ||
| variant="destructive" | ||
| size="sm" | ||
| disabled={deleting} | ||
| onClick={handleDelete} | ||
| > | ||
| {deleting ? ( | ||
| <Loader2 className="h-4 w-4 animate-spin" /> | ||
| ) : ( | ||
| <Trash2 className="h-4 w-4" /> | ||
| )} | ||
| </Button> |
There was a problem hiding this comment.
The delete action is an icon-only button; add an accessible label (e.g., aria-label="Delete file") so it’s usable with screen readers.
| {currentPath | ||
| ? "This directory is empty." | ||
| : "No default volume configured. Set DATABRICKS_DEFAULT_VOLUME to get started."} | ||
| </p> |
There was a problem hiding this comment.
PR description mentions adding DATABRICKS_DEFAULT_VOLUME, and the UI instructs users to set it, but apps/dev-playground/.env.dist doesn’t include this variable. Please update .env.dist (and/or README) so new contributors discover the required config without reading the UI at runtime.
| const loadPreview = useCallback(async (filePath: string) => { | ||
| setPreviewLoading(true); | ||
| setPreview(null); | ||
|
|
||
| try { | ||
| const response = await fetch( | ||
| `/api/files/preview?path=${encodeURIComponent(filePath)}`, | ||
| ); | ||
|
|
||
| if (!response.ok) { | ||
| const data = await response.json().catch(() => ({})); | ||
| throw new Error(data.error ?? `HTTP ${response.status}`); | ||
| } | ||
|
|
||
| const data = await response.json(); | ||
| setPreview(data); | ||
| } catch { | ||
| setPreview(null); | ||
| } finally { | ||
| setPreviewLoading(false); | ||
| } |
There was a problem hiding this comment.
loadPreview can race when users click multiple files quickly: a slower response for an earlier file can overwrite preview for the currently selected file. Track the latest requested path (e.g., ref/requestId) or use AbortController and only apply the response if it matches the current selectedFile.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Atila Fassina <atila@fassina.eu>
atilafassina
force-pushed
the
plugin/files-playground
branch
from
0279ba2 to
8d31087
Compare
Adds the new File Plugin to Dev Playground.
Add a new env_var to your
.envfile.For example:
(you can use that one, nothing critical there 😉 )