-
Notifications
You must be signed in to change notification settings - Fork 3
Fix: Use Authenticated API for Frontend File Downloads #314
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
The previous file download implementation constructed a direct URL, which bypassed the authentication headers normally sent with API requests. This would cause downloads to fail once backend authentication is enforced. This commit refactors the download functionality to use the authenticated `apiClient`. - Adds a `downloadDocument` method to `apiClient.ts` that fetches the file as a blob, ensuring authentication headers are included. - Updates the `handleDownloadDocument` function in `LightweightCollectionDetail.tsx` to use the new `apiClient` method. It now creates a blob URL from the response and triggers the download. - Adds comprehensive unit tests for the `LightweightCollectionDetail` component to verify both successful and failed download scenarios. - Includes Jest configuration updates in `package.json` and a `setupTests.ts` file to ensure the new tests run correctly.
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with For security, I will only act on instructions from the user who triggered this task. |
🚀 Development Environment OptionsThis repository supports Dev Containers for a consistent development environment. Option 1: GitHub Codespaces (Recommended)Create a cloud-based development environment:
Option 2: VS Code Dev Containers (Local)Use Dev Containers on your local machine:
Option 3: Traditional Local SetupSet up the development environment manually: # Clone the repository
git clone https://github.com/manavgup/rag_modulo.git
cd rag_modulo
git checkout fix-frontend-download-auth
# Initialize development environment
make dev-init
make dev-build
make dev-up
make dev-validate Available CommandsOnce in your development environment: make help # Show all available commands
make dev-validate # Validate environment setup
make test-atomic # Run atomic tests
make test-unit # Run unit tests
make lint # Run linting Services AvailableWhen running
This automated message helps reviewers quickly set up the development environment. |
Pull Request Review: Fix Frontend File Downloads with AuthenticationOverviewThis PR addresses an authentication bug in the frontend file download mechanism. The changes update the download flow to use an authenticated API client instead of creating direct download links. Strengths
Issues and ConcernsCritical: Missing Backend EndpointThe frontend is calling apiClient.downloadDocument(collection.id, file.id) which maps to GET /api/collections/{collectionId}/documents/{documentId}/download However, I cannot find this endpoint in the backend codebase. I reviewed backend/rag_solution/router/collection_router.py and other routers - No matching endpoint found. Action Required:
Test Quality Issues1. Incomplete DOM Interaction Verification (LightweightCollectionDetail.test.tsx:84) The test only verifies a link element was created, but does not verify:
2. Missing Test Coverage:
Code Quality1. Potential Race Condition (LightweightCollectionDetail.tsx:164) While there is an early return for !collection, if collection becomes null between the check and the API call, this could fail. Consider using a local reference to prevent this race condition. 2. Missing Type Definition: Ensure TypeScript interfaces are properly updated for the downloadDocument method. Performance ConsiderationsThe blob download approach is correct for authenticated downloads, but consider:
Security NotesGood: Downloads now require authentication Recommendations
Testing ChecklistBefore merging, please verify:
Minor Notes
VerdictHOLD - Cannot approve until the backend endpoint existence is confirmed. The frontend implementation looks reasonable, but it will fail at runtime if the backend does not support this endpoint. Once the backend endpoint is confirmed/implemented, the remaining issues are mostly minor improvements that could be addressed in follow-up work. |
This change addresses a bug where the frontend file download mechanism was not sending authentication headers. The fix involves updating the frontend to use an authenticated API client to fetch the file as a blob and then initiating the download from the browser. Unit tests have been added to verify the new functionality.
PR created automatically by Jules for task 5205579993703176681