Merged
Conversation
Allow users to import blog subscriptions from OPML files exported by feed readers like Feedly, Inoreader, and NewsBlur. Duplicates are gracefully skipped and reported. Based on upstream PR Hyaxia/blogwatcher#12 by @pastukhov, adapted to our project patterns (context threading, testify, squirrel, etc.). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 3 minutes and 26 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds OPML import: new CLI Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant CLI as CLI
participant FS as File System
participant OPML as OPML Parser
participant Ctrl as Controller
participant DB as Database
User->>CLI: run `import <opml-file>`
CLI->>FS: open OPML file
FS-->>CLI: file handle
CLI->>DB: open database (viper db path)
DB-->>CLI: db connection
CLI->>Ctrl: ImportOPML(ctx, db, file)
Ctrl->>OPML: Parse(file)
OPML->>OPML: decode XML, collect []Feed
OPML-->>Ctrl: []Feed
loop for each Feed
Ctrl->>DB: AddBlog(title, siteURL, feedURL)
alt exists
DB-->>Ctrl: BlogAlreadyExistsError
Ctrl->>Ctrl: skipped++
else new
DB-->>Ctrl: success
Ctrl->>Ctrl: added++
end
end
Ctrl-->>CLI: (added, skipped, err)
CLI->>User: print summary (added/skipped) or error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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 |
![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)
internal/opml/opml_test.go (1)
95-111: Charset test doesn’t currently prove non-UTF-8 decoding.This case declares
ISO-8859-1but uses ASCII-only content, so transcoding isn’t actually exercised. Consider adding at least one non-ASCII byte sequence (e.g.,Caf\xe9) and asserting it parses asCafé.Suggested test adjustment
import ( + "bytes" "strings" "testing" @@ -func TestParseSubscriptionList(t *testing.T) { - input := `<?xml version="1.0" encoding="ISO-8859-1"?> +func TestParseSubscriptionList(t *testing.T) { + input := []byte(`<?xml version="1.0" encoding="ISO-8859-1"?> <opml version="2.0"> <head><title>mySubscriptions.opml</title></head> <body> - <outline text="CNET News.com" description="Tech news" htmlUrl="http://news.com.com/" language="unknown" title="CNET News.com" type="rss" version="RSS2" xmlUrl="http://news.com.com/2547-1_3-0-5.xml"/> + <outline text="Caf\xe9 News" description="Tech news" htmlUrl="http://news.com.com/" language="unknown" title="Caf\xe9 News" type="rss" version="RSS2" xmlUrl="http://news.com.com/2547-1_3-0-5.xml"/> <outline text="washingtonpost.com - Politics" description="Politics" htmlUrl="http://www.washingtonpost.com/wp-dyn/politics" language="unknown" title="washingtonpost.com - Politics" type="rss" version="RSS2" xmlUrl="http://www.washingtonpost.com/wp-srv/politics/rssheadlines.xml"/> <outline text="Scripting News" description="It's even worse than it appears." htmlUrl="http://www.scripting.com/" language="unknown" title="Scripting News" type="rss" version="RSS2" xmlUrl="http://www.scripting.com/rss.xml"/> </body> -</opml>` +</opml>`) - feeds, err := Parse(strings.NewReader(input)) + feeds, err := Parse(bytes.NewReader(input)) require.NoError(t, err) require.Len(t, feeds, 3) - assert.Equal(t, "CNET News.com", feeds[0].Title) + assert.Equal(t, "Café News", feeds[0].Title)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/opml/opml_test.go` around lines 95 - 111, The test declares ISO-8859-1 but uses only ASCII so decoding isn't exercised; update the test input string (used by Parse in internal/opml/opml_test.go) to include at least one non-ASCII byte sequence encoded for ISO-8859-1 (e.g., "Caf\xe9" in the XML) and then add an assertion that the parsed feed Title (e.g., feeds[0].Title) equals the correctly transcoded UTF-8 string "Café" to verify the decoder in Parse handles non-UTF-8 charsets.internal/controller/controller_test.go (1)
122-154: Add a regression test for feeds withouttitle/text.Given duplicate detection uses blog name, add a case where an OPML feed has
xmlUrlbut notitle/text, then assert import still adds it (using fallback name) instead of treating multiple entries as duplicates.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/controller_test.go` around lines 122 - 154, Add a new test that exercises ImportOPML when outlines have xmlUrl but no title/text: create OPML with two <outline> entries that only set xmlUrl="http://nosite.com/feed", call ImportOPML(ctx, db, ...), and assert that added == 1 and skipped == 1 (duplicate detection should use the fallback name), then fetch the created blog via db.GetBlogByName(ctx, "http://nosite.com/feed") and assert the blog is present and blog.URL == "http://nosite.com/feed". This uses the existing ImportOPML and db.GetBlogByName symbols to verify the fallback-name behavior and dedup logic.e2e/e2e_test.go (1)
443-443: Use an OS-agnostic missing-path in the failure case.Line 443 hardcodes a Unix absolute path. Using a guaranteed-missing path under
t.TempDir()makes this check more portable and less environment-dependent.Proposed tweak
- // Import a nonexistent file should fail. - c.fail(t, []string{"import", "/nonexistent/file.opml"}, nil) + // Import a nonexistent file should fail. + missingPath := filepath.Join(t.TempDir(), "does-not-exist.opml") + c.fail(t, []string{"import", missingPath}, nil)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/e2e_test.go` at line 443, Replace the hardcoded Unix path "/nonexistent/file.opml" with an OS-agnostic missing file path built from t.TempDir(); for example, create missingPath := filepath.Join(t.TempDir(), "nonexistent.opml") (importing path/filepath if not already) and call c.fail(t, []string{"import", missingPath}, nil) so the test uses a guaranteed-missing, platform-independent path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/controller/controller.go`:
- Around line 152-157: When calling AddBlog, guard against empty OPML feed
titles by ensuring the title passed is not blank: check feed.Title (trim
whitespace) and if empty, fall back to a deterministic non-empty value such as
siteURL (derived from feed.SiteURL or feed.FeedURL) or feed.FeedURL, then call
AddBlog with that fallback title; update the code path around siteURL,
feed.Title and the AddBlog(...) invocation so de-duplication by name won't
collapse distinct feeds with empty titles.
---
Nitpick comments:
In `@e2e/e2e_test.go`:
- Line 443: Replace the hardcoded Unix path "/nonexistent/file.opml" with an
OS-agnostic missing file path built from t.TempDir(); for example, create
missingPath := filepath.Join(t.TempDir(), "nonexistent.opml") (importing
path/filepath if not already) and call c.fail(t, []string{"import",
missingPath}, nil) so the test uses a guaranteed-missing, platform-independent
path.
In `@internal/controller/controller_test.go`:
- Around line 122-154: Add a new test that exercises ImportOPML when outlines
have xmlUrl but no title/text: create OPML with two <outline> entries that only
set xmlUrl="http://nosite.com/feed", call ImportOPML(ctx, db, ...), and assert
that added == 1 and skipped == 1 (duplicate detection should use the fallback
name), then fetch the created blog via db.GetBlogByName(ctx,
"http://nosite.com/feed") and assert the blog is present and blog.URL ==
"http://nosite.com/feed". This uses the existing ImportOPML and db.GetBlogByName
symbols to verify the fallback-name behavior and dedup logic.
In `@internal/opml/opml_test.go`:
- Around line 95-111: The test declares ISO-8859-1 but uses only ASCII so
decoding isn't exercised; update the test input string (used by Parse in
internal/opml/opml_test.go) to include at least one non-ASCII byte sequence
encoded for ISO-8859-1 (e.g., "Caf\xe9" in the XML) and then add an assertion
that the parsed feed Title (e.g., feeds[0].Title) equals the correctly
transcoded UTF-8 string "Café" to verify the decoder in Parse handles non-UTF-8
charsets.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0d1734c7-0e51-46b2-bf99-0114b3de62b9
📒 Files selected for processing (11)
e2e/e2e_test.goe2e/expected/30_import_opml.txte2e/expected/31_import_blogs_listed.txte2e/expected/32_import_opml_duplicates.txtgo.modinternal/cli/commands.gointernal/cli/root.gointernal/controller/controller.gointernal/controller/controller_test.gointernal/opml/opml.gointernal/opml/opml_test.go
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Prevents multiple untitled feeds from colliding on empty name. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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.
Summary
blogwatcher-cli import <file>command to bulk-import blog subscriptions from OPML files (Feedly, Inoreader, NewsBlur, etc.)internal/opmlpackage: parses OPML 1.0/2.0 with support for non-UTF-8 encodings (ISO-8859-1), nested categories, and mixed outline typesImported 2 blog(s), skipped 1 duplicate(s)Credits
Based on Hyaxia/blogwatcher#12 by @pastukhov, adapted to our project patterns:
context.Contextthreading through controller and CLIassert/requireinstead of rawt.Fatal/t.ErrorfClose()callsTest plan
golangci-lint run-- 0 issuesgotestsum -- ./... -count=1-- 38 tests pass (35 existing + 3 new)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests