Refactor mobile file viewer with enhanced FileBrowser and folder page #10
Conversation
…nents - Added new package-lock.json for dashboard package - Substantial updates to FileBrowser.vue and FilesFolderPage.vue adding features and refactoring - Large number of insertions and deletions reflect dependency management and component improvements Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
WalkthroughAdds a new FileBrowser Vue component implementing grid/list views, selection (mouse/keyboard/touch), drag-and-drop, context menus, routing-based navigation/preview, and bulk file operations. Replaces the previous table-driven files UI in FilesFolderPage.vue with a single FileBrowser instance and removes the old page-level file management subcomponents and logic. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Router
participant FilesPage as FilesFolderPage
participant FileBrowser
participant Services as Store/Services
User->>Router: Navigate to /files/:bucket/:folder?
Router->>FilesPage: mount with route props
FilesPage->>FileBrowser: render with bucket, folder
FileBrowser->>Services: fetch items for bucket/folder
Services-->>FileBrowser: return items
FileBrowser-->>User: render grid/list
rect rgba(200,230,255,0.15)
note right of FileBrowser: Navigation & preview
User->>FileBrowser: click breadcrumb/folder
FileBrowser->>Router: router.push(new folder)
Router->>FileBrowser: update props
FileBrowser->>Services: refetch items
end
rect rgba(220,255,220,0.15)
note right of FileBrowser: Selection & bulk ops
User->>FileBrowser: select items / keyboard shortcuts
User->>FileBrowser: trigger Move/Delete/Refresh
FileBrowser->>Services: perform operations
Services-->>FileBrowser: results / errors
end
rect rgba(255,240,200,0.15)
note right of FileBrowser: Drag-and-drop
User->>FileBrowser: drag items onto folder
FileBrowser->>Services: move items (progress)
Services-->>FileBrowser: success/error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
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: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
packages/dashboard/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
packages/dashboard/src/components/files/FileBrowser.vue(1 hunks)packages/dashboard/src/pages/files/FilesFolderPage.vue(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Build
packages/dashboard/src/pages/files/FilesFolderPage.vue
[warning] 11-11: assist/source/organizeImports: The imports and exports are not sorted. Safe fix: Organize Imports (Biome).
packages/dashboard/src/components/files/FileBrowser.vue
[error] 402-402: lint/correctness/noUnusedFunctionParameters: Unused parameter 'emit' in setup(props, { emit }).
[error] 566-566: lint/correctness/noUnusedFunctionParameters: Unused parameters might be the result of an incomplete refactoring in handleTouchStart(event, item, index).
[error] 583-583: lint/correctness/noUnusedFunctionParameters: Unused parameter 'event' in handleTouchEnd(event).
[error] 655-655: lint/correctness/noUnusedFunctionParameters: Unused parameter 'folder' in getItemCount(folder).
[error] 660-660: lint/correctness/noUnusedFunctionParameters: Unused parameter 'event' in showContextMenu(event, item).
[error] 701-701: lint/correctness/noUnusedFunctionParameters: Unused parameter 'event' in handleDragLeave(event).
| const handleImageError = (event, item) => { | ||
| // Replace broken image with icon | ||
| event.target.style.display = 'none' | ||
| const icon = document.createElement('div') | ||
| icon.innerHTML = `<q-icon name="${item.icon}" color="${item.color}" size="48px" />` | ||
| event.target.parentElement.appendChild(icon) | ||
| } |
There was a problem hiding this comment.
Replace the DOM injection fallback for thumbnails.
Appending a literal <q-icon> string via innerHTML never instantiates the Quasar component, so users get an empty preview when thumbnails fail. Track the error state on the item (or a local map) and let the template render the existing icon branch instead of manipulating the DOM.
One option:
- const handleImageError = (event, item) => {
- // Replace broken image with icon
- event.target.style.display = 'none'
- const icon = document.createElement('div')
- icon.innerHTML = `<q-icon name="${item.icon}" color="${item.color}" size="48px" />`
- event.target.parentElement.appendChild(icon)
- }
+ const handleImageError = (event, item) => {
+ event.target.style.display = 'none'
+ item.thumbnailError = true
+ }and update the template to gate the <img> with && !item.thumbnailError.
📝 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 handleImageError = (event, item) => { | |
| // Replace broken image with icon | |
| event.target.style.display = 'none' | |
| const icon = document.createElement('div') | |
| icon.innerHTML = `<q-icon name="${item.icon}" color="${item.color}" size="48px" />` | |
| event.target.parentElement.appendChild(icon) | |
| } | |
| const handleImageError = (event, item) => { | |
| event.target.style.display = 'none' | |
| item.thumbnailError = true | |
| } |
- Fixed unused function parameters in FileBrowser.vue - Reorganized imports to follow Biome sorting rules - Prefixed unused parameters with underscore where needed - Removed unused emit parameter from setup function All lint checks now pass successfully.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/dashboard/src/components/files/FileBrowser.vue (2)
597-610:⚠️ Regression: Restore tap-to-open for files on mobile.
handleTouchEndonly opens folders on short taps (line 606), so tapping files on mobile never launches the preview. This regresses the core "mobile file viewer" functionality. Please treat short taps the same as desktop double-clicks by callingopenItemfor any item type.Apply this diff:
const touchDuration = Date.now() - touchStartTime.value; if (touchDuration < 500 && !selectionMode.value) { // Short tap - open item - if (currentItem.value?.type === "folder") { + if (currentItem.value) { openItem(currentItem.value); } }
663-669:⚠️ Replace DOM injection fallback for thumbnails.Appending a literal
<q-icon>string viainnerHTMLnever instantiates the Quasar component, so users see an empty preview when thumbnails fail. Track the error state on the item (or a local reactive map) and let the template render the existing icon branch instead of manipulating the DOM directly.One option:
const handleImageError = (event, item) => { - // Replace broken image with icon event.target.style.display = "none"; - const icon = document.createElement("div"); - icon.innerHTML = `<q-icon name="${item.icon}" color="${item.color}" size="48px" />`; - event.target.parentElement.appendChild(icon); + item.thumbnailError = true; }Then update the template (lines 86-101) to conditionally render the
<img>only when!item.thumbnailError:<div class="file-card-preview"> <img - v-if="item.type === 'file' && isMediaFile(item.name)" + v-if="item.type === 'file' && isMediaFile(item.name) && !item.thumbnailError" :src="getThumbnailUrl(item, bucket)" :alt="item.name" class="file-thumbnail-img" loading="lazy" @error="handleImageError($event, item)" /> <q-icon v-else :name="item.icon" :color="item.color" size="48px" /> </div>Apply the same change to the list view thumbnail at lines 192-199.
🧹 Nitpick comments (3)
packages/dashboard/src/components/files/FileBrowser.vue (3)
435-435: Consider makingshowThumbnailsInListconfigurable.
showThumbnailsInListis declared and always set totrue(line 435) but is never modified. If this is intended as a user preference, consider either:
- Persisting it in localStorage like
viewMode, or- Exposing it as a prop for parent control, or
- Removing it if thumbnails should always be shown in list view.
67-67: Remove unnecessary$eventfrom template bindings.Several template event bindings pass
$eventto handlers that don't use it (indicated by the_eventparameter prefix):
- Line 67, 173:
@touchstart="handleTouchStart($event, item, index)"→@touchstart="handleTouchStart(item, index)"- Line 73, 179:
@dragleave="handleDragLeave($event)"→@dragleave="handleDragLeave"- Line 121, 226:
@click.stop="showContextMenu($event, item)"→@click.stop="showContextMenu(item)"This aligns the template with the handler signatures and removes the confusion caused by passing unused parameters.
Also applies to: 73-73, 121-121, 173-173, 179-179, 226-226
6-12: Consider accessibility improvements for interactive elements.Interactive elements like breadcrumb items (lines 6-12), grid view cards (lines 55-137), and list rows (lines 161-243) use divs with click handlers. While keyboard navigation is implemented in JavaScript, consider enhancing accessibility by:
- Using
<button>elements for breadcrumb items instead of divs with@click- Adding
role="button",tabindex="0", andaria-labelattributes to file cards/rows- Ensuring focus states are visually distinct (already handled in CSS with
.focusedclass)These changes would improve the experience for screen reader users and align with WCAG best practices.
Also applies to: 55-137, 161-243
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
packages/dashboard/src/components/files/FileBrowser.vue(1 hunks)packages/dashboard/src/pages/files/FilesFolderPage.vue(1 hunks)
🔇 Additional comments (21)
packages/dashboard/src/components/files/FileBrowser.vue (18)
400-408: LGTM: Props are well-defined.The component accepts
bucket(required) andfolder(optional, defaults to empty string) as props. This provides a clear and minimal API for controlling the file browser location.
448-473: Verify hardcoded "Home/" in breadcrumb logic.Line 463 removes a hardcoded
"Home/"prefix from breadcrumb paths. This assumes the folder structure always starts with "Home/" which may not hold for all bucket configurations. Please verify this assumption or make the prefix configurable.If the assumption is incorrect, consider removing the hardcoded replacement or making it dynamic based on the actual bucket/folder structure.
492-509: LGTM:fetchFilesimplementation is solid.The method correctly manages loading state, calls the API handler with appropriate parameters, and handles errors with user-friendly notifications. The use of
finallyensures loading state is always cleared.
511-527: LGTM: Selection methods are straightforward and correct.
isSelected,clearSelection, andhandleSelectAllall correctly manage selection state without unnecessary complexity.
529-578: LGTM: Click and context menu handlers are well-implemented.
handleItemClickcorrectly handles Ctrl/Cmd-click for toggling, Shift-click for range selection, and normal clicks for navigation.handleItemDoubleClickandhandleContextMenuappropriately handle their respective interactions with proper mobile detection.
580-595: LGTM: Touch start handler implements long-press selection correctly.
handleTouchStartcorrectly implements long-press detection (500ms) to enter selection mode with haptic feedback. The timer management is sound.
612-617: LGTM: Touch move handler correctly cancels long-press.
handleTouchMoveappropriately clears the touch timer when the user drags, preventing accidental selection mode activation.
684-777: LGTM: Drag-and-drop implementation is robust and user-friendly.The drag-and-drop handlers correctly:
- Validate readonly mode and drop targets
- Handle multi-item dragging
- Prevent dropping items on themselves
- Show progress notifications during moves
- Handle errors gracefully
- Refresh the view after successful operations
The progress notification with percentage updates (lines 735-762) is particularly good UX for long-running operations.
780-790: LGTM: File operation delegators are concise.
handleDelete,handleRename, andhandleUpdateMetadatacorrectly delegate to theoptionscomponent with the target item, using the fallbackcurrentItem.valuewhen no item is provided.
792-824: LGTM: Share handler with clipboard fallback.
handleSharecorrectly builds URLs for both folders and files, uses the modernnavigator.clipboardAPI, and provides user feedback for success/failure.
826-852: LGTM: Cache refresh handles folders recursively.
handleRefreshCachecorrectly distinguishes between files and folders, fetching folder contents recursively before opening the cache dialog. TheonCompletecallback ensures the view refreshes after the operation.
854-864: LGTM: Bulk operations are well-structured.
handleBulkMoveandhandleBulkDeleteappropriately delegate to specialized dialogs/components, withhandleBulkDeletecorrectly handling single vs. bulk deletion.
866-895: LGTM: Bulk cache refresh aggregates files correctly.
handleBulkRefreshCachecorrectly iterates selected items, fetching folder contents where needed, then aggregates all files before opening the cache dialog. TheonCompletecallback ensures both refresh and selection clearing.
897-907: LGTM: Gallery and move completion handlers are simple and correct.
handleMoveCompleteandopenGalleryare straightforward utility methods that correctly manage state and component interactions.
910-974: LGTM: Comprehensive keyboard navigation.
handleKeyDownimplements a complete set of keyboard shortcuts:
- Arrow keys for navigation with boundary checks
- Enter to open items
- Space to toggle selection
- Delete/Backspace for deletion
- Escape to clear selection
- Ctrl/Cmd+A to select all
All shortcuts prevent default browser behavior appropriately and handle edge cases (empty lists, focused index bounds).
977-1005: LGTM: Lifecycle management is sound.The component correctly:
- Persists view mode preference to localStorage
- Fetches files on mount
- Registers and cleans up keyboard event listeners (preventing memory leaks)
- Handles route-based file preview using
nextTick- Re-fetches data when folder or bucket props change
1081-1530: LGTM: Comprehensive and responsive styling.The styles are well-organized with:
- Clear separation between grid and list views
- Responsive breakpoints for mobile/tablet/desktop
- Smooth animations for state transitions
- Accessible focus states
- Consistent use of spacing and colors
410-410:⚠️ Build-blocking: Remove unusedemitparameter.The setup function accepts
emitin the destructured context but never uses it, causing a lint failure (noUnusedFunctionParameters). Please remove it from the signature.Apply this diff:
- setup(props) { + setup(props) {Wait, I see the code already shows
setup(props)withoutemit. Let me re-examine...Actually, the provided code at line 410 shows:
setup(props) {This is correct—no
emitis present. However, the past review comment states thatemitwas causing a lint error. Since the current code doesn't showemit, this issue appears to have been resolved. Let me verify the other past issues.packages/dashboard/src/pages/files/FilesFolderPage.vue (3)
1-8: LGTM: Simplified page structure.The template is now a clean wrapper around the
FileBrowsercomponent, passing through the requiredbucketandfolderprops from the route. This delegation approach is appropriate and maintainable.
10-25: LGTM: Minimal component logic.The script correctly imports
FileBrowser, registers it, and exposes a single computed property (selectedBucket) to provide the bucket from the route. This is exactly the right level of abstraction for a page component.
27-33: LGTM: Simple page styling.The scoped styles establish the page as a full-height flex container, allowing
FileBrowserto control its own layout. This is appropriate for a wrapper component.
Summary
FileBrowser.vuefor improved file browsing experienceFilesFolderPage.vuewith UI and logic enhancementspackage-lock.jsonwith updated dependencies reflecting the new featuresChanges
Components
FileBrowser, improve folder navigation and file displayPackage Management
package-lock.jsonwith locked dependencies to ensure reproducible builds and updated packages necessary for refactoringCode Quality
Test plan
FileBrowsercomponent🌿 Generated by Terry
ℹ️ Tag @terragon-labs to ask questions and address PR feedback
📎 Task: https://www.terragonlabs.com/task/f4e09764-3cea-4fec-9ccd-3ac1bbf43b82
Summary by CodeRabbit