feat: update UI to show estimated deletion date instead of default#205
feat: update UI to show estimated deletion date instead of default#205walbertus wants to merge 10 commits intojon4hz:mainfrom
Conversation
|
@jon4hz please take a look at this when you're available, thanks |
|
Hi, thank you so much for the PR! |
|
updated, sorry for the delay @jon4hz |
|
can you rebase the branch to main please? |
c4fd19f to
8a9a175
Compare
|
updated @jon4hz |
There was a problem hiding this comment.
Pull request overview
This pull request implements functionality to display estimated deletion dates in the UI that take into account disk usage policies, not just the default cleanup delay. Previously, the UI only showed DefaultDeleteAt, which didn't reflect when disk usage policies might trigger earlier deletion.
Changes:
- Added
EstimatedDeleteAtfield to database Media model and API models - Implemented
GetEstimatedDeleteAtmethod in policy interface to calculate earliest deletion time considering all policies - Created scheduled job (every 6 hours) and cleanup job hook to recalculate estimated deletion dates
- Updated all frontend JavaScript/TypeScript code to use
EstimatedDeleteAtinstead ofDefaultDeleteAtwith fallback logic
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/database/media.go | Added EstimatedDeleteAt field to Media model and SetMediaEstimatedDeleteAt method |
| internal/database/interface.go | Added SetMediaEstimatedDeleteAt to MediaDB interface |
| internal/policy/policy.go | Added GetEstimatedDeleteAt to Policy interface and implemented in Engine |
| internal/policy/default.go | Implemented GetEstimatedDeleteAt to return DefaultDeleteAt |
| internal/policy/disk_usage.go | Implemented GetEstimatedDeleteAt to return earliest deletion date when threshold exceeded, refactored disk usage logic |
| internal/engine/estimate.go | New file implementing the estimation job that updates EstimatedDeleteAt for all media items |
| internal/engine/engine.go | Integrated estimation job to run at end of cleanup job |
| internal/engine/scheduler.go | Added scheduled job to run estimation every 6 hours |
| internal/api/models/models.go | Added EstimatedDeleteAt field to UserMediaItem and AdminMediaItem |
| internal/api/models/converter.go | Updated converters to include EstimatedDeleteAt with fallback to DefaultDeleteAt |
| internal/api/handler/plugin.go | Updated CheckMediaItem to use EstimatedDeleteAt with fallback |
| web/templates/pages/dashboard.templ | Updated JavaScript to use EstimatedDeleteAt instead of DefaultDeleteAt |
| web/templates/pages/dashboard_templ.go | Generated file reflecting dashboard.templ changes |
| web/templates/components/stats.templ | Updated stats calculations to use EstimatedDeleteAt |
| web/templates/components/stats_templ.go | Generated file reflecting stats.templ changes |
| web/templates/components/dashboard-media-grid.templ | Updated media grid to use EstimatedDeleteAt |
| web/templates/components/dashboard-media-grid_templ.go | Generated file reflecting dashboard-media-grid.templ changes |
| web/templates/components/admin-media-grid.templ | Updated admin media grid to use EstimatedDeleteAt |
| web/templates/components/admin-media-grid_templ.go | Generated file reflecting admin-media-grid.templ changes |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for _, item := range mediaItems { | ||
| estimatedDeleteAt, err := e.policy.GetEstimatedDeleteAt(ctx, item) | ||
| if err != nil { | ||
| log.Error("failed to estimate delete at for media item", "itemID", item.ID, "error", err) | ||
| continue | ||
| } | ||
| err = e.db.SetMediaEstimatedDeleteAt(ctx, item.ID, estimatedDeleteAt) | ||
| if err != nil { | ||
| log.Error("failed to set estimated delete at for media item", "itemID", item.ID, "error", err) | ||
| continue | ||
| } | ||
| log.Debug("estimated delete at set for media item", "itemID", item.ID, "estimatedDeleteAt", estimatedDeleteAt) | ||
| } | ||
|
|
||
| log.Info("estimate deletions job completed successfully") | ||
| return nil | ||
| } |
There was a problem hiding this comment.
The estimate deletions job calls GetEstimatedDeleteAt for each media item, which in turn calls getCurrentDiskUsage for each item. If multiple media items belong to the same library, the disk usage will be queried repeatedly for the same library folders. Consider caching disk usage results per library within the estimation job to improve performance, especially for libraries with many media items.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| func (p *DiskUsageDelete) GetEstimatedDeleteAt(ctx context.Context, media database.Media) (time.Time, error) { | ||
| if len(media.DiskUsageDeletePolicies) == 0 { | ||
| return time.Time{}, nil | ||
| } | ||
|
|
||
| currentDiskUsage, err := p.getCurrentDiskUsage(ctx, media.LibraryName) | ||
| if err != nil { | ||
| return 0, err | ||
| return time.Time{}, err | ||
| } |
There was a problem hiding this comment.
I'll see what I can do about this in a different PR. Copilot has a point that querying the disk usage for every library item isn't very efficient.
|
Hi @walbertus, Would you mind taking a look at the conflict? |
- Simplify redundant function in internal/database/media.go - Checked internal/engine/estimate.go for hidden Unicode (none found)
- Add Select() to Updates() calls to ensure zero values are saved - Affects SetMediaEstimatedDeleteAt, SetMediaProtectedUntil, and MarkMediaAsUnkeepable - Without Select(), GORM skips zero-value fields (zero time, nil pointers, false bools) - Fixes potential data inconsistency when clearing or setting zero values
9dd4293 to
5f7c3b9
Compare
|
I'll recheck this in a moment ya, it seem my last force push is wrong. It's been few months, I need time to revisit this |
|
Hi @walbertus, Thanks for rebasing, and sorry that I couldn't give this PR more attention sooner. I'll try to test this in the next few days and if everything looks fine, we'll |
this is done by adding new job schedule to recalculate the estimate for deletion date. This function also called during the cleanup job
close #203