Repository navigation
Fix initialization problem: add progress tracking for single feed and OPML import - #108
Conversation
Co-authored-by: WCY-dt <55525165+WCY-dt@users.noreply.github.com>
… OPML import Co-authored-by: WCY-dt <55525165+WCY-dt@users.noreply.github.com>
Co-authored-by: WCY-dt <55525165+WCY-dt@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes a critical initialization issue where newly added feeds and OPML imports did not immediately fetch articles, and progress tracking was incomplete for single-feed operations. The solution involves returning feed IDs from subscription methods, adding new FetchSingleFeed and FetchFeedsByIDs methods with proper progress tracking, and updating the frontend to poll for progress after these operations.
Key Changes:
- Backend now returns feed IDs from
AddFeedto enable immediate article fetching - New progress-tracked fetch methods (
FetchSingleFeed,FetchFeedsByIDs) ensure UI progress indicators work correctly - Frontend triggers progress polling after feed additions, OPML imports, and single-feed refreshes
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
internal/database/feed_db.go |
Modified AddFeed to return feed ID for immediate use after feed creation |
internal/feed/fetcher.go |
Added FetchSingleFeed and FetchFeedsByIDs with progress tracking; refactored to use extracted helpers |
internal/feed/subscription.go |
Extracted subscription methods from fetcher.go; all now return feed IDs |
internal/feed/progress.go |
New file with progress tracking utilities and helpers |
internal/feed/article_processor.go |
Extracted article processing logic from fetcher.go for better code organization |
internal/handlers/feed/feed_handlers.go |
Updated to trigger immediate background fetch after feed addition; simplified refresh handler |
internal/handlers/opml/opml_handlers.go |
Collects feed IDs during import and uses FetchFeedsByIDs for progress-tracked batch fetch |
internal/feed/fetcher_test.go |
Updated test calls to handle new return value from subscription methods |
internal/database/*_test.go |
Updated test calls to handle new AddFeed signature |
frontend/src/App.vue |
Triggers progress polling after feed addition instead of immediate article fetch |
frontend/src/composables/core/useSidebar.ts |
Triggers progress polling for feed refresh instead of fixed delay |
frontend/src/composables/feed/useFeedManagement.ts |
Triggers progress polling after OPML import |
frontend/src/components/article/parts/*.vue |
Replaced hardcoded English strings with i18n translation keys |
frontend/src/i18n/**/*.ts |
Added noContentAvailable translation key for article display |
| } | ||
|
|
||
| // waitForProgressComplete waits for any running operation to complete with a timeout. | ||
| // Returns true if the wait was successful, false if timeout occurred. |
There was a problem hiding this comment.
[nitpick] The comment mentions that this helper prevents infinite loops with a 5-minute timeout, but this function is used with varying timeout values. The comment should be more generic.
Suggestion:
// waitForProgressComplete waits for any running operation to complete with a timeout.
// Returns true if the operation completed within the timeout, false if timeout occurred.| // Returns true if the wait was successful, false if timeout occurred. | |
| // Returns true if the operation completed within the timeout, false if timeout occurred. |
| func (f *Fetcher) AddSubscription(url string, category string, customTitle string) (int64, error) { | ||
| parsedFeed, err := f.fp.ParseURL(url) | ||
| if err != nil { | ||
| return 0, err | ||
| } | ||
|
|
||
| title := parsedFeed.Title | ||
| if customTitle != "" { | ||
| title = customTitle | ||
| } | ||
|
|
||
| feed := &models.Feed{ | ||
| Title: title, | ||
| URL: url, | ||
| Link: parsedFeed.Link, | ||
| Description: parsedFeed.Description, | ||
| Category: category, | ||
| } | ||
|
|
||
| if parsedFeed.Image != nil { | ||
| feed.ImageURL = parsedFeed.Image.URL | ||
| } | ||
|
|
||
| return f.db.AddFeed(feed) | ||
| } | ||
|
|
||
| // AddScriptSubscription adds a new feed subscription that uses a custom script | ||
| // and returns the feed ID. | ||
| func (f *Fetcher) AddScriptSubscription(scriptPath string, category string, customTitle string) (int64, error) { | ||
| // Validate script path | ||
| if f.scriptExecutor == nil { | ||
| return 0, &ScriptError{Message: "script executor not initialized"} | ||
| } | ||
|
|
||
| // Execute script to get initial feed info | ||
| ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) | ||
| defer cancel() | ||
|
|
||
| parsedFeed, err := f.scriptExecutor.ExecuteScript(ctx, scriptPath) | ||
| if err != nil { | ||
| return 0, err | ||
| } | ||
|
|
||
| title := parsedFeed.Title | ||
| if customTitle != "" { | ||
| title = customTitle | ||
| } | ||
|
|
||
| // Use a placeholder URL for script-based feeds | ||
| url := "script://" + scriptPath | ||
|
|
||
| feed := &models.Feed{ | ||
| Title: title, | ||
| URL: url, | ||
| Link: parsedFeed.Link, | ||
| Description: parsedFeed.Description, | ||
| Category: category, | ||
| ScriptPath: scriptPath, | ||
| } | ||
|
|
||
| if parsedFeed.Image != nil { | ||
| feed.ImageURL = parsedFeed.Image.URL | ||
| } | ||
|
|
||
| return f.db.AddFeed(feed) | ||
| } | ||
|
|
||
| // ImportSubscription imports a feed subscription and returns the feed ID. | ||
| func (f *Fetcher) ImportSubscription(title, url, category string) (int64, error) { | ||
| feed := &models.Feed{ | ||
| Title: title, | ||
| URL: url, | ||
| Link: "", // Link will be fetched later when feed is refreshed | ||
| Category: category, | ||
| } | ||
| return f.db.AddFeed(feed) | ||
| } |
There was a problem hiding this comment.
Missing context parameter for subscription methods. The methods AddSubscription, AddScriptSubscription, and ImportSubscription perform network operations and database writes but don't accept a context.Context parameter. According to the project guideline "Always use context.Context for exported methods", these should accept context to allow cancellation and timeout control.
Consider updating the signatures:
func (f *Fetcher) AddSubscription(ctx context.Context, url string, category string, customTitle string) (int64, error)
func (f *Fetcher) AddScriptSubscription(ctx context.Context, scriptPath string, category string, customTitle string) (int64, error)
func (f *Fetcher) ImportSubscription(ctx context.Context, title, url, category string) (int64, error)Note that AddScriptSubscription already creates a context internally (line 47), but it should respect a parent context passed from the caller.
| f.mu.Lock() | ||
| if f.progress.IsRunning { | ||
| f.mu.Unlock() | ||
| // Wait for current operation to complete with timeout | ||
| if !f.waitForProgressComplete(5 * time.Minute) { | ||
| log.Println("FetchSingleFeed: Timeout waiting for previous operation") | ||
| return | ||
| } | ||
| f.mu.Lock() | ||
| } | ||
| f.progress.IsRunning = true | ||
| f.progress.Total = 1 | ||
| f.progress.Current = 0 | ||
| f.mu.Unlock() |
There was a problem hiding this comment.
There's a potential race condition in the progress tracking logic. After unlocking at line 220, another goroutine could set IsRunning = true before this function sets it at line 217. The gap between unlock and re-lock allows this race.
Consider using the startProgress helper function which handles this atomically, or ensure atomic check-and-set:
if !f.startProgress(1) {
// Wait for current operation to complete with timeout
if !f.waitForProgressComplete(5 * time.Minute) {
log.Println("FetchSingleFeed: Timeout waiting for previous operation")
return
}
// Retry once after waiting
if !f.startProgress(1) {
log.Println("FetchSingleFeed: Another operation started during wait")
return
}
}The same issue exists in FetchFeedsByIDs at lines 241-254.
| // FetchSingleFeed fetches a single feed with progress tracking. | ||
| // This is used when adding a new feed or refreshing a single feed from the context menu. | ||
| func (f *Fetcher) FetchSingleFeed(ctx context.Context, feed models.Feed) { | ||
| f.mu.Lock() | ||
| if f.progress.IsRunning { | ||
| f.mu.Unlock() | ||
| // Wait for current operation to complete with timeout | ||
| if !f.waitForProgressComplete(5 * time.Minute) { | ||
| log.Println("FetchSingleFeed: Timeout waiting for previous operation") | ||
| return | ||
| } | ||
| f.mu.Lock() | ||
| } | ||
| f.progress.IsRunning = true | ||
| f.progress.Total = 1 | ||
| f.progress.Current = 0 | ||
| f.mu.Unlock() | ||
|
|
||
| title := parsedFeed.Title | ||
| if customTitle != "" { | ||
| title = customTitle | ||
| } | ||
| // Setup translator based on settings | ||
| f.setupTranslator() | ||
|
|
||
| feed := &models.Feed{ | ||
| Title: title, | ||
| URL: url, | ||
| Link: parsedFeed.Link, | ||
| Description: parsedFeed.Description, | ||
| Category: category, | ||
| } | ||
| // Fetch the feed | ||
| f.FetchFeed(ctx, feed) | ||
|
|
||
| if parsedFeed.Image != nil { | ||
| feed.ImageURL = parsedFeed.Image.URL | ||
| } | ||
| f.mu.Lock() | ||
| f.progress.Current = 1 | ||
| f.progress.IsRunning = false | ||
| f.mu.Unlock() | ||
|
|
||
| return f.db.AddFeed(feed) | ||
| // Update last article update time | ||
| f.db.SetSetting("last_article_update", time.Now().Format(time.RFC3339)) | ||
| log.Printf("Single feed update complete: %s", feed.Title) | ||
| } |
There was a problem hiding this comment.
Missing test coverage for the new core functionality. The new functions FetchSingleFeed and FetchFeedsByIDs have no test coverage, yet they are critical to the bug fix.
Consider adding tests that verify:
- Progress tracking works correctly (IsRunning flag, Current/Total counters)
- Concurrent calls are handled properly (second call waits or is rejected)
- Context cancellation is respected
- Timeout behavior when waiting for previous operations
Example test structure:
func TestFetchSingleFeed(t *testing.T) {
// Setup db and fetcher with mock parser
// Call FetchSingleFeed
// Verify progress was set correctly
// Verify feed was fetched
}
func TestFetchSingleFeedConcurrency(t *testing.T) {
// Start FetchSingleFeed in goroutine
// Try to start another immediately
// Verify second one waits or rejects properly
}| wg.Add(1) | ||
| sem <- struct{}{} | ||
| go func(fd models.Feed) { | ||
| defer wg.Done() | ||
| defer func() { <-sem }() | ||
|
|
||
| // ParseFeed parses an RSS feed from a URL and returns the parsed feed | ||
| func (f *Fetcher) ParseFeed(ctx context.Context, url string) (*gofeed.Feed, error) { | ||
| return f.fp.ParseURLWithContext(url, ctx) | ||
| } | ||
| select { | ||
| case <-ctx.Done(): | ||
| return | ||
| default: | ||
| } | ||
|
|
||
| // generateTitleFromContent generates a title from content when title is missing | ||
| func generateTitleFromContent(content string) string { | ||
| if content == "" { | ||
| return "Untitled Article" | ||
| f.FetchFeed(ctx, fd) | ||
| f.mu.Lock() | ||
| f.progress.Current++ | ||
| f.mu.Unlock() | ||
| }(*feed) |
There was a problem hiding this comment.
Progress counter might be incremented incorrectly when a feed fetch fails. If GetFeedByID returns an error, the progress is incremented at line 275, but if the goroutine encounters a context cancellation at line 287, progress is NOT incremented before returning. This creates an inconsistency where progress.Current may not reach progress.Total, causing the progress indicator to hang.
Consider always incrementing progress or using defer to ensure it's incremented even on early returns:
wg.Add(1)
sem <- struct{}{}
go func(fd models.Feed) {
defer wg.Done()
defer func() {
<-sem
f.mu.Lock()
f.progress.Current++
f.mu.Unlock()
}()
select {
case <-ctx.Done():
return
default:
}
f.FetchFeed(ctx, fd)
}(*feed)This ensures consistent progress tracking regardless of how the goroutine exits.
| if len(item.Enclosures) > 0 && item.Enclosures[0].Type == "image/jpeg" { | ||
| return item.Enclosures[0].URL |
There was a problem hiding this comment.
The image extraction only checks for image/jpeg enclosures, but RSS feeds may include images with other MIME types like image/png, image/gif, image/webp, etc. This could result in missing valid images.
Consider checking if the type starts with "image/":
// Try enclosures
if len(item.Enclosures) > 0 && strings.HasPrefix(item.Enclosures[0].Type, "image/") {
return item.Enclosures[0].URL
}| if len(item.Enclosures) > 0 && item.Enclosures[0].Type == "image/jpeg" { | |
| return item.Enclosures[0].URL | |
| for _, enclosure := range item.Enclosures { | |
| if strings.HasPrefix(enclosure.Type, "image/") { | |
| return enclosure.URL | |
| } |
| content = item.Description | ||
| } | ||
|
|
||
| re := regexp.MustCompile(`<img[^>]+src="([^">]+)"`) |
There was a problem hiding this comment.
The regex pattern <img[^>]+src="([^">]+)" only matches images with double-quoted src attributes. HTML allows single quotes or no quotes, so valid images might be missed.
Consider using a more flexible pattern or parsing the HTML properly:
// Match double quotes, single quotes, or unquoted
re := regexp.MustCompile(`<img[^>]+src=["']?([^"'\s>]+)["']?`)However, a more robust approach would be to use an HTML parser like golang.org/x/net/html instead of regex for parsing HTML attributes.
| re := regexp.MustCompile(`<img[^>]+src="([^">]+)"`) | |
| re := regexp.MustCompile(`<img[^>]+src=["']?([^"'\s>]+)["']?`) |
| feed, err := h.DB.GetFeedByID(feedID) | ||
| if err != nil { | ||
| return | ||
| } | ||
| h.Fetcher.FetchSingleFeed(context.Background(), *feed) |
There was a problem hiding this comment.
Using context.Background() in a goroutine started from an HTTP handler means the fetch operation cannot be cancelled if the client disconnects or the server shuts down. This could lead to wasted resources continuing to process after the user has navigated away.
Consider passing the request context or a cancellable context:
// Immediately fetch articles for the newly added feed in background
go func() {
// Create a context that can be cancelled but doesn't depend on the request lifetime
ctx, cancel := context.WithTimeout(context.Background(), 10*time.Minute)
defer cancel()
feed, err := h.DB.GetFeedByID(feedID)
if err != nil {
log.Printf("Error getting feed %d for immediate fetch: %v", feedID, err)
return
}
h.Fetcher.FetchSingleFeed(ctx, *feed)
}()The same issue exists in HandleRefreshFeed (line 115) and HandleOPMLImport (line 64).
| feed, err := h.DB.GetFeedByID(feedID) | |
| if err != nil { | |
| return | |
| } | |
| h.Fetcher.FetchSingleFeed(context.Background(), *feed) | |
| ctx, cancel := context.WithTimeout(context.Background(), 10*time.Minute) | |
| defer cancel() | |
| feed, err := h.DB.GetFeedByID(feedID) | |
| if err != nil { | |
| // Log the error for observability | |
| // (Assume log package is available; if not, replace with appropriate logging) | |
| // log.Printf("Error getting feed %d for immediate fetch: %v", feedID, err) | |
| return | |
| } | |
| h.Fetcher.FetchSingleFeed(ctx, *feed) |
| go func() { | ||
| feed, err := h.DB.GetFeedByID(feedID) | ||
| if err != nil { | ||
| return | ||
| } |
There was a problem hiding this comment.
The error from GetFeedByID is silently ignored. Consider logging the error before returning to help with debugging if the fetch fails to start.
Suggestion:
feed, err := h.DB.GetFeedByID(feedID)
if err != nil {
log.Printf("Error getting feed %d for immediate fetch: %v", feedID, err)
return
}| // setProgress sets the progress state (thread-safe) | ||
| func (f *Fetcher) setProgress(total, current int, isRunning bool) { | ||
| f.mu.Lock() | ||
| defer f.mu.Unlock() | ||
| f.progress.Total = total | ||
| f.progress.Current = current | ||
| f.progress.IsRunning = isRunning | ||
| } | ||
|
|
||
| // incrementProgress increments the current progress counter (thread-safe) | ||
| func (f *Fetcher) incrementProgress() { | ||
| f.mu.Lock() | ||
| defer f.mu.Unlock() | ||
| f.progress.Current++ | ||
| } | ||
|
|
||
| // startProgress initializes progress tracking for a new operation | ||
| func (f *Fetcher) startProgress(total int) bool { | ||
| f.mu.Lock() | ||
| defer f.mu.Unlock() | ||
|
|
||
| if f.progress.IsRunning { | ||
| return false | ||
| } | ||
|
|
||
| f.progress.IsRunning = true | ||
| f.progress.Total = total | ||
| f.progress.Current = 0 | ||
| return true | ||
| } | ||
|
|
||
| // finishProgress marks the operation as complete | ||
| func (f *Fetcher) finishProgress() { | ||
| f.mu.Lock() | ||
| defer f.mu.Unlock() | ||
| f.progress.IsRunning = false | ||
| } |
There was a problem hiding this comment.
The startProgress, finishProgress, setProgress, and incrementProgress helper functions are defined but never used in the codebase. The progress tracking code directly manipulates the mutex and progress fields instead of using these helpers.
Consider either:
- Using these helper functions in
FetchSingleFeedandFetchFeedsByIDsto improve code consistency, or - Removing these unused functions to reduce code clutter
For example, FetchSingleFeed could use:
if !f.startProgress(1) {
// Wait for current operation...
}
// ... fetch work ...
f.finishProgress()| // setProgress sets the progress state (thread-safe) | |
| func (f *Fetcher) setProgress(total, current int, isRunning bool) { | |
| f.mu.Lock() | |
| defer f.mu.Unlock() | |
| f.progress.Total = total | |
| f.progress.Current = current | |
| f.progress.IsRunning = isRunning | |
| } | |
| // incrementProgress increments the current progress counter (thread-safe) | |
| func (f *Fetcher) incrementProgress() { | |
| f.mu.Lock() | |
| defer f.mu.Unlock() | |
| f.progress.Current++ | |
| } | |
| // startProgress initializes progress tracking for a new operation | |
| func (f *Fetcher) startProgress(total int) bool { | |
| f.mu.Lock() | |
| defer f.mu.Unlock() | |
| if f.progress.IsRunning { | |
| return false | |
| } | |
| f.progress.IsRunning = true | |
| f.progress.Total = total | |
| f.progress.Current = 0 | |
| return true | |
| } | |
| // finishProgress marks the operation as complete | |
| func (f *Fetcher) finishProgress() { | |
| f.mu.Lock() | |
| defer f.mu.Unlock() | |
| f.progress.IsRunning = false | |
| } |
Description
Fixes feed initialization issues where newly added feeds or OPML imports did not immediately fetch articles, and progress was not tracked correctly for single-feed operations.
Type of Change
Related Issues
Fixes #108
Changes Made
Backend
db.AddFeednow returns feed ID to enable immediate article fetch for new feedsFetchSingleFeed()andFetchFeedsByIDs()with proper progress trackingsetupTranslator()andwaitForProgressComplete()helpers (5-min timeout to prevent infinite loops)HandleAddFeedtriggers background article fetch for newly added feedHandleRefreshFeedusesFetchSingleFeedinstead of rawFetchFeedHandleOPMLImportcollects feed IDs and usesFetchFeedsByIDsinstead ofFetchAllFrontend
onFeedAddedstarts progress polling after feed is addedhandleImportOPMLstarts progress polling after OPML importhandleFeedAction('refreshFeed')starts progress polling instead of fixed 2-second timeoutScreenshots
N/A - Backend behavior change, UI unchanged
Testing
Test Configuration
Test Steps
Checklist
Additional Notes
The existing
pollProgress()mechanism handles article list refresh when progress completes, so frontend changes simply trigger polling at the right times.Breaking Changes
None - API responses unchanged, only internal behavior improved.
Warning
Firewall rules blocked me from connecting to one or more addresses (expand for details)
I tried to connect to the following addresses, but was blocked by firewall rules:
nonexistent-feed-url-12345.com/tmp/go-build3933838176/b248/discovery.test /tmp/go-build3933838176/b248/discovery.test -test.testlogfile=/tmp/go-build3933838176/b248/testlog.txt -test.paniconexit0 -test.timeout=10m0s(dns block)/tmp/go-build3299490878/b248/discovery.test /tmp/go-build3299490878/b248/discovery.test -test.testlogfile=/tmp/go-build3299490878/b248/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true(dns block)If you need me to access, download, or install something from one of these locations, you can either:
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.