perf: speed up existing webui content downloads#1482
perf: speed up existing webui content downloads#1482
Conversation
s0up4200
changed the title
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d432f080a9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
No actionable comments were generated in the recent review. 🎉 WalkthroughAdds disk-based torrent file download handling with traversal-safe path resolution, local-filesystem access gating, a fast-path direct stream for full-file downloads (with write-deadline), and a compression bypass for the download endpoint. Also updates TorrentsHandler constructor to accept an instanceStore and adjusts related tests and UI guards for incognito/local access. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Handler as DownloadTorrentContentFile
participant AccessCheck as requireLocalAccess
participant Resolver as syncManager / contentResolver
participant PathUtil as resolveTorrentFilePath
participant FS as Filesystem
Client->>Handler: GET /api/instances/{id}/torrents/{hash}/files/{index}/download
Handler->>AccessCheck: verify local filesystem access
alt access denied
AccessCheck-->>Client: 403 Forbidden
else access granted
Handler->>Resolver: fetch torrent metadata
Resolver-->>Handler: file list & metadata
Handler->>PathUtil: build ordered path candidates (save, download, content)
PathUtil-->>Handler: validated absolute paths
Handler->>FS: open selected file
FS-->>Handler: file handle & stats
alt full-file fast-path
Handler-->>Client: set headers (Accept-Ranges, Content-Length), stream file (write-deadline)
else fallback
Handler-->>Client: ServeContent fallback (supports 304/412)
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
…load-speedup-safe # Conflicts: # internal/api/handlers/torrents.go # internal/api/handlers/torrents_download_test.go
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
web/src/components/torrents/details/TorrentFileTable.tsx (1)
469-480: Consider addingdisabled={incognitoMode}to Rename menu items for consistency.The new Download item is correctly disabled in incognito mode, matching the pattern in
TorrentFileTree.tsx. However, the existing Rename File/Folder items in this component don't have thedisabledprop, unlike their counterparts inTorrentFileTree.tsx(Line 367, 464). This is a pre-existing inconsistency but worth addressing for UI consistency.♻️ Suggested fix for consistency
{isFile && onRenameFile && ( - <ContextMenuItem onClick={() => onRenameFile(node.id)}> + <ContextMenuItem onClick={() => onRenameFile(node.id)} disabled={incognitoMode}> <Pencil className="h-3.5 w-3.5 mr-2" /> Rename File </ContextMenuItem> )} {!isFile && onRenameFolder && ( - <ContextMenuItem onClick={() => onRenameFolder(node.id)}> + <ContextMenuItem onClick={() => onRenameFolder(node.id)} disabled={incognitoMode}> <Pencil className="h-3.5 w-3.5 mr-2" /> Rename Folder </ContextMenuItem> )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/torrents/details/TorrentFileTable.tsx` around lines 469 - 480, Add the same incognito-mode disabling to the Rename menu items: when rendering the ContextMenuItem for rename, pass disabled={incognitoMode} so both the Rename File and Rename Folder options respect incognitoMode; update the two places that render ContextMenuItem with onRenameFile(node.id) and onRenameFolder(node.id) (symbols: isFile, onRenameFile, onRenameFolder, ContextMenuItem, incognitoMode in TorrentFileTable.tsx) to include the disabled prop.internal/api/server.go (1)
678-691: Path matching logic is correct but could be more explicit.The function correctly identifies the download endpoint pattern by checking the last 8 path segments, which handles the baseURL prefix gracefully. The check at indices 0, 1, 3, 5, 7 matches the static parts (
api,instances,torrents,files,download) while skipping the dynamic parameters.Consider adding a brief comment explaining why the last 8 segments are used (to handle baseURL prefixes).
📝 Suggested documentation
func skipCompressionForPath(requestPath string) bool { cleanPath := path.Clean("/" + requestPath) segments := strings.Split(strings.Trim(cleanPath, "/"), "/") if len(segments) < 8 { return false } + // Check last 8 segments to handle baseURL prefixes (e.g., /qui/api/...) tail := segments[len(segments)-8:] return tail[0] == "api" && tail[1] == "instances" && tail[3] == "torrents" && tail[5] == "files" && tail[7] == "download" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/server.go` around lines 678 - 691, The function skipCompressionForPath uses the last 8 path segments to detect the download endpoint but lacks context; add a concise comment above skipCompressionForPath explaining that trimming to the last 8 segments (tail) is intentional to allow arbitrary baseURL prefixes while still matching the fixed pattern "api/instances/:instanceID/torrents/:torrentID/files/:fileID/download"; mention variables segments and tail in the comment so future readers understand why the length check and indexed comparisons (tail[0], tail[1], tail[3], tail[5], tail[7]) are used.internal/api/handlers/torrents_download_test.go (1)
138-156: The fixture reuse suggestion cannot be implemented — no integration fixtures exist ininternal/qbittorrent/.While table-driven tests are preferred per guidelines, consolidating these tests would reduce clarity: they test distinct error scenarios (missing instance, forbidden access, missing file, traversal attacks, nil properties) with different mock setups and assertions. The existing helpers (
createInstanceStoreWithInstance,newDownloadRequest) already reduce duplication. Consider table-driven consolidation only for tests sharing identical setup and assertion patterns.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/handlers/torrents_download_test.go` around lines 138 - 156, These tests cover distinct error scenarios and cannot be safely merged into a table-driven test because each uses different mock setups and assertions; keep the separate test functions (e.g., TestDownloadTorrentContentFile_ReturnsNotFoundForMissingInstance) and existing helpers (createInstanceStoreWithInstance, newDownloadRequest, mockContentResolver) as-is, and add a brief comment above the block of tests explaining why a table-driven refactor or shared integration fixture under internal/qbittorrent/ was not used to prevent future maintainers from attempting the consolidation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/src/components/torrents/TorrentDetailsPanel.tsx`:
- Around line 658-662: The download action should not be exposed when
incognitoMode is true: update the UI rendering in TorrentDetailsPanel to hide or
disable any download buttons/menu items rather than relying on the early return
in handleDownloadFile; locate the download action render points (the elements
that call handleDownloadFile and related menu items in this component) and
conditionally render nothing or a disabled control when incognitoMode is true,
and keep handleDownloadFile as a defensive guard. Ensure the UX is consistent
across all places the download action appears (including the other download
action occurrences referenced in this file).
---
Nitpick comments:
In `@internal/api/handlers/torrents_download_test.go`:
- Around line 138-156: These tests cover distinct error scenarios and cannot be
safely merged into a table-driven test because each uses different mock setups
and assertions; keep the separate test functions (e.g.,
TestDownloadTorrentContentFile_ReturnsNotFoundForMissingInstance) and existing
helpers (createInstanceStoreWithInstance, newDownloadRequest,
mockContentResolver) as-is, and add a brief comment above the block of tests
explaining why a table-driven refactor or shared integration fixture under
internal/qbittorrent/ was not used to prevent future maintainers from attempting
the consolidation.
In `@internal/api/server.go`:
- Around line 678-691: The function skipCompressionForPath uses the last 8 path
segments to detect the download endpoint but lacks context; add a concise
comment above skipCompressionForPath explaining that trimming to the last 8
segments (tail) is intentional to allow arbitrary baseURL prefixes while still
matching the fixed pattern
"api/instances/:instanceID/torrents/:torrentID/files/:fileID/download"; mention
variables segments and tail in the comment so future readers understand why the
length check and indexed comparisons (tail[0], tail[1], tail[3], tail[5],
tail[7]) are used.
In `@web/src/components/torrents/details/TorrentFileTable.tsx`:
- Around line 469-480: Add the same incognito-mode disabling to the Rename menu
items: when rendering the ContextMenuItem for rename, pass
disabled={incognitoMode} so both the Rename File and Rename Folder options
respect incognitoMode; update the two places that render ContextMenuItem with
onRenameFile(node.id) and onRenameFolder(node.id) (symbols: isFile,
onRenameFile, onRenameFolder, ContextMenuItem, incognitoMode in
TorrentFileTable.tsx) to include the disabled prop.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5610c786d2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
1 participant
Improves performance of the existing torrent content-file download flow in the WebUI by bypassing HTTP compression on that download endpoint. Keeps existing local-filesystem access checks and path-safety validation while reducing CPU overhead and improving large-file throughput.
Summary by CodeRabbit
New Features
Improvements
Tests