Add directory download support to backups and file download APIs - #18
Merged
Conversation
|
Warning Review limit reached
More reviews will be available in 30 minutes and 14 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThis PR implements end-to-end directory ZIP download functionality. A new DB migration (015) creates the ChangesDirectory Download Feature
Sequence DiagramsequenceDiagram
participant User
participant FileTree
participant GoServer as Go HTTP Server
participant BackupService
participant SQLite
User->>FileTree: clicks Download on directory row
FileTree->>GoServer: POST /api/file-tree/directory-download-link?path=...
GoServer->>BackupService: PrepareDirectoryDownload(ctx, path, userID)
BackupService->>SQLite: GetRunningBackupRunForTag("directory_download")
alt archive already cached and ready
BackupService->>SQLite: GetDirectoryDownloadArchive(normalizedPath, fingerprint)
SQLite-->>BackupService: DirectoryDownloadArchive
BackupService-->>GoServer: DirectoryDownloadResult{Status: Ready}
GoServer->>SQLite: CreateFileDownloadLink(source=directory_download)
GoServer-->>FileTree: 200 + download_url
FileTree->>User: navigates to download_url
else no cached archive
BackupService->>SQLite: CreateBackupJob(tag=directory_download) + CreateBackupRun
BackupService-->>GoServer: DirectoryDownloadResult{Status: Started, RunID}
GoServer-->>FileTree: 202 + run_id
loop poll until ready
FileTree->>GoServer: GET /api/file-tree/directory-downloads/{run_id}
GoServer->>BackupService: GetDirectoryDownloadStatus(ctx, runID)
BackupService->>SQLite: GetBackupRun + GetDirectoryDownloadArchive
GoServer-->>FileTree: 200 {status: ready, download_url} or 202
end
FileTree->>User: navigates to download_url
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
cmd/omnihance-a3-agent/docs/openapi.yml (1)
4622-4661: ⚡ Quick winMake core
DirectoryDownloadResponsefields required to match consumers.Line 4622 currently leaves all properties optional. The consumer contract expects
status,message,job_id, andrun_idto always exist; optionalizing them weakens generated types and can drift from runtime expectations.🛠️ Proposed OpenAPI patch
DirectoryDownloadResponse: type: object + required: + - status + - message + - job_id + - run_id properties: status: type: string🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/omnihance-a3-agent/docs/openapi.yml` around lines 4622 - 4661, The DirectoryDownloadResponse schema in the OpenAPI specification currently has all properties optional, but the consumer contract expects status, message, job_id, and run_id to always be present. Add a required array property to the DirectoryDownloadResponse object that lists these four fields (status, message, job_id, run_id) to enforce them as mandatory in the generated types and match runtime expectations.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/omnihance-a3-agent/docs/openapi.yml`:
- Around line 341-392: The secured endpoints in the OpenAPI specification
declare ApiKeyAuth but are missing documentation for the 401 Unauthorized
response, which is required to complete the auth contract for client generation.
Add a 401 response definition to both the /api/file-tree/directory-download-link
endpoint (at lines 341-392) and the other affected endpoint (at lines 393-438)
that use ApiKeyAuth security. Each 401 response should reference the
ErrorResponse schema and describe authentication failure as the reason.
In `@cmd/omnihance-a3-agent/omnihance-a3-agent-ui/src/components/file-tree.tsx`:
- Around line 609-629: The refetchInterval callback in the useQuery
configuration does not handle query errors, causing endless polling when the
status fetch fails because query.state.data is empty. Modify the refetchInterval
function to check if query.state.error exists before checking the status; if an
error is present, return false to stop polling. Only return the 2000ms interval
if there is valid data and the status is not one of the terminal states (ready,
failed, or cancelled).
- Around line 892-898: The isDownloadPending state calculation for directories
only checks if the downloadDirectoryMutation is currently in-flight, but does
not account for the polling state that continues after the mutation completes.
To fix this, extend the pending state check for directories to also include a
check for any active polling operation associated with the same itemPath.
Specifically, add a condition that checks whether there is an ongoing poll for
the directory (likely checking a polling state variable or hook) in addition to
the existing downloadDirectoryMutation.isPending check, so that the row remains
in a pending state while the directory download is being polled/processed in the
background.
In `@internal/services/backup_service.go`:
- Around line 452-459: The issue is that in the PrepareDirectoryDownload
function, the call to runningDirectoryDownloadResult happens before checking the
fingerprint and archive cache for reusable cached archives. This causes
unnecessary 409 conflict errors when a ready cached archive exists. Reorder the
logic so that the archive cache lookup occurs before the
runningDirectoryDownloadResult check, allowing cached archives to be reused
immediately without being blocked by running jobs for other directories.
---
Nitpick comments:
In `@cmd/omnihance-a3-agent/docs/openapi.yml`:
- Around line 4622-4661: The DirectoryDownloadResponse schema in the OpenAPI
specification currently has all properties optional, but the consumer contract
expects status, message, job_id, and run_id to always be present. Add a required
array property to the DirectoryDownloadResponse object that lists these four
fields (status, message, job_id, run_id) to enforce them as mandatory in the
generated types and match runtime expectations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5b216af9-77d8-423c-8357-26304a6ae96d
📒 Files selected for processing (17)
cmd/omnihance-a3-agent/docs/openapi.ymlcmd/omnihance-a3-agent/omnihance-a3-agent-ui/src/components/backup-page.tsxcmd/omnihance-a3-agent/omnihance-a3-agent-ui/src/components/file-tree.tsxcmd/omnihance-a3-agent/omnihance-a3-agent-ui/src/constants.tscmd/omnihance-a3-agent/omnihance-a3-agent-ui/src/lib/api.tsinternal/config/config.gointernal/db/backup_jobs.gointernal/db/directory_downloads.gointernal/db/file_downloads.gointernal/db/internal_db.gointernal/db/mock_InternalDB.gointernal/server/file_download_routes.gointernal/server/file_download_routes_test.gointernal/server/file_system_routes.gointernal/services/backup_service.gointernal/services/backup_service_test.gointernal/services/mock_BackupService.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Testing
Summary by CodeRabbit
Release Notes
New Features