feat(extraction): track archive extraction progress - #219
Conversation
- Report archive extraction and chunk merging progress - Display processing phases in download views
|
Review complete for PR #219. I've submitted my review as a GitHub PR review. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThe PR adds progress reporting for chunk merging and archive extraction. Electron IPC and preload events carry processing phases and progress values. Frontend state and views display determinate or indeterminate processing progress. ChangesProcessing progress
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant handler.fs.ts
participant extraction
participant preload.mts
participant DownloadManager.svelte
participant StorePage.svelte
handler.fs.ts->>extraction: Start extraction with progress callback
extraction-->>handler.fs.ts: Report extraction progress
handler.fs.ts->>preload.mts: Send processing:progress IPC event
preload.mts->>DownloadManager.svelte: Dispatch processing:progress DOM event
DownloadManager.svelte->>StorePage.svelte: Update processing phase and progress
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Nat3z
left a comment
Nat3z
left a comment
There was a problem hiding this comment.
Review by @nat-openclaw
Summary
Solid
},
{
"file": "packages/ogi-addon/src/extraction.ts",
"line": 324,
"side": "RIGHT",
"severity": "important",
"body": "Progress polling calls getRegularFileBytes(stagingDir), which recursively readdirs and stats the entire staging tree every 150ms. For game archives with tens of thousands of files, this adds significant I/O during extraction and can slow extraction itself (each walk may take seconds while the extractor is still writing).\n\nConsider throttling more aggressively (e.g. 500ms–1s), skipping polls while measuring is true and not scheduling another until the prior walk completes, or estimating progress from archive list metadata plus a single directory size check at a lower frequency."
}
]
### Verdict
### What Looks Good
---
<details>
<summary>Findings (2)</summary>
**important** — packages/ogi-addon/src/extraction.ts:284 — Windows 7-Zip output path is passed as a single `-o${stagingDir}` argument. This PR changed from separate `-o` and `outputDir` args; combined `-o` without quoting breaks when the install path contains spaces (common on Windows, e.g. `C:\Program Files\...`). Use separate arguments like the pre-PR code: ```typescript child = yield* spawnProcess( sevenZipPath, ['x', filePath, '-o', stagingDir, '-y'], { stdio: 'ignore' } ); ```
**important** — packages/ogi-addon/src/extraction.ts:324 — Progress polling calls `getRegularFileBytes(stagingDir)`, which recursively `readdir`s and `stat`s the entire staging tree every 150ms. For game archives with tens of thousands of files, this adds significant I/O during extraction and can slow extraction itself (each walk may take seconds while the extractor is still writing). Consider throttling more aggressively (e.g. 500ms–1s), skipping polls while `measuring` is true *and* not scheduling another until the prior walk completes, or estimating progress from archive list metadata plus a single directory size check at a lower frequency.
</details>
| if (process.platform === 'win32') { | ||
| child = yield* spawnProcess( | ||
| sevenZipPath, | ||
| ['x', filePath, `-o${stagingDir}`, '-y'], |
There was a problem hiding this comment.
Windows 7-Zip output path is passed as a single -o${stagingDir} argument. This PR changed from separate -o and outputDir args; combined -o without quoting breaks when the install path contains spaces (common on Windows, e.g. C:\Program Files\...).
Use separate arguments like the pre-PR code:
child = yield* spawnProcess(
sevenZipPath,
['x', filePath, '-o', stagingDir, '-y'],
{ stdio: 'ignore' }
);| return; | ||
| } | ||
| measuring = true; | ||
| activeMeasurement = getRegularFileBytes(stagingDir) |
There was a problem hiding this comment.
Progress polling calls getRegularFileBytes(stagingDir), which recursively readdirs and stats the entire staging tree every 150ms. For game archives with tens of thousands of files, this adds significant I/O during extraction and can slow extraction itself (each walk may take seconds while the extractor is still writing).
Consider throttling more aggressively (e.g. 500ms–1s), skipping polls while measuring is true and not scheduling another until the prior walk completes, or estimating progress from archive list metadata plus a single directory size check at a lower frequency.
|
I hit an infra error before I could finish: React to this comment with 🚀 to retry. |
Greptile SummaryAdds determinate and indeterminate progress reporting across archive extraction, chunk merging, and file-moving stages.
Confidence Score: 5/5The PR appears safe to merge; no concrete blocking or independently actionable non-blocking defect was established. The extraction lifecycle awaits archive processing, cleans its staging directory on both success and failure, preserves overwrite semantics during the staged merge, and propagates bounded progress state through the existing download lifecycle.
|
| Filename | Overview |
|---|---|
| packages/ogi-addon/src/extraction.ts | Adds staged extraction, archive-size discovery, periodic byte-based progress reporting, overwrite-aware merging, and guaranteed staging cleanup. |
| packages/ogi-addon/src/extraction-progress.ts | Adds platform/archive support checks and guarded parsers for 7-Zip, ZipInfo, and unrar listing output. |
| application/src/electron/handlers/handler.ddl.ts | Reports throttled byte progress while merging chunk files and identifies the processing phase in IPC events. |
| application/src/electron/handlers/handler.fs.ts | Forwards archive extraction progress to the renderer for downloads with an associated ID. |
| application/src/frontend/managers/DownloadManager.svelte | Tracks moving and extraction phases, maps indeterminate progress to NaN, and clears processing state on completion. |
| application/src/frontend/views/DownloadView.svelte | Renders processing phases with clamped determinate progress or a pulsing indeterminate bar. |
| application/src/frontend/components/StorePage.svelte | Updates the store download card to distinguish downloading from determinate or indeterminate processing. |
| packages/ogi-addon/tests/extraction-progress.test.ts | Covers archive support detection, unrar implementation detection, size parsing, and unsafe totals. |
Sequence Diagram
sequenceDiagram
participant UI as Download UI
participant Manager as DownloadManager
participant Electron as Electron handlers
participant Extractor as Archive extractor
participant FS as Staging filesystem
Electron->>Extractor: extraction(archive, output, onProgress)
Extractor->>FS: Create staging directory
Extractor-->>Manager: processing:progress (0 or indeterminate)
loop Every 150 ms
Extractor->>FS: Measure extracted regular-file bytes
Extractor-->>Manager: processing:progress (ratio)
Manager-->>UI: Update phase and progress
end
Extractor->>FS: Merge staged files into output
Extractor-->>Manager: processing:progress (1)
Manager-->>UI: Mark processing complete
Reviews (1): Last reviewed commit: "feat(extraction): track archive extracti..." | Re-trigger Greptile
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
application/src/electron/handlers/handler.ddl.ts (1)
1372-1373: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMulti-part merges do not report the new progress.
mergeChunkFilesForPartnow acceptsonProgress, but onlymergeChunkFilesat Line 2632 supplies it. The multi-part caller at Line 1186 omits it, so a multi-part download shows themergingstate without theMerging chunksphase and without merge progress. Pass a callback there as well to make the two paths consistent.♻️ Proposed change at the multi-part call site
// Merge chunk files - yield* this.mergeChunkFilesForPart(part); + yield* this.mergeChunkFilesForPart(part, (progress) => + this.sendProgress({ progress, processingPhase: 'Merging chunks' }) + );🤖 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 `@application/src/electron/handlers/handler.ddl.ts` around lines 1372 - 1373, Update the multi-part caller of mergeChunkFilesForPart to pass the existing merge-progress callback, matching the callback supplied by mergeChunkFiles. Ensure multi-part downloads report the “Merging chunks” phase and incremental merge progress while preserving the single-part behavior.application/src/frontend/components/StorePage.svelte (1)
579-586: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winIndeterminate progress bars lack progress semantics and a reduced-motion guard. Both views added a full-width pulsing bar for indeterminate processing. Neither bar declares progress semantics, so a screen reader user cannot perceive that processing is running or how far it has advanced.
animate-pulsealso runs continuously and ignoresprefers-reduced-motion.
application/src/frontend/components/StorePage.svelte#L579-L586: addrole="progressbar"witharia-valuemin/aria-valuemax, setaria-valuenowonly whenhasDeterminateProgress(activeDownload.progress)is true, setaria-valuetextto the processing phase otherwise, and changeclass:animate-pulsetoclass:motion-safe:animate-pulse.application/src/frontend/views/DownloadView.svelte#L609-L619: add the samerole="progressbar"attributes to both branches, setaria-valuenowon the determinate branch andaria-valuetexton the indeterminate branch, and replaceanimate-pulsewithmotion-safe:animate-pulse.🤖 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 `@application/src/frontend/components/StorePage.svelte` around lines 579 - 586, Update the progress-bar elements in application/src/frontend/components/StorePage.svelte lines 579-586 and application/src/frontend/views/DownloadView.svelte lines 609-619: add role="progressbar" with aria-valuemin and aria-valuemax, expose aria-valuenow only for determinate progress and aria-valuetext with the processing phase for indeterminate progress, and replace animate-pulse with motion-safe:animate-pulse.
🤖 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 `@application/src/frontend/views/DownloadView.svelte`:
- Around line 621-626: Update the progress display in the DownloadView
progress-stats markup so the ellipsis is rendered only when
hasDeterminateProgress(download.progress) is false, and change determinate
percentage formatting to Math.round(download.progress * 100), matching
StorePage.svelte.
In `@packages/ogi-addon/src/extraction.ts`:
- Around line 365-370: Update the cleanup effect in the extraction pipeline’s
Effect.ensuring block so fsAsync.rm failures are caught and converted into a
successful, non-failing cleanup result. Keep staging cleanup best-effort while
ensuring cleanup rejection cannot become an untyped defect or alter the
successful extraction outcome.
---
Nitpick comments:
In `@application/src/electron/handlers/handler.ddl.ts`:
- Around line 1372-1373: Update the multi-part caller of mergeChunkFilesForPart
to pass the existing merge-progress callback, matching the callback supplied by
mergeChunkFiles. Ensure multi-part downloads report the “Merging chunks” phase
and incremental merge progress while preserving the single-part behavior.
In `@application/src/frontend/components/StorePage.svelte`:
- Around line 579-586: Update the progress-bar elements in
application/src/frontend/components/StorePage.svelte lines 579-586 and
application/src/frontend/views/DownloadView.svelte lines 609-619: add
role="progressbar" with aria-valuemin and aria-valuemax, expose aria-valuenow
only for determinate progress and aria-valuetext with the processing phase for
indeterminate progress, and replace animate-pulse with
motion-safe:animate-pulse.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0daad6c0-e3dc-4638-b19b-3f8575c0e0fe
📒 Files selected for processing (10)
application/src/electron/handlers/handler.ddl.tsapplication/src/electron/handlers/handler.fs.tsapplication/src/electron/preload.mtsapplication/src/frontend/components/StorePage.svelteapplication/src/frontend/managers/DownloadManager.svelteapplication/src/frontend/store.svelte.tsapplication/src/frontend/views/DownloadView.sveltepackages/ogi-addon/src/extraction-progress.tspackages/ogi-addon/src/extraction.tspackages/ogi-addon/tests/extraction-progress.test.ts
| <div class="progress-stats"> | ||
| <span class="progress-percentage"> | ||
| Merging files... | ||
| {download.processingPhase ?? 'Processing files'}... | ||
| {#if hasDeterminateProgress(download.progress)} | ||
| {Math.floor(download.progress * 100)}% | ||
| {/if} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Ellipsis and percentage contradict each other.
Line 623 always appends ..., and Line 625 then appends the percentage. A determinate merge renders Merging chunks... 42%. StorePage.svelte renders the same state as Merging chunks 42% and uses Math.round instead of Math.floor. Show the ellipsis only when progress is indeterminate, and use the same rounding as StorePage.svelte.
🐛 Proposed fix
<span class="progress-percentage">
- {download.processingPhase ?? 'Processing files'}...
{`#if` hasDeterminateProgress(download.progress)}
- {Math.floor(download.progress * 100)}%
+ {download.processingPhase ?? 'Processing files'}
+ {Math.round(download.progress * 100)}%
+ {:else}
+ {download.processingPhase ?? 'Processing files'}...
{/if}📝 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.
| <div class="progress-stats"> | |
| <span class="progress-percentage"> | |
| Merging files... | |
| {download.processingPhase ?? 'Processing files'}... | |
| {#if hasDeterminateProgress(download.progress)} | |
| {Math.floor(download.progress * 100)}% | |
| {/if} | |
| <div class="progress-stats"> | |
| <span class="progress-percentage"> | |
| {`#if` hasDeterminateProgress(download.progress)} | |
| {download.processingPhase ?? 'Processing files'} | |
| {Math.round(download.progress * 100)}% | |
| {:else} | |
| {download.processingPhase ?? 'Processing files'}... | |
| {/if} |
🤖 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 `@application/src/frontend/views/DownloadView.svelte` around lines 621 - 626,
Update the progress display in the DownloadView progress-stats markup so the
ellipsis is rendered only when hasDeterminateProgress(download.progress) is
false, and change determinate percentage formatting to
Math.round(download.progress * 100), matching StorePage.svelte.
| }).pipe( | ||
| Effect.ensuring( | ||
| Effect.promise(() => | ||
| fsAsync.rm(stagingDir, { recursive: true, force: true }) | ||
| ) | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Cleanup rejection becomes an untyped defect.
Effect.promise treats a rejected promise as a defect, not a typed failure. fsAsync.rm with force: true still rejects for EPERM, EBUSY, or ENOTEMPTY, which occurs on Windows when an antivirus or the extractor still holds a handle on a staged file. A successful extraction then dies with a defect that ExtractionError does not describe, and the caller in handler.fs.ts cannot convert it to a user-facing message.
Make the cleanup non-failing.
🛡️ Proposed fix to make staging cleanup non-failing
}).pipe(
Effect.ensuring(
- Effect.promise(() =>
- fsAsync.rm(stagingDir, { recursive: true, force: true })
- )
+ Effect.tryPromise({
+ try: () => fsAsync.rm(stagingDir, { recursive: true, force: true }),
+ catch: (cause) => cause,
+ }).pipe(Effect.ignore)
)
);📝 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.
| }).pipe( | |
| Effect.ensuring( | |
| Effect.promise(() => | |
| fsAsync.rm(stagingDir, { recursive: true, force: true }) | |
| ) | |
| ) | |
| }).pipe( | |
| Effect.ensuring( | |
| Effect.tryPromise({ | |
| try: () => fsAsync.rm(stagingDir, { recursive: true, force: true }), | |
| catch: (cause) => cause, | |
| }).pipe(Effect.ignore) | |
| ) | |
| ); |
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { type ChildProcess, spawn } from 'node:child_process';
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 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 `@packages/ogi-addon/src/extraction.ts` around lines 365 - 370, Update the
cleanup effect in the extraction pipeline’s Effect.ensuring block so fsAsync.rm
failures are caught and converted into a successful, non-failing cleanup result.
Keep staging cleanup best-effort while ensuring cleanup rejection cannot become
an untyped defect or alter the successful extraction outcome.
1 participant
Summary
Testing
Summary by CodeRabbit
New Features
Bug Fixes