feat: implement file download and preview features with improved URL handling #315
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors file download and preview features by centralizing URL encoding logic and improving API route handling. The changes move encoding responsibility from multiple call sites to a single location in the getDownloadUrl function, and introduce a new direct download endpoint alongside the existing URL generation endpoint.
Key changes:
- Centralized URL encoding in
getDownloadUrlfunction, removing duplicate encoding logic from 5+ call sites - Added new
/files/downloadendpoint for direct file streaming with authentication - Updated API routes to use query parameters instead of path parameters for object names
- Added accessibility improvements with screen-reader-only descriptions in modals
Reviewed Changes
Copilot reviewed 31 out of 31 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| apps/web/src/http/endpoints/files/index.ts | Centralized URL encoding logic in getDownloadUrl function |
| apps/web/src/utils/download-queue-utils.ts | Removed redundant encoding calls (now handled by getDownloadUrl) |
| apps/web/src/hooks/use-file-preview.ts | Removed redundant encoding calls |
| apps/web/src/hooks/use-enhanced-file-manager.ts | Removed redundant encoding calls |
| apps/web/src/components/tables/files-grid.tsx | Removed redundant encoding calls |
| apps/web/src/components/modals/file-preview-modal.tsx | Added DialogDescription for accessibility |
| apps/web/src/app/api/(proxy)/files/download/route.ts | Updated to use query parameters instead of path parameters |
| apps/web/src/app/api/(proxy)/files/download-url/route.ts | New proxy route for download URL generation |
| apps/web/src/app/(shares)/reverse-shares/components/*.tsx | Simplified component props and removed duplicate code |
| apps/web/messages/*.json | Added file preview description translations |
| apps/server/src/modules/file/controller.ts | Added new downloadFile method with authentication logic |
| apps/server/src/modules/file/routes.ts | Added /files/download endpoint and updated /files/download-url to use query params |
| apps/server/src/providers/filesystem-storage.provider.ts | Simplified presigned URL generation and removed token consumption methods |
| apps/server/src/modules/filesystem/controller.ts | Removed manual token consumption calls |
| async downloadFile(request: FastifyRequest, reply: FastifyReply) { | ||
| try { | ||
| const { objectName, password } = request.query as { | ||
| objectName: string; | ||
| password?: string; | ||
| }; |
There was a problem hiding this comment.
The new downloadFile method duplicates significant authentication and access control logic from getDownloadUrl. Consider extracting the shared authentication logic (lines 330-364 in downloadFile and lines 225-267 in getDownloadUrl) into a separate private method like checkFileAccess to improve maintainability and reduce code duplication.
| return reply.status(401).send({ error: "Unauthorized access to file." }); | ||
| } | ||
|
|
||
| const storageProvider = (this.fileService as any).storageProvider; |
There was a problem hiding this comment.
Using type assertion (this.fileService as any) to access storageProvider bypasses TypeScript's type safety. Consider adding a proper getter method to the FileService class or updating its type definition to include the storageProvider property.
| const storageProvider = (this.fileService as any).storageProvider; | |
| const storageProvider = this.fileService.getStorageProvider(); |
| return reply.status(401).send({ error: "Unauthorized access to file." }); | ||
| } | ||
|
|
||
| const storageProvider = (this.fileService as any).storageProvider; |
There was a problem hiding this comment.
Duplicate usage of type assertion (this.fileService as any) to access storageProvider. This issue appears twice in the downloadFile method (lines 312 and 370). Consider adding a proper getter method to the FileService class or updating its type definition to include the storageProvider property.
danielalves96
deleted the
feat/file-download-and-preview-features-with-improved-URL-handling
branch
No description provided.