feat: optimistic caching for topic sub-feeds with per-URI health tracking - #805
Merged
Merged
Conversation
…tracking When a sub-feed in a TopicFeed fails to fetch, the system now automatically falls back to the last cached result (stored up to 14 days in Redis) so the topic feed remains available despite partial upstream failures. Changes: - internal/constant/ttl.go: add SubFeedCacheTTL (14d) and SubFeedHealthTTL (30d) - internal/constant/cache_namespace.go: add PrefixSubFeedResult and PrefixSubFeedHealth - internal/feedruntime/cached_provider.go (new): CachedFeedProvider wraps any FeedProvider; on failure returns cached snapshot; health metadata (last success, last failure, last error, cached_at) persisted in Redis - internal/feedruntime/builder.go: wrap each topic input with CachedFeedProvider - internal/util/cache.go: add TryCacheSetString / TryCacheGetString that return errors instead of calling log.Fatalf when Redis is unavailable (test-safe) - internal/controller/topic_feed.go: add sub_feed_health to TopicDetailResponse - web/admin/src/api/topic.ts: add SubFeedHealth type; update TopicDetail - web/admin/src/views/dashboard/topic_feed/detail.vue: add sub-feed health card showing per-URI last-success, last-failure, error message, and cache status - web/admin/src/locale/zh-CN/topic.ts, en-US/topic.ts: add i18n keys - internal/feedruntime/builder_test.go: update nested-topic test for CachedFeedProvider Co-authored-by: Colin <Colin_XKL@outlook.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
Reviewer's GuideImplements an optimistic caching layer around topic sub-feed providers that stores per-URI health metadata in Redis, exposes this health in the topic detail API, and surfaces it in the admin UI as a new Sub-Feed Cache Health table, along with non-fatal Redis helpers and minimal test updates. Sequence diagram for optimistic CachedFeedProvider.Fetch behaviorsequenceDiagram
participant Client
participant CachedFeedProvider
participant InnerFeedProvider
participant Redis
Client->>CachedFeedProvider: Fetch(ctx)
CachedFeedProvider->>InnerFeedProvider: Fetch(ctx)
alt [live fetch succeeds]
InnerFeedProvider-->>CachedFeedProvider: feed, nil
CachedFeedProvider->>Redis: TryCacheSetString(subFeedResultKey, feedJSON, SubFeedCacheTTL)
CachedFeedProvider->>Redis: TryCacheSetString(subFeedHealthKey, successHealthJSON, SubFeedHealthTTL)
CachedFeedProvider-->>Client: feed, nil
else [live fetch fails]
InnerFeedProvider-->>CachedFeedProvider: nil, err
CachedFeedProvider->>Redis: TryCacheGetString(subFeedResultKey)
alt [cached snapshot exists]
Redis-->>CachedFeedProvider: feedJSON
CachedFeedProvider->>Redis: TryCacheSetString(subFeedHealthKey, failureHealthJSON, SubFeedHealthTTL)
CachedFeedProvider-->>Client: cachedFeed, nil
else [no cached snapshot]
Redis-->>CachedFeedProvider: "", error
CachedFeedProvider->>Redis: TryCacheSetString(subFeedHealthKey, failureHealthJSON, SubFeedHealthTTL)
CachedFeedProvider-->>Client: nil, err
end
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces optimistic caching and health tracking for sub-feeds during topic aggregation, allowing failing sub-feeds to fall back to their last known good snapshot. It also adds a new 'Sub-Feed Cache Health' section to the admin dashboard. The review feedback highlights a critical connection leak in the newly added tryGetRedisClient function, recommending that the Redis client be cached thread-safely using a mutex. Additionally, the reviewer noted a UI logic issue where sub-feeds that have failed without any cached fallback are misleadingly labeled as 'Using Cache' instead of 'Failed', and provided suggestions to update the status labeling and translation keys.
Comment on lines
+6
to
+8
| "errors" | ||
| "strings" | ||
|
|
Contributor
There was a problem hiding this comment.
Import the sync package to allow thread-safe caching of the Redis client in tryGetRedisClient.
Suggested change
| "errors" | |
| "strings" | |
| "errors" | |
| "strings" | |
| "sync" | |
Comment on lines
+35
to
+55
| // tryGetRedisClient returns a Redis client or an error without fatally crashing. | ||
| // It is safe to call in environments where Redis may not be configured (e.g. tests). | ||
| func tryGetRedisClient() (*redis.Client, error) { | ||
| envClient := GetEnvClient() | ||
| if envClient == nil { | ||
| return nil, errors.New("env client not available") | ||
| } | ||
| redisURI := strings.TrimSpace(envClient.GetString("REDIS_URI")) | ||
| if redisURI == "" { | ||
| return nil, errors.New("REDIS_URI is not configured") | ||
| } | ||
| opts, err := redis.ParseURL(redisURI) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| rdb := redis.NewClient(opts) | ||
| if rdb == nil { | ||
| return nil, errors.New("failed to create redis client") | ||
| } | ||
| return rdb, nil | ||
| } |
Contributor
There was a problem hiding this comment.
Creating a new Redis client and connection pool on every call to tryGetRedisClient will cause a severe connection leak, especially since TryCacheGetString is called in a loop for each sub-feed URI in GetTopicFeedDetail. Cache the client instance using a mutex to ensure thread safety and prevent resource exhaustion.
var (
tryRdb *redis.Client
tryRdbMu sync.Mutex
tryRdbErr error
tryRdbInit bool
)
// tryGetRedisClient returns a Redis client or an error without fatally crashing.
// It is safe to call in environments where Redis may not be configured (e.g. tests).
func tryGetRedisClient() (*redis.Client, error) {
tryRdbMu.Lock()
defer tryRdbMu.Unlock()
if tryRdbInit {
return tryRdb, tryRdbErr
}
envClient := GetEnvClient()
if envClient == nil {
return nil, errors.New("env client not available")
}
redisURI := strings.TrimSpace(envClient.GetString("REDIS_URI"))
if redisURI == "" {
return nil, errors.New("REDIS_URI is not configured")
}
opts, err := redis.ParseURL(redisURI)
if err != nil {
tryRdbErr = err
tryRdbInit = true
return nil, err
}
tryRdb = redis.NewClient(opts)
if tryRdb == nil {
tryRdbErr = errors.New("failed to create redis client")
}
tryRdbInit = true
return tryRdb, tryRdbErr
}
Comment on lines
+110
to
+113
| 'topic.detail.subFeedHealth.status': 'Status', | ||
| 'topic.detail.subFeedHealth.status.ok': 'OK', | ||
| 'topic.detail.subFeedHealth.status.stale': 'Using Cache', | ||
| 'topic.detail.subFeedHealth.status.unknown': 'Unknown', |
Contributor
There was a problem hiding this comment.
Add a translation key for the 'failed' status of a sub-feed when it has never succeeded and has no cached snapshot to fall back to.
'topic.detail.subFeedHealth.status': 'Status',
'topic.detail.subFeedHealth.status.ok': 'OK',
'topic.detail.subFeedHealth.status.stale': 'Using Cache',
'topic.detail.subFeedHealth.status.failed': 'Failed',
'topic.detail.subFeedHealth.status.unknown': 'Unknown',
Comment on lines
+107
to
+110
| 'topic.detail.subFeedHealth.status': '状态', | ||
| 'topic.detail.subFeedHealth.status.ok': '正常', | ||
| 'topic.detail.subFeedHealth.status.stale': '使用缓存', | ||
| 'topic.detail.subFeedHealth.status.unknown': '未知', |
Contributor
There was a problem hiding this comment.
Add a translation key for the 'failed' status of a sub-feed in Chinese.
Suggested change
| 'topic.detail.subFeedHealth.status': '状态', | |
| 'topic.detail.subFeedHealth.status.ok': '正常', | |
| 'topic.detail.subFeedHealth.status.stale': '使用缓存', | |
| 'topic.detail.subFeedHealth.status.unknown': '未知', | |
| 'topic.detail.subFeedHealth.status': '状态', | |
| 'topic.detail.subFeedHealth.status.ok': '正常', | |
| 'topic.detail.subFeedHealth.status.stale': '使用缓存', | |
| 'topic.detail.subFeedHealth.status.failed': '失败', | |
| 'topic.detail.subFeedHealth.status.unknown': '未知', |
Comment on lines
+384
to
+396
| const subFeedStatusLabel = (record: SubFeedHealth) => { | ||
| if (!record.last_success_at && !record.last_failure_at) | ||
| return t('topic.detail.subFeedHealth.status.unknown'); | ||
| if (record.last_failure_at && !record.last_success_at) | ||
| return t('topic.detail.subFeedHealth.status.stale'); | ||
| if (record.last_failure_at && record.last_success_at) { | ||
| const failedAt = new Date(record.last_failure_at).getTime(); | ||
| const succeededAt = new Date(record.last_success_at).getTime(); | ||
| if (failedAt > succeededAt) | ||
| return t('topic.detail.subFeedHealth.status.stale'); | ||
| } | ||
| return t('topic.detail.subFeedHealth.status.ok'); | ||
| }; |
Contributor
There was a problem hiding this comment.
When a sub-feed has failed but has never succeeded (record.last_failure_at && !record.last_success_at), it cannot fall back to any cached snapshot. Showing 'Using Cache' (stale) is misleading. Update the status label to return the 'failed' status instead.
const subFeedStatusLabel = (record: SubFeedHealth) => {
if (!record.last_success_at && !record.last_failure_at)
return t('topic.detail.subFeedHealth.status.unknown');
if (record.last_failure_at && !record.last_success_at)
return t('topic.detail.subFeedHealth.status.failed');
if (record.last_failure_at && record.last_success_at) {
const failedAt = new Date(record.last_failure_at).getTime();
const succeededAt = new Date(record.last_success_at).getTime();
if (failedAt > succeededAt)
return t('topic.detail.subFeedHealth.status.stale');
}
return t('topic.detail.subFeedHealth.status.ok');
};
Up to standards ✅🟢 Issues
|
Colin-XKL
marked this pull request as ready for review
June 1, 2026 07:21
Contributor
Review Summary by QodoAdd optimistic caching for topic sub-feeds with per-URI health tracking
WalkthroughsDescription• Implement optimistic caching for topic sub-feeds with automatic fallback to cached results when live fetch fails • Add per-URI health tracking stored in Redis with success/failure timestamps and error messages • Display sub-feed cache health status in admin topic detail page with color-coded status indicators • Introduce test-safe Redis cache helpers that gracefully handle missing Redis configuration Diagramflowchart LR
A["Topic Feed Request"] --> B["BuildTopic"]
B --> C["Wrap Each Input<br/>with CachedFeedProvider"]
C --> D["Live Fetch"]
D -->|Success| E["Cache Result<br/>+ Record Success"]
D -->|Failure| F["Load Cached<br/>Snapshot"]
F -->|Cache Hit| G["Return Cached<br/>Feed"]
F -->|Cache Miss| H["Propagate Error"]
E --> I["Store in Redis<br/>14 days TTL"]
G --> J["Topic Aggregation<br/>Continues"]
E --> K["Update Health<br/>Metadata"]
F --> K
K --> L["Store in Redis<br/>30 days TTL"]
L --> M["Admin UI Shows<br/>Sub-Feed Health"]
File Changes1. internal/constant/cache_namespace.go
|
Contributor
Code Review by Qodo
1. Cached fetch failure not propagated
|
![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:
- In the Vue
subFeedStatusColor/subFeedStatusLabelhelpers, the caselast_failure_at && !last_success_atreturns a red tag color but the label 'Using Cache', which is inconsistent with the backing data (no successful cache write yet); consider aligning this branch to represent a pure failure state (e.g. red + 'Error') or only use 'Using Cache' when a successful cache exists. - The new
tryGetRedisClientcreates a fresh Redis client (and reparsesREDIS_URI) on every cache call, which is wasteful and may leak connections; consider reusing the existing client fromGetRedisClientor memoizing a singleton*redis.Clientfor the TryCache helpers.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the Vue `subFeedStatusColor`/`subFeedStatusLabel` helpers, the case `last_failure_at && !last_success_at` returns a red tag color but the label 'Using Cache', which is inconsistent with the backing data (no successful cache write yet); consider aligning this branch to represent a pure failure state (e.g. red + 'Error') or only use 'Using Cache' when a successful cache exists.
- The new `tryGetRedisClient` creates a fresh Redis client (and reparses `REDIS_URI`) on every cache call, which is wasteful and may leak connections; consider reusing the existing client from `GetRedisClient` or memoizing a singleton `*redis.Client` for the TryCache helpers.
## Individual Comments
### Comment 1
<location path="web/admin/src/views/dashboard/topic_feed/detail.vue" line_range="373-375" />
<code_context>
+ // Returns a colour for the sub-feed status tag.
+ // "stale" (using cache) = orange; "ok" (last_failure is absent or last_success is more recent) = green; unknown = gray.
+ const subFeedStatusColor = (record: SubFeedHealth) => {
+ if (!record.last_success_at && !record.last_failure_at) return 'gray';
+ if (record.last_failure_at && !record.last_success_at) return 'red';
+ if (record.last_failure_at && record.last_success_at) {
+ const failedAt = new Date(record.last_failure_at).getTime();
</code_context>
<issue_to_address>
**issue:** Status color and label semantics are inconsistent for the `last_failure_at && !last_success_at` case.
When `last_failure_at` is set and `last_success_at` is not, `subFeedStatusColor` returns `'red'` but `subFeedStatusLabel` returns `'Using Cache'` (stale), and the function comment only mentions orange/green/gray. This breaks the color/label mapping and can confuse users. Please either use the same "stale" color (orange) for this branch as the `failedAt > succeededAt` case, or introduce a distinct label for this hard-error case so color and label semantics stay consistent.
</issue_to_address>
### Comment 2
<location path="internal/util/cache.go" line_range="37-46" />
<code_context>
+// tryGetRedisClient returns a Redis client or an error without fatally crashing.
+// It is safe to call in environments where Redis may not be configured (e.g. tests).
+func tryGetRedisClient() (*redis.Client, error) {
+ envClient := GetEnvClient()
+ if envClient == nil {
+ return nil, errors.New("env client not available")
+ }
+ redisURI := strings.TrimSpace(envClient.GetString("REDIS_URI"))
+ if redisURI == "" {
+ return nil, errors.New("REDIS_URI is not configured")
+ }
+ opts, err := redis.ParseURL(redisURI)
+ if err != nil {
+ return nil, err
+ }
+ rdb := redis.NewClient(opts)
+ if rdb == nil {
+ return nil, errors.New("failed to create redis client")
</code_context>
<issue_to_address>
**suggestion (performance):** A new Redis client is created for every TryCache call, which can add unnecessary overhead.
Both `TryCacheSetString` and `TryCacheGetString` call `tryGetRedisClient`, which parses `REDIS_URI` and constructs a new `redis.Client` each time. Since the client already manages its own pool, this repeated construction is unnecessary overhead and may complicate connection handling in long-lived services. Consider reusing a shared non-fatal client instance (similar to `GetRedisClient`, but returning errors instead of calling `log.Fatal`) while still failing gracefully when `REDIS_URI` is missing or invalid.
Suggested implementation:
```golang
"github.com/redis/go-redis/v9"
"github.com/sirupsen/logrus"
"log"
"sync"
```
```golang
return rdb
}
var (
tryRedisClient *redis.Client
tryRedisClientErr error
tryRedisClientOnce sync.Once
)
// tryGetRedisClient returns a Redis client or an error without fatally crashing.
// It is safe to call in environments where Redis may not be configured (e.g. tests).
// The client is constructed once and reused for subsequent calls.
func tryGetRedisClient() (*redis.Client, error) {
tryRedisClientOnce.Do(func() {
envClient := GetEnvClient()
if envClient == nil {
tryRedisClientErr = errors.New("env client not available")
return
}
redisURI := strings.TrimSpace(envClient.GetString("REDIS_URI"))
if redisURI == "" {
tryRedisClientErr = errors.New("REDIS_URI is not configured")
return
}
opts, err := redis.ParseURL(redisURI)
if err != nil {
tryRedisClientErr = err
return
}
rdb := redis.NewClient(opts)
if rdb == nil {
tryRedisClientErr = errors.New("failed to create redis client")
return
}
tryRedisClient = rdb
})
if tryRedisClientErr != nil {
return nil, tryRedisClientErr
}
return tryRedisClient, nil
}
```
</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
+373
to
+375
| const subFeedStatusColor = (record: SubFeedHealth) => { | ||
| if (!record.last_success_at && !record.last_failure_at) return 'gray'; | ||
| if (record.last_failure_at && !record.last_success_at) return 'red'; |
Contributor
There was a problem hiding this comment.
issue: Status color and label semantics are inconsistent for the last_failure_at && !last_success_at case.
When last_failure_at is set and last_success_at is not, subFeedStatusColor returns 'red' but subFeedStatusLabel returns 'Using Cache' (stale), and the function comment only mentions orange/green/gray. This breaks the color/label mapping and can confuse users. Please either use the same "stale" color (orange) for this branch as the failedAt > succeededAt case, or introduce a distinct label for this hard-error case so color and label semantics stay consistent.
Comment on lines
+37
to
+46
| func tryGetRedisClient() (*redis.Client, error) { | ||
| envClient := GetEnvClient() | ||
| if envClient == nil { | ||
| return nil, errors.New("env client not available") | ||
| } | ||
| redisURI := strings.TrimSpace(envClient.GetString("REDIS_URI")) | ||
| if redisURI == "" { | ||
| return nil, errors.New("REDIS_URI is not configured") | ||
| } | ||
| opts, err := redis.ParseURL(redisURI) |
Contributor
There was a problem hiding this comment.
suggestion (performance): A new Redis client is created for every TryCache call, which can add unnecessary overhead.
Both TryCacheSetString and TryCacheGetString call tryGetRedisClient, which parses REDIS_URI and constructs a new redis.Client each time. Since the client already manages its own pool, this repeated construction is unnecessary overhead and may complicate connection handling in long-lived services. Consider reusing a shared non-fatal client instance (similar to GetRedisClient, but returning errors instead of calling log.Fatal) while still failing gracefully when REDIS_URI is missing or invalid.
Suggested implementation:
"github.com/redis/go-redis/v9"
"github.com/sirupsen/logrus"
"log"
"sync" return rdb
}
var (
tryRedisClient *redis.Client
tryRedisClientErr error
tryRedisClientOnce sync.Once
)
// tryGetRedisClient returns a Redis client or an error without fatally crashing.
// It is safe to call in environments where Redis may not be configured (e.g. tests).
// The client is constructed once and reused for subsequent calls.
func tryGetRedisClient() (*redis.Client, error) {
tryRedisClientOnce.Do(func() {
envClient := GetEnvClient()
if envClient == nil {
tryRedisClientErr = errors.New("env client not available")
return
}
redisURI := strings.TrimSpace(envClient.GetString("REDIS_URI"))
if redisURI == "" {
tryRedisClientErr = errors.New("REDIS_URI is not configured")
return
}
opts, err := redis.ParseURL(redisURI)
if err != nil {
tryRedisClientErr = err
return
}
rdb := redis.NewClient(opts)
if rdb == nil {
tryRedisClientErr = errors.New("failed to create redis client")
return
}
tryRedisClient = rdb
})
if tryRedisClientErr != nil {
return nil, tryRedisClientErr
}
return tryRedisClient, nil
}
|
Warning Review limit reached
More reviews will be available in 23 minutes and 8 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
WalkthroughThis PR adds optimistic Redis caching for sub-feed topic aggregation with persistent health tracking. When a sub-feed fails to fetch, the system returns the most recent cached snapshot; on success, the fresh result is cached. Health metadata recording timestamps and errors is persisted and exposed through a new "Sub-Feed Health" dashboard card in the topic feed detail view. ChangesSub-feed Caching and Health Display
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
internal/feedruntime/cached_provider.go (1)
86-104: 💤 Low value
CachedAtreflects the in-memory write time, not a confirmed cache persist.
recordSuccesssetsCachedAt = &nowregardless of whethercacheResultactually persisted to Redis (the write failure is only logged at warn). The health view will then show acached_ateven when nothing was cached. Acceptable if intentional, but worth confirming the dashboard semantics — surfacingcached_atonly when the snapshot write succeeded would be more accurate.🤖 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 `@internal/feedruntime/cached_provider.go` around lines 86 - 104, recordSuccess currently sets h.CachedAt unconditionally causing the health view to show a cached time even when cacheResult failed; change the flow so CachedAt is only set when the snapshot persist actually succeeds: either have cacheResult return a success bool/error and set h.CachedAt after a successful cacheResult call, or add a parameter to recordSuccess (e.g., persisted bool or err) and only assign h.CachedAt = &now when persistence succeeded; keep LastSuccessAt updated regardless and leave recordFailure unchanged, and ensure calls to saveHealth and loadHealth still use the same functions (CachedFeedProvider.recordSuccess, cacheResult, loadHealth, saveHealth).internal/controller/topic_feed.go (1)
214-217: ⚖️ Poor tradeoffPer-URI sequential Redis reads on the detail request path.
This loop issues one
GetSubFeedHealth(a RedisGET) per input URI, sequentially, on the request thread — and each call also spins up a new Redis client (see the root-cause note ininternal/util/cache.go). For topics with many inputs this is N round trips per detail load. If this view is hit frequently, consider batching the health lookups into a single pipelined/MGETcall.🤖 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 `@internal/controller/topic_feed.go` around lines 214 - 217, The current loop in topic_feed.go calling feedruntime.GetSubFeedHealth for each uri causes N sequential Redis GETs (and per-call client creation); replace this with a batched lookup: add a new feedruntime function (e.g., GetSubFeedHealthBatch(inputURIs []string) []feedruntime.SubFeedHealth) that uses a single MGET or pipelined requests via the shared Redis client (see internal/util/cache.go) to fetch all entries in one round trip and returns the slice in the same order, then replace the per-URI loop (topicData.InputURIs -> subFeedHealth) with a single call to that new batch function.
🤖 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 `@internal/feedruntime/cached_provider.go`:
- Around line 60-69: The cacheResult function currently writes any successful
result (including nil or empty feeds) and thus can overwrite a good snapshot;
modify CachedFeedProvider.cacheResult to skip caching when the incoming feed is
nil or contains no articles (e.g., if feed == nil || len(feed.Articles) == 0)
before calling json.Marshal and TryCacheSetString (keep using
subFeedResultKey(p.URI) and util.TryCacheSetString); optionally emit a debug/log
message when skipping the cache write so the behavior is observable.
In `@internal/util/cache.go`:
- Around line 37-75: The code creates a new redis.Client on every call in
tryGetRedisClient (used by TryCacheSetString/TryCacheGetString and
CacheSetString/CacheGetString) and never closes them, causing many idle pools;
change to a package-level singleton redis client initialized once (use sync.Once
or similar) and return that from tryGetRedisClient/GetRedisClient instead of
calling redis.NewClient each time, ensure the existing Close call in
internal/controller/dependency_monitor.go (or add a single Close hook during app
shutdown) closes the singleton client, and remove per-call NewClient usage so
all cache ops reuse the same long-lived *redis.Client.
In `@web/admin/src/views/dashboard/topic_feed/detail.vue`:
- Around line 371-396: Both helpers subFeedStatusColor and subFeedStatusLabel
compute status logic separately and disagree for the case where last_failure_at
exists but last_success_at is absent; centralize by creating a single status
computation (e.g., computeSubFeedStatus(record: SubFeedHealth) returning an
explicit enum/value like 'unknown'|'failed'|'stale'|'ok') and then have
subFeedStatusColor and subFeedStatusLabel map that single status to color and to
i18n keys respectively; ensure the failure-only case maps to a new i18n key
(topic.detail.subFeedHealth.status.failed) and remove the duplicated timestamp
comparisons from both functions so they use the shared computeSubFeedStatus
function.
---
Nitpick comments:
In `@internal/controller/topic_feed.go`:
- Around line 214-217: The current loop in topic_feed.go calling
feedruntime.GetSubFeedHealth for each uri causes N sequential Redis GETs (and
per-call client creation); replace this with a batched lookup: add a new
feedruntime function (e.g., GetSubFeedHealthBatch(inputURIs []string)
[]feedruntime.SubFeedHealth) that uses a single MGET or pipelined requests via
the shared Redis client (see internal/util/cache.go) to fetch all entries in one
round trip and returns the slice in the same order, then replace the per-URI
loop (topicData.InputURIs -> subFeedHealth) with a single call to that new batch
function.
In `@internal/feedruntime/cached_provider.go`:
- Around line 86-104: recordSuccess currently sets h.CachedAt unconditionally
causing the health view to show a cached time even when cacheResult failed;
change the flow so CachedAt is only set when the snapshot persist actually
succeeds: either have cacheResult return a success bool/error and set h.CachedAt
after a successful cacheResult call, or add a parameter to recordSuccess (e.g.,
persisted bool or err) and only assign h.CachedAt = &now when persistence
succeeded; keep LastSuccessAt updated regardless and leave recordFailure
unchanged, and ensure calls to saveHealth and loadHealth still use the same
functions (CachedFeedProvider.recordSuccess, cacheResult, loadHealth,
saveHealth).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 516feb18-d428-4533-acf1-5e7d50c5d6f3
📒 Files selected for processing (11)
internal/constant/cache_namespace.gointernal/constant/ttl.gointernal/controller/topic_feed.gointernal/feedruntime/builder.gointernal/feedruntime/builder_test.gointernal/feedruntime/cached_provider.gointernal/util/cache.goweb/admin/src/api/topic.tsweb/admin/src/locale/en-US/topic.tsweb/admin/src/locale/zh-CN/topic.tsweb/admin/src/views/dashboard/topic_feed/detail.vue
Comment on lines
+60
to
+69
| func (p *CachedFeedProvider) cacheResult(feed *model.CraftFeed) { | ||
| data, err := json.Marshal(feed) | ||
| if err != nil { | ||
| logrus.Warnf("CachedFeedProvider [%s]: marshal failed: %v", p.URI, err) | ||
| return | ||
| } | ||
| if err := util.TryCacheSetString(subFeedResultKey(p.URI), string(data), constant.SubFeedCacheTTL); err != nil { | ||
| logrus.Warnf("CachedFeedProvider [%s]: cache write failed: %v", p.URI, err) | ||
| } | ||
| } |
There was a problem hiding this comment.
Avoid overwriting a good snapshot with an empty/degraded successful fetch.
cacheResult caches any non-error result, including a nil feed (which marshals to "null") or a successful response that transiently contains zero articles. When that happens the previous known-good snapshot is replaced, so a later failure falls back to an empty feed — undermining the purpose of the optimistic cache. Consider skipping the write when the feed is nil or has no articles.
🛡️ Proposed guard
// cacheResult writes feed to Redis with SubFeedCacheTTL.
func (p *CachedFeedProvider) cacheResult(feed *model.CraftFeed) {
+ // Don't overwrite a known-good snapshot with an empty/degraded result.
+ if feed == nil || len(feed.Articles) == 0 {
+ return
+ }
data, err := json.Marshal(feed)
if err != nil {
logrus.Warnf("CachedFeedProvider [%s]: marshal failed: %v", p.URI, err)
return
}📝 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.
Suggested change
| func (p *CachedFeedProvider) cacheResult(feed *model.CraftFeed) { | |
| data, err := json.Marshal(feed) | |
| if err != nil { | |
| logrus.Warnf("CachedFeedProvider [%s]: marshal failed: %v", p.URI, err) | |
| return | |
| } | |
| if err := util.TryCacheSetString(subFeedResultKey(p.URI), string(data), constant.SubFeedCacheTTL); err != nil { | |
| logrus.Warnf("CachedFeedProvider [%s]: cache write failed: %v", p.URI, err) | |
| } | |
| } | |
| func (p *CachedFeedProvider) cacheResult(feed *model.CraftFeed) { | |
| // Don't overwrite a known-good snapshot with an empty/degraded result. | |
| if feed == nil || len(feed.Articles) == 0 { | |
| return | |
| } | |
| data, err := json.Marshal(feed) | |
| if err != nil { | |
| logrus.Warnf("CachedFeedProvider [%s]: marshal failed: %v", p.URI, err) | |
| return | |
| } | |
| if err := util.TryCacheSetString(subFeedResultKey(p.URI), string(data), constant.SubFeedCacheTTL); err != nil { | |
| logrus.Warnf("CachedFeedProvider [%s]: cache write failed: %v", p.URI, err) | |
| } | |
| } |
🤖 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 `@internal/feedruntime/cached_provider.go` around lines 60 - 69, The
cacheResult function currently writes any successful result (including nil or
empty feeds) and thus can overwrite a good snapshot; modify
CachedFeedProvider.cacheResult to skip caching when the incoming feed is nil or
contains no articles (e.g., if feed == nil || len(feed.Articles) == 0) before
calling json.Marshal and TryCacheSetString (keep using subFeedResultKey(p.URI)
and util.TryCacheSetString); optionally emit a debug/log message when skipping
the cache write so the behavior is observable.
Comment on lines
+37
to
+75
| func tryGetRedisClient() (*redis.Client, error) { | ||
| envClient := GetEnvClient() | ||
| if envClient == nil { | ||
| return nil, errors.New("env client not available") | ||
| } | ||
| redisURI := strings.TrimSpace(envClient.GetString("REDIS_URI")) | ||
| if redisURI == "" { | ||
| return nil, errors.New("REDIS_URI is not configured") | ||
| } | ||
| opts, err := redis.ParseURL(redisURI) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| rdb := redis.NewClient(opts) | ||
| if rdb == nil { | ||
| return nil, errors.New("failed to create redis client") | ||
| } | ||
| return rdb, nil | ||
| } | ||
|
|
||
| // TryCacheSetString is like CacheSetString but returns an error instead of crashing | ||
| // when Redis is not configured. Safe to call in environments without Redis. | ||
| func TryCacheSetString(key string, value string, ttl time.Duration) error { | ||
| rdb, err := tryGetRedisClient() | ||
| if err != nil { | ||
| return err | ||
| } | ||
| return rdb.Set(context.Background(), key, value, ttl).Err() | ||
| } | ||
|
|
||
| // TryCacheGetString is like CacheGetString but returns an error instead of crashing | ||
| // when Redis is not configured. Safe to call in environments without Redis. | ||
| func TryCacheGetString(key string) (string, error) { | ||
| rdb, err := tryGetRedisClient() | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
| return rdb.Get(context.Background(), key).Result() | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Any Close() on redis clients?
rg -nP '\.Close\(\)' --type=go -C2
# Existing singleton/sync.Once redis client patterns?
rg -nP 'redis\.NewClient|sync\.Once|var\s+\w*[Rr]edis\w*\s' --type=go -C2Repository: Colin-XKL/FeedCraft
Length of output: 10121
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the current contents of internal/util/cache.go around the relevant functions
sed -n '1,140p' internal/util/cache.go
# Find all usages of the helper functions in the repo
rg -n "tryGetRedisClient|TryCacheSetString|TryCacheGetString" --type=go internal || true
# Check whether any caller closes the client returned by tryGetRedisClient
rg -n "tryGetRedisClient\(\)" -S --type=go internal || true
rg -n "rdb\.(Close|Dispose|Shutdown|Stop)\(" --type=go internal || trueRepository: Colin-XKL/FeedCraft
Length of output: 4886
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Inspect Redis client close/defer sites in dependency_monitor.go
sed -n '70,130p' internal/controller/dependency_monitor.go
# 2) Confirm no other callers of tryGetRedisClient (besides TryCache*)
rg -n "tryGetRedisClient" --type=go internal
# 3) Find any .Close() calls on redis clients (best-effort: look for Close() near rdb / redis.NewClient)
rg -n "rdb\.Close\(\)|redis\.NewClient|\.Close\(\)" --type=go internal/controller/dependency_monitor.go internal/util/cache.goRepository: Colin-XKL/FeedCraft
Length of output: 3226
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for any redis client close usage project-wide
rg -n --type=go "\.Close\(\)" internal | rg -n --fixed-string "redis" || true
rg -n --type=go "Close\(\)\s*$" internal || true
# Confirm whether tryGetRedisClient callers ever close returned client
rg -n --type=go "tryGetRedisClient\(\)" internal || true
rg -n --type=go "TryCache(Set|Get)String|CachedFunc" internal/feedruntime internal || true
# Check whether any other redis.NewClient usage exists besides dependency_monitor.go and util/cache.go
rg -n --type=go "redis\.NewClient\(" internal || trueRepository: Colin-XKL/FeedCraft
Length of output: 5067
Avoid creating a new Redis client/connection pool for every cache call
internal/util/cache.go’s tryGetRedisClient (and GetRedisClient via CacheSetString/CacheGetString) calls redis.NewClient(opts) on every invocation, while TryCacheSetString/TryCacheGetString never Close() the returned client; only internal/controller/dependency_monitor.go closes its own redis.NewClient. Reuse a single long-lived *redis.Client (e.g., sync.Once) across cache ops and close it during app shutdown to prevent accumulating idle pools.
🤖 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 `@internal/util/cache.go` around lines 37 - 75, The code creates a new
redis.Client on every call in tryGetRedisClient (used by
TryCacheSetString/TryCacheGetString and CacheSetString/CacheGetString) and never
closes them, causing many idle pools; change to a package-level singleton redis
client initialized once (use sync.Once or similar) and return that from
tryGetRedisClient/GetRedisClient instead of calling redis.NewClient each time,
ensure the existing Close call in internal/controller/dependency_monitor.go (or
add a single Close hook during app shutdown) closes the singleton client, and
remove per-call NewClient usage so all cache ops reuse the same long-lived
*redis.Client.
Comment on lines
+371
to
+396
| // Returns a colour for the sub-feed status tag. | ||
| // "stale" (using cache) = orange; "ok" (last_failure is absent or last_success is more recent) = green; unknown = gray. | ||
| const subFeedStatusColor = (record: SubFeedHealth) => { | ||
| if (!record.last_success_at && !record.last_failure_at) return 'gray'; | ||
| if (record.last_failure_at && !record.last_success_at) return 'red'; | ||
| if (record.last_failure_at && record.last_success_at) { | ||
| const failedAt = new Date(record.last_failure_at).getTime(); | ||
| const succeededAt = new Date(record.last_success_at).getTime(); | ||
| if (failedAt > succeededAt) return 'orange'; | ||
| } | ||
| return 'green'; | ||
| }; | ||
|
|
||
| const subFeedStatusLabel = (record: SubFeedHealth) => { | ||
| if (!record.last_success_at && !record.last_failure_at) | ||
| return t('topic.detail.subFeedHealth.status.unknown'); | ||
| if (record.last_failure_at && !record.last_success_at) | ||
| return t('topic.detail.subFeedHealth.status.stale'); | ||
| if (record.last_failure_at && record.last_success_at) { | ||
| const failedAt = new Date(record.last_failure_at).getTime(); | ||
| const succeededAt = new Date(record.last_success_at).getTime(); | ||
| if (failedAt > succeededAt) | ||
| return t('topic.detail.subFeedHealth.status.stale'); | ||
| } | ||
| return t('topic.detail.subFeedHealth.status.ok'); | ||
| }; |
There was a problem hiding this comment.
Color and label disagree for the failure-only state.
When last_failure_at is set but last_success_at is absent, subFeedStatusColor returns red while subFeedStatusLabel returns status.stale ("Using Cache"). A red tag labeled "Using Cache" is contradictory — with no prior success there is no cached snapshot to fall back to. The two helpers also duplicate the timestamp comparison, which is what allows them to drift.
Consider computing the status once and deriving both color and label from it, and giving the failure-only case its own label (e.g. a status.failed key) instead of reusing stale.
♻️ Single source of truth for status
- // Returns a colour for the sub-feed status tag.
- // "stale" (using cache) = orange; "ok" (last_failure is absent or last_success is more recent) = green; unknown = gray.
- const subFeedStatusColor = (record: SubFeedHealth) => {
- if (!record.last_success_at && !record.last_failure_at) return 'gray';
- if (record.last_failure_at && !record.last_success_at) return 'red';
- if (record.last_failure_at && record.last_success_at) {
- const failedAt = new Date(record.last_failure_at).getTime();
- const succeededAt = new Date(record.last_success_at).getTime();
- if (failedAt > succeededAt) return 'orange';
- }
- return 'green';
- };
-
- const subFeedStatusLabel = (record: SubFeedHealth) => {
- if (!record.last_success_at && !record.last_failure_at)
- return t('topic.detail.subFeedHealth.status.unknown');
- if (record.last_failure_at && !record.last_success_at)
- return t('topic.detail.subFeedHealth.status.stale');
- if (record.last_failure_at && record.last_success_at) {
- const failedAt = new Date(record.last_failure_at).getTime();
- const succeededAt = new Date(record.last_success_at).getTime();
- if (failedAt > succeededAt)
- return t('topic.detail.subFeedHealth.status.stale');
- }
- return t('topic.detail.subFeedHealth.status.ok');
- };
+ type SubFeedStatus = 'unknown' | 'failed' | 'stale' | 'ok';
+
+ const subFeedStatus = (record: SubFeedHealth): SubFeedStatus => {
+ if (!record.last_success_at && !record.last_failure_at) return 'unknown';
+ if (record.last_failure_at && !record.last_success_at) return 'failed';
+ if (record.last_failure_at && record.last_success_at) {
+ const failedAt = new Date(record.last_failure_at).getTime();
+ const succeededAt = new Date(record.last_success_at).getTime();
+ if (failedAt > succeededAt) return 'stale';
+ }
+ return 'ok';
+ };
+
+ const statusColorMap: Record<SubFeedStatus, string> = {
+ unknown: 'gray',
+ failed: 'red',
+ stale: 'orange',
+ ok: 'green',
+ };
+
+ const subFeedStatusColor = (record: SubFeedHealth) =>
+ statusColorMap[subFeedStatus(record)];
+
+ const subFeedStatusLabel = (record: SubFeedHealth) =>
+ t(`topic.detail.subFeedHealth.status.${subFeedStatus(record)}`);This assumes adding a topic.detail.subFeedHealth.status.failed key to both locale files (e.g. "Failed" / "失败").
📝 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.
Suggested change
| // Returns a colour for the sub-feed status tag. | |
| // "stale" (using cache) = orange; "ok" (last_failure is absent or last_success is more recent) = green; unknown = gray. | |
| const subFeedStatusColor = (record: SubFeedHealth) => { | |
| if (!record.last_success_at && !record.last_failure_at) return 'gray'; | |
| if (record.last_failure_at && !record.last_success_at) return 'red'; | |
| if (record.last_failure_at && record.last_success_at) { | |
| const failedAt = new Date(record.last_failure_at).getTime(); | |
| const succeededAt = new Date(record.last_success_at).getTime(); | |
| if (failedAt > succeededAt) return 'orange'; | |
| } | |
| return 'green'; | |
| }; | |
| const subFeedStatusLabel = (record: SubFeedHealth) => { | |
| if (!record.last_success_at && !record.last_failure_at) | |
| return t('topic.detail.subFeedHealth.status.unknown'); | |
| if (record.last_failure_at && !record.last_success_at) | |
| return t('topic.detail.subFeedHealth.status.stale'); | |
| if (record.last_failure_at && record.last_success_at) { | |
| const failedAt = new Date(record.last_failure_at).getTime(); | |
| const succeededAt = new Date(record.last_success_at).getTime(); | |
| if (failedAt > succeededAt) | |
| return t('topic.detail.subFeedHealth.status.stale'); | |
| } | |
| return t('topic.detail.subFeedHealth.status.ok'); | |
| }; | |
| type SubFeedStatus = 'unknown' | 'failed' | 'stale' | 'ok'; | |
| const subFeedStatus = (record: SubFeedHealth): SubFeedStatus => { | |
| if (!record.last_success_at && !record.last_failure_at) return 'unknown'; | |
| if (record.last_failure_at && !record.last_success_at) return 'failed'; | |
| if (record.last_failure_at && record.last_success_at) { | |
| const failedAt = new Date(record.last_failure_at).getTime(); | |
| const succeededAt = new Date(record.last_success_at).getTime(); | |
| if (failedAt > succeededAt) return 'stale'; | |
| } | |
| return 'ok'; | |
| }; | |
| const statusColorMap: Record<SubFeedStatus, string> = { | |
| unknown: 'gray', | |
| failed: 'red', | |
| stale: 'orange', | |
| ok: 'green', | |
| }; | |
| const subFeedStatusColor = (record: SubFeedHealth) => | |
| statusColorMap[subFeedStatus(record)]; | |
| const subFeedStatusLabel = (record: SubFeedHealth) => | |
| t(`topic.detail.subFeedHealth.status.${subFeedStatus(record)}`); |
🤖 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 `@web/admin/src/views/dashboard/topic_feed/detail.vue` around lines 371 - 396,
Both helpers subFeedStatusColor and subFeedStatusLabel compute status logic
separately and disagree for the case where last_failure_at exists but
last_success_at is absent; centralize by creating a single status computation
(e.g., computeSubFeedStatus(record: SubFeedHealth) returning an explicit
enum/value like 'unknown'|'failed'|'stale'|'ok') and then have
subFeedStatusColor and subFeedStatusLabel map that single status to color and to
i18n keys respectively; ensure the failure-only case maps to a new i18n key
(topic.detail.subFeedHealth.status.failed) and remove the duplicated timestamp
comparisons from both functions so they use the shared computeSubFeedStatus
function.
Reuse the existing util.GetMD5Hash helper instead of importing crypto/sha256 directly. MD5 is sufficient for cache key derivation. Co-authored-by: Colin <Colin_XKL@outlook.com>
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+47
to
+56
| // Live fetch failed — try the cached snapshot. | ||
| cached := p.loadCachedResult() | ||
| p.recordFailure(err) | ||
| if cached != nil { | ||
| logrus.Infof("CachedFeedProvider [%s]: live fetch failed (%v), returning cached snapshot", p.URI, err) | ||
| return cached, nil | ||
| } | ||
|
|
||
| // No cache available; propagate the original error. | ||
| return nil, err |
Contributor
There was a problem hiding this comment.
1. Cached fetch failure not propagated 📘 Rule violation ☼ Reliability
CachedFeedProvider.Fetch returns cached data with a nil error when the live fetch fails, preventing upstream callers from detecting degraded behavior. This violates the requirement to propagate failures instead of swallowing errors.
Agent Prompt
## Issue description
`CachedFeedProvider.Fetch` swallows the live fetch error when a cached snapshot exists by returning `(cached, nil)`. This prevents upstream layers (topic aggregation, observability, callers) from detecting and reacting to failures.
## Issue Context
Compliance requires errors from fallible operations to be propagated (or otherwise made explicitly detectable/actionable), not converted into success silently.
## Fix Focus Areas
- internal/feedruntime/cached_provider.go[47-56]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+85
to
+94
| // recordSuccess updates the health metadata after a live successful fetch. | ||
| func (p *CachedFeedProvider) recordSuccess() { | ||
| now := time.Now() | ||
| h := p.loadHealth() | ||
| h.URI = p.URI | ||
| h.LastSuccessAt = &now | ||
| h.CachedAt = &now | ||
| h.LastError = "" | ||
| p.saveHealth(h) | ||
| } |
Contributor
There was a problem hiding this comment.
2. Cached_at set without cache 🐞 Bug ◔ Observability
SubFeedHealth.CachedAt is documented as “when the last successful result was written to the cache”, but recordSuccess sets it unconditionally even if the Redis write in cacheResult fails, making health data claim a cached snapshot exists when it might not.
Agent Prompt
### Issue description
`recordSuccess()` always sets `CachedAt = now` even when `cacheResult()` fails to write to Redis. This contradicts the struct comment and makes the admin “Cache Written” timestamp unreliable.
### Issue Context
`Fetch()` calls `cacheResult(feed)` and then `recordSuccess()` on any successful upstream fetch; `cacheResult()` only logs on Redis write failure and does not communicate success/failure to `recordSuccess()`.
### Fix Focus Areas
- internal/feedruntime/cached_provider.go[39-57]
- internal/feedruntime/cached_provider.go[59-69]
- internal/feedruntime/cached_provider.go[85-104]
### Implementation notes
- Make `cacheResult` return `bool` (or `error`) indicating whether the Redis write succeeded.
- Pass the outcome into `recordSuccess` and only set `CachedAt` when the write succeeded.
- Alternatively, rename the field/comment to reflect “last successful fetch attempt” if that’s the intended meaning (and update UI label accordingly).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Co-authored-by: Colin <Colin_XKL@outlook.com>
…ntHash - GetMD5Hash renamed to getMD5Hash (package-private) - GetPasswordMD5Hash is now the only exported MD5 entry point (security-focused) - adapter/embedding.go and feedruntime/cached_provider.go switched to GetTextContentHash (FNV-64a, efficiency-focused) for cache key derivation - hash_test.go updated to test via GetPasswordMD5Hash Co-authored-by: Colin <Colin_XKL@outlook.com>
Co-authored-by: Colin <Colin_XKL@outlook.com>
Co-authored-by: Colin <Colin_XKL@outlook.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
2 participants
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.
Overview
When a sub-feed inside a TopicFeed fails to fetch, the topic feed now automatically falls back to the last cached result (stored in Redis for up to 14 days), keeping the aggregated feed available even during partial upstream failures.
A new "Sub-Feed Cache Health" section in the admin topic detail page gives operators visibility into per-URI success/failure history, last successful update time, and recent error messages.
Changes
Backend
internal/constant/ttl.go: AddSubFeedCacheTTL(14 days) andSubFeedHealthTTL(30 days)internal/constant/cache_namespace.go: AddPrefixSubFeedResultandPrefixSubFeedHealthRedis key prefixesinternal/feedruntime/cached_provider.go(new):CachedFeedProviderwraps anyFeedProvider; on successful fetch caches theCraftFeedin Redis; on failure returns the last cached snapshot. Health metadata (last_success_at,last_failure_at,last_error,cached_at) is stored per URI in Redis.internal/feedruntime/builder.go: Wrap every topic input withCachedFeedProviderduringBuildTopicinternal/util/cache.go: AddTryCacheSetString/TryCacheGetString— safe variants that return errors instead of callinglog.Fatalfwhen Redis is unconfigured (required for test environments)internal/controller/topic_feed.go: Addsub_feed_health []SubFeedHealthtoTopicDetailResponse; populated from Redis viafeedruntime.GetSubFeedHealthFrontend
web/admin/src/api/topic.ts: AddSubFeedHealthtype; extendTopicDetailweb/admin/src/views/dashboard/topic_feed/detail.vue: New "Sub-Feed Cache Health" card in the topic detail page — shows per-URI status (green/orange/red), last success time, last failure time, last error text, and cache timestampweb/admin/src/locale/zh-CN/topic.ts&en-US/topic.ts: i18n keys for the new sectionTests
internal/feedruntime/builder_test.go: UpdateTestBuildTopicProvider_NestedTopicsto unwrapCachedFeedProviderbefore asserting inner typeDemo
topic_subfeed_health_demo.mp4
Topic detail page — sub-feed health with one OK feed and one failing feed:
Topic detail sub-feed health section
Topic with fully-failing feed served from cache:
Topic cache fallback detail
Behavior Notes
CachedFeedProvideris transparent — if live fetch fails but cache exists, the parentTopicFeedsees a successful result (no error propagation). The degraded state is visible only via the per-URI health section in admin.TryCacheSetString/TryCacheGetStringnever crash in environments without Redis — existing controller tests continue to pass.To show artifacts inline, enable in settings.
Summary by Sourcery
Add optimistic caching and per-URI health tracking for topic sub-feeds and expose this metadata in the admin topic detail view.
New Features:
Enhancements:
Tests: