feat: unify feed preview targets - #780
Merged
Merged
Conversation
Co-authored-by: Colin <Colin_XKL@outlook.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
Reviewer's GuideUnifies the Feed Viewer to accept both external URLs and internal feedcraft:// URIs (recipes, topics, inboxes), adds an inbox runtime provider, routes admin UI preview actions through these internal targets, and tightens validation, error handling, and tests around the new input model and observability triggers. Sequence diagram for unified feed preview flow via feedcraft URIssequenceDiagram
actor AdminUser
participant TopicPage
participant FeedViewerPage
participant FeedViewerController as PreviewFeedViewer
participant FeedruntimeBuilder as BuildProviderFromInputWithRecipeTrigger
participant InboxProvider
participant DB
AdminUser->>TopicPage: click previewTopic / previewInbox / previewRecipe
TopicPage->>FeedViewerPage: router.push(name=FeedViewer, query target+id)
FeedViewerPage->>FeedViewerPage: applyRouteQuery
FeedViewerPage->>FeedViewerPage: selectInputURI
FeedViewerPage->>FeedViewerPage: fetchFeed
FeedViewerPage->>FeedViewerController: GET /api/feed_viewer/preview
FeedViewerController->>FeedViewerController: validateFeedViewerInputURI
FeedViewerController->>FeedruntimeBuilder: BuildProviderFromInputWithRecipeTrigger(InputKindURI, URI)
FeedruntimeBuilder->>FeedruntimeBuilder: buildProviderFromURI
FeedruntimeBuilder->>InboxProvider: new InboxProvider(DB, InboxID)
FeedViewerController->>InboxProvider: Fetch
InboxProvider->>DB: GetInboxByID
InboxProvider->>DB: ListInboxItems
InboxProvider-->>FeedViewerController: *model.CraftFeed
FeedViewerController-->>FeedViewerPage: FeedViewerPreview
FeedViewerPage-->>AdminUser: render FeedViewContainer
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| Complexity | 5 medium |
🟢 Metrics 93 complexity · 7 duplication
Metric Results Complexity 93 Duplication 7
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
![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 expands the Feed Viewer's capabilities to support internal resource types, including Custom Recipes, Topic Feeds, and Inboxes, via a new feedcraft:// URI scheme. The backend was updated with a new InboxProvider and enhanced URI validation, while the frontend now features a selection-based interface for previewing these resources and direct "Preview" buttons in management views. Feedback suggests that the InboxProvider.Fetch implementation should be updated to use the request context for database operations to support proper cancellation and timeouts.
Comment on lines
+451
to
+465
| _ = ctx | ||
| db := p.DB | ||
| if db == nil { | ||
| db = util.GetDatabase() | ||
| } | ||
|
|
||
| inbox, err := dao.GetInboxByID(db, p.InboxID) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to get inbox %s: %w", p.InboxID, err) | ||
| } | ||
|
|
||
| items, err := dao.ListInboxItems(db, p.InboxID) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to list inbox items: %w", err) | ||
| } |
Contributor
There was a problem hiding this comment.
The ctx parameter is currently ignored in InboxProvider.Fetch. It is best practice to use db.WithContext(ctx) to ensure that database operations respect the request context, allowing for proper timeout and cancellation handling.
Suggested change
| _ = ctx | |
| db := p.DB | |
| if db == nil { | |
| db = util.GetDatabase() | |
| } | |
| inbox, err := dao.GetInboxByID(db, p.InboxID) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to get inbox %s: %w", p.InboxID, err) | |
| } | |
| items, err := dao.ListInboxItems(db, p.InboxID) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to list inbox items: %w", err) | |
| } | |
| db := p.DB | |
| if db == nil { | |
| db = util.GetDatabase() | |
| } | |
| db = db.WithContext(ctx) | |
| inbox, err := dao.GetInboxByID(db, p.InboxID) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to get inbox %s: %w", p.InboxID, err) | |
| } | |
| items, err := dao.ListInboxItems(db, p.InboxID) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to list inbox items: %w", err) | |
| } | |
Co-authored-by: Colin <Colin_XKL@outlook.com>
Co-authored-by: Colin <Colin_XKL@outlook.com>
Colin-XKL
marked this pull request as ready for review
May 23, 2026 16:21
Contributor
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
![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 1 issue, and left some high level feedback:
- In
validateFeedViewerInputURI, consider reusing the internal resource type constants fromfeedruntime(e.g.,internalResourceTypeRecipe/topic/inbox) instead of hard‑coded string literals so supported types stay in sync between validation and runtime building. - The
InboxProvider.Fetchimplementation ignores thectxpassed in; wiring this context into the DB calls (including cancellation/timeout propagation) would make inbox previews more robust under request cancellation. - In
feed_viewer.vue’sselectInputURI,new URL(inputURI)will treat some inputs as relative URLs and can throw in edge cases; it may be safer to explicitly branch onfeedcraft://andhttp(s)://prefixes (or supply a base URL) rather than relying onURL’s default parsing behavior.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `validateFeedViewerInputURI`, consider reusing the internal resource type constants from `feedruntime` (e.g., `internalResourceTypeRecipe/topic/inbox`) instead of hard‑coded string literals so supported types stay in sync between validation and runtime building.
- The `InboxProvider.Fetch` implementation ignores the `ctx` passed in; wiring this context into the DB calls (including cancellation/timeout propagation) would make inbox previews more robust under request cancellation.
- In `feed_viewer.vue`’s `selectInputURI`, `new URL(inputURI)` will treat some inputs as relative URLs and can throw in edge cases; it may be safer to explicitly branch on `feedcraft://` and `http(s)://` prefixes (or supply a base URL) rather than relying on `URL`’s default parsing behavior.
## Individual Comments
### Comment 1
<location path="internal/feedruntime/builder.go" line_range="501-502" />
<code_context>
+ return feed, nil
+}
+
+func (p *InboxProvider) BaseURL() string {
+ return fmt.Sprintf("feedcraft://inbox/%s", p.InboxID)
+}
+
</code_context>
<issue_to_address>
**nitpick:** Reuse the internal inbox resource type constant when constructing the feedcraft URI
`BaseURL` formats the URI as `feedcraft://inbox/%s` with a literal "inbox", while `internalResourceTypeInbox` is already defined and used for parsing. Please use that constant here so URI construction and parsing stay in sync if the identifier ever changes.
</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
+501
to
+502
| func (p *InboxProvider) BaseURL() string { | ||
| return fmt.Sprintf("feedcraft://inbox/%s", p.InboxID) |
Contributor
There was a problem hiding this comment.
nitpick: Reuse the internal inbox resource type constant when constructing the feedcraft URI
BaseURL formats the URI as feedcraft://inbox/%s with a literal "inbox", while internalResourceTypeInbox is already defined and used for parsing. Please use that constant here so URI construction and parsing stay in sync if the identifier ever changes.
WalkthroughThe PR extends feed preview to support ChangesMulti-target Feed Preview with Internal Resource URIs
Possibly Related PRs
Poem
🚥 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 docstrings
🧪 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: 1
🧹 Nitpick comments (2)
internal/controller/feed_viewer_test.go (1)
138-214: ⚡ Quick winAdd explicit 404-path tests for missing internal resources.
The new URI tests cover success and craft-name validation, but not the new not-found classification contract. Add cases for missing
feedcraft://recipe/...,feedcraft://topic/..., andfeedcraft://inbox/...expecting HTTP 404 and the “selected preview target was not found” message.🤖 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/feed_viewer_test.go` around lines 138 - 214, Add three new test cases in internal/controller/feed_viewer_test.go that call performFeedViewerPreviewRequest with missing internal IDs and assert HTTP 404 and the “selected preview target was not found” message: one for "feedcraft://recipe/<missing>", one for "feedcraft://topic/<missing>", and one for "feedcraft://inbox/<missing>". Reuse the test pattern from TestPreviewFeedViewerSupportsRecipeURI/TestPreviewFeedViewerSupportsTopicURI/TestPreviewFeedViewerSupportsInboxURI (gin.SetMode, no DB setup for the missing ID), check recorder.Code == http.StatusNotFound and unmarshal the response body to assert response.Msg contains "selected preview target was not found". Ensure tests are named clearly (e.g., TestPreviewFeedViewerMissingRecipeReturns404, TestPreviewFeedViewerMissingTopicReturns404, TestPreviewFeedViewerMissingInboxReturns404) and use performFeedViewerPreviewRequest to exercise the handler.internal/feedruntime/builder.go (1)
461-476: ⚡ Quick winPropagate request context into inbox DB queries.
InboxProvider.Fetchcurrently discardsctx(_ = ctx), and the DAO functions only accept*gorm.DB, so upstream cancellation/deadlines won’t reach the DB unlessdb = db.WithContext(ctx)is applied beforeGetInboxByID/ListInboxItems.♻️ Suggested change
func (p *InboxProvider) Fetch(ctx context.Context) (*model.CraftFeed, error) { - _ = ctx db := p.DB if db == nil { db = util.GetDatabase() } + if ctx != nil { + db = db.WithContext(ctx) + } inbox, err := dao.GetInboxByID(db, p.InboxID) if err != nil { return nil, fmt.Errorf("failed to get inbox %s: %w", p.InboxID, 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/builder.go` around lines 461 - 476, InboxProvider.Fetch currently ignores the incoming ctx and calls DAO functions with a plain *gorm.DB, so propagate the request context into DB queries by calling db = db.WithContext(ctx) (or otherwise attaching ctx to the existing *gorm.DB) before invoking GetInboxByID and ListInboxItems; update the code path in InboxProvider.Fetch to use the context-aware db when calling GetInboxByID and ListInboxItems so cancellations/deadlines are honored.
🤖 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 `@web/admin/src/views/dashboard/feed_viewer/feed_viewer.vue`:
- Around line 167-171: clearPreviewState currently increments previewRequestSeq
and clears errorMessage/feedContent but doesn't reset isLoading, which can leave
the UI stuck; update clearPreviewState to set isLoading.value = false, and also
update the early-return path that checks the preview sequence (the sequence
check in the preview fetch/handler around the previewRequestSeq comparison) to
set isLoading.value = false before returning when a stale response is detected.
Ensure you reference clearPreviewState, previewRequestSeq, isLoading,
feedContent, and the preview sequence check in the async preview-fetch function
so both invalidation points clear the loading flag.
---
Nitpick comments:
In `@internal/controller/feed_viewer_test.go`:
- Around line 138-214: Add three new test cases in
internal/controller/feed_viewer_test.go that call
performFeedViewerPreviewRequest with missing internal IDs and assert HTTP 404
and the “selected preview target was not found” message: one for
"feedcraft://recipe/<missing>", one for "feedcraft://topic/<missing>", and one
for "feedcraft://inbox/<missing>". Reuse the test pattern from
TestPreviewFeedViewerSupportsRecipeURI/TestPreviewFeedViewerSupportsTopicURI/TestPreviewFeedViewerSupportsInboxURI
(gin.SetMode, no DB setup for the missing ID), check recorder.Code ==
http.StatusNotFound and unmarshal the response body to assert response.Msg
contains "selected preview target was not found". Ensure tests are named clearly
(e.g., TestPreviewFeedViewerMissingRecipeReturns404,
TestPreviewFeedViewerMissingTopicReturns404,
TestPreviewFeedViewerMissingInboxReturns404) and use
performFeedViewerPreviewRequest to exercise the handler.
In `@internal/feedruntime/builder.go`:
- Around line 461-476: InboxProvider.Fetch currently ignores the incoming ctx
and calls DAO functions with a plain *gorm.DB, so propagate the request context
into DB queries by calling db = db.WithContext(ctx) (or otherwise attaching ctx
to the existing *gorm.DB) before invoking GetInboxByID and ListInboxItems;
update the code path in InboxProvider.Fetch to use the context-aware db when
calling GetInboxByID and ListInboxItems so cancellations/deadlines are honored.
🪄 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: c1c976ea-7f6f-493b-80c5-811ea1ed2e16
📒 Files selected for processing (15)
internal/controller/feed_viewer.gointernal/controller/feed_viewer_test.gointernal/feedruntime/builder.gointernal/feedruntime/builder_test.goweb/admin/src/locale/en-US/feedViewer.tsweb/admin/src/locale/en-US/inbox.tsweb/admin/src/locale/en-US/topic.tsweb/admin/src/locale/zh-CN/feedViewer.tsweb/admin/src/locale/zh-CN/inbox.tsweb/admin/src/locale/zh-CN/topic.tsweb/admin/src/views/dashboard/custom_recipe/custom_recipe.vueweb/admin/src/views/dashboard/feed_viewer/feed_viewer.vueweb/admin/src/views/dashboard/inbox/index.vueweb/admin/src/views/dashboard/topic_feed/detail.vueweb/admin/src/views/dashboard/topic_feed/topic_feed.vue
Comment on lines
+167
to
+171
| function clearPreviewState() { | ||
| previewRequestSeq += 1; | ||
| errorMessage.value = ''; | ||
| feedContent.value = null; | ||
| } |
There was a problem hiding this comment.
Reset loading state when invalidating an in-flight preview request.
When clearPreviewState() runs (Line 167), it invalidates pending requests, but it does not clear isLoading. Because stale requests fail the sequence check in Line 193, the loading flag can stay stuck true.
💡 Suggested fix
function clearPreviewState() {
previewRequestSeq += 1;
+ isLoading.value = false;
errorMessage.value = '';
feedContent.value = null;
}Also applies to: 193-195
🤖 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/feed_viewer/feed_viewer.vue` around lines 167 -
171, clearPreviewState currently increments previewRequestSeq and clears
errorMessage/feedContent but doesn't reset isLoading, which can leave the UI
stuck; update clearPreviewState to set isLoading.value = false, and also update
the early-return path that checks the preview sequence (the sequence check in
the preview fetch/handler around the previewRequestSeq comparison) to set
isLoading.value = false before returning when a stale response is detected.
Ensure you reference clearPreviewState, previewRequestSeq, isLoading,
feedContent, and the preview sequence check in the async preview-fetch function
so both invalidation points clear the loading flag.
This was referenced May 23, 2026
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.
Summary
feedcraft://inbox/:idruntime support so Inbox preview does not require an intermediate Recipe.user_requestobservability trigger for Recipe previews while keeping Topic aggregation semantics intact.Walkthrough
unified_feed_preview_final.mp4
Testing
task fixpassed with the existing frontend ESLint warning intopic_feed/detail.vue(no-console) and no errors.go test ./...passed.task backend-buildpassed.task frontend-buildpassed with existing Browserslist/eval build warnings.To show artifacts inline, enable in settings.
Summary by Sourcery
Unify Feed Viewer preview handling around runtime input URIs, adding internal support for recipes, topics, and inboxes and wiring UI entry points to these targets.
New Features:
Bug Fixes:
Enhancements:
Tests: