feat(cache): auto clear stale download cache on startup - #1892
Open
icgnos wants to merge 1 commit into
Open
Conversation
Contributor
Reviewer's GuideAdds configurable automatic clearing of stale download cache entries on launcher startup, including UI controls, configuration model updates, and a Tauri-side async cleanup routine based on file modification time and a retention threshold. Sequence diagram for auto-clearing stale download cache on startupsequenceDiagram
participant Run as run
participant App as AppHandle
participant Config as LauncherConfig
participant Misc as auto_clear_download_cache
participant FS as tokio_fs
Run->>Config: read download.cache.auto_clear
Run->>Config: read download.cache.retention_hours
alt auto_clear_download_cache is true
Run->>App: handle
Run->>Misc: spawn auto_clear_download_cache(AppHandle)
Misc->>Config: read download.cache.directory
Misc->>Config: read download.cache.retention_hours
Misc->>FS: read_dir(cache_dir)
Misc->>FS: metadata(path)
Misc->>FS: remove_file(stale_path)
else auto_clear_download_cache is false
Run->>Run: skip cache cleanup
end
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
github-actions
Bot
added
the
size/L
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The
NumberInputforretentionHourscurrently allows an empty string which becomes0onNumber(value)and can briefly desync from the slider/max bounds; consider handling empty input explicitly (e.g., keeping previous value or defaulting to min) to avoid transient invalid state. - In
auto_clear_download_cache, aretention_hoursof0disables clearing even ifauto_clearis true; if this is not intended, you may want to rely solely on theauto_clearflag or ensure the UI cannot drive the config into a contradictory state.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `NumberInput` for `retentionHours` currently allows an empty string which becomes `0` on `Number(value)` and can briefly desync from the slider/max bounds; consider handling empty input explicitly (e.g., keeping previous value or defaulting to min) to avoid transient invalid state.
- In `auto_clear_download_cache`, a `retention_hours` of `0` disables clearing even if `auto_clear` is true; if this is not intended, you may want to rely solely on the `auto_clear` flag or ensure the UI cannot drive the config into a contradictory state.
## Individual Comments
### Comment 1
<location path="src/pages/settings/download/index.tsx" line_range="387-396" />
<code_context>
+ ),
+ children: (
+ <HStack spacing={4}>
+ <Slider
+ min={1}
+ max={168}
+ step={1}
+ w={32}
+ colorScheme={primaryColor}
+ value={sliderRetentionHours}
+ onChange={(value) => {
+ setSliderRetentionHours(value);
+ setRetentionHours(value);
+ }}
+ onBlur={() => {
+ update("download.cache.retentionHours", retentionHours);
+ }}
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Keep slider value within min/max bounds when syncing from state
`sliderRetentionHours` is set directly from `retentionHours`, but the slider only supports values in [1, 168]. If `retentionHours` ever goes outside this range (e.g., via existing configs or future changes), the slider could receive an invalid value and display incorrectly. Please clamp `retentionHours` to the slider’s min/max both when assigning `sliderRetentionHours` and when calling `update`.
Suggested implementation:
```typescript
<Slider
min={1}
max={168}
step={1}
w={32}
colorScheme={primaryColor}
value={Math.min(168, Math.max(1, sliderRetentionHours))}
onChange={(value) => {
setSliderRetentionHours(value);
setRetentionHours(value);
}}
onBlur={() => {
const clampedRetentionHours = Math.min(
168,
Math.max(1, retentionHours)
);
update("download.cache.retentionHours", clampedRetentionHours);
}}
```
To fully implement your comment, also ensure that any place where `sliderRetentionHours` is initialized or synchronized from `retentionHours` clamps the value, for example:
- If you have something like `setSliderRetentionHours(retentionHours);` (e.g., in a `useEffect` or initialization), change it to:
```ts
setSliderRetentionHours(Math.min(168, Math.max(1, retentionHours)));
```
This guarantees that both the state driving the slider and the value persisted via `update` always stay within the slider’s `[1, 168]` bounds.
</issue_to_address>
### Comment 2
<location path="src-tauri/src/launcher_config/helpers/misc.rs" line_range="273-278" />
<code_context>
+ .saturating_sub(retention_hours as u64 * 3600);
+
+ let mut stale_paths = Vec::new();
+ let mut entries = match tokio::fs::read_dir(&cache_dir).await {
+ Ok(entries) => entries,
+ Err(_) => return Ok(()),
+ };
+ while let Some(entry) = entries.next_entry().await? {
+ let file_type = match entry.file_type().await {
+ Ok(ft) => ft,
+ Err(_) => continue,
</code_context>
<issue_to_address>
**suggestion (performance):** Reuse metadata from `DirEntry` to avoid extra syscalls
Within the loop, `tokio::fs::metadata(&path)` is called for every entry even though `DirEntry` can already provide metadata. For large caches this roughly doubles metadata syscalls. Prefer `entry.metadata().await` and derive both `file_type` and `modified` from that single result to cut IO and simplify error handling.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+387
to
+396
| <Slider | ||
| min={1} | ||
| max={168} | ||
| step={1} | ||
| w={32} | ||
| colorScheme={primaryColor} | ||
| value={sliderRetentionHours} | ||
| onChange={(value) => { | ||
| setSliderRetentionHours(value); | ||
| setRetentionHours(value); |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): Keep slider value within min/max bounds when syncing from state
sliderRetentionHours is set directly from retentionHours, but the slider only supports values in [1, 168]. If retentionHours ever goes outside this range (e.g., via existing configs or future changes), the slider could receive an invalid value and display incorrectly. Please clamp retentionHours to the slider’s min/max both when assigning sliderRetentionHours and when calling update.
Suggested implementation:
<Slider
min={1}
max={168}
step={1}
w={32}
colorScheme={primaryColor}
value={Math.min(168, Math.max(1, sliderRetentionHours))}
onChange={(value) => {
setSliderRetentionHours(value);
setRetentionHours(value);
}}
onBlur={() => {
const clampedRetentionHours = Math.min(
168,
Math.max(1, retentionHours)
);
update("download.cache.retentionHours", clampedRetentionHours);
}}To fully implement your comment, also ensure that any place where sliderRetentionHours is initialized or synchronized from retentionHours clamps the value, for example:
- If you have something like
setSliderRetentionHours(retentionHours);(e.g., in auseEffector initialization), change it to:
setSliderRetentionHours(Math.min(168, Math.max(1, retentionHours)));This guarantees that both the state driving the slider and the value persisted via update always stay within the slider’s [1, 168] bounds.
Comment on lines
+273
to
+278
| let mut entries = match tokio::fs::read_dir(&cache_dir).await { | ||
| Ok(entries) => entries, | ||
| Err(_) => return Ok(()), | ||
| }; | ||
| while let Some(entry) = entries.next_entry().await? { | ||
| let file_type = match entry.file_type().await { |
Contributor
There was a problem hiding this comment.
suggestion (performance): Reuse metadata from DirEntry to avoid extra syscalls
Within the loop, tokio::fs::metadata(&path) is called for every entry even though DirEntry can already provide metadata. For large caches this roughly doubles metadata syscalls. Prefer entry.metadata().await and derive both file_type and modified from that single result to cut IO and simplify error handling.
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.
Checklist
This PR is a ..
Related Issues
Description
Summary by Sourcery
Add configurable automatic clearing of stale download cache files on startup.
New Features:
Enhancements: