Harden embedding filter configuration - #762
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 GuideHardens embedding filter configuration and error handling across backend and admin UI by tightening embedding config fallback rules, making anchors mandatory, improving cache keying, and adding dedicated embedding-filter parameters and validation to the dashboard form. 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 155 complexity · 10 duplication
Metric Results Complexity 155 Duplication 10
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 enhances the embedding filter by implementing fallback configuration logic to LLM settings, improving error humanization for better user feedback, and refining the admin UI with specialized input components for filter parameters. It also updates the logic to treat empty anchor lists as a configuration error. The review feedback suggests optimizing the caching layer by replacing string-formatted keys with struct-based keys in sync.Map to prevent delimiter collisions and reduce redundant MD5 hashing during vector lookups.
Comment on lines
+121
to
+130
| func buildAnchorVectorCacheKey(anchor string, cfg embeddingConfig, effectiveInstruction string) string { | ||
| return fmt.Sprintf( | ||
| "anchor|%s|%s|%s|%s|%s", | ||
| cfg.apiType, | ||
| cfg.apiBase, | ||
| cfg.apiModel, | ||
| util.GetMD5Hash(effectiveInstruction), | ||
| util.GetMD5Hash(anchor), | ||
| ) | ||
| } |
Contributor
There was a problem hiding this comment.
Using fmt.Sprintf with a pipe delimiter for cache keys can lead to collisions if any of the configuration fields (like apiBase or apiModel) contain the delimiter. Additionally, repeatedly hashing the effectiveInstruction inside this function is inefficient when processing multiple anchors.
Consider using a struct as the key for sync.Map, which is more idiomatic in Go and avoids delimiter injection issues. You should also pre-calculate the instruction hash outside the loop in GetOrComputeAnchorVectors.
type anchorCacheKey struct {
apiType, apiBase, apiModel, instrHash, anchorHash string
}
func buildAnchorVectorCacheKey(anchorHash string, cfg embeddingConfig, instructionHash string) anchorCacheKey {
return anchorCacheKey{
apiType: cfg.apiType,
apiBase: cfg.apiBase,
apiModel: cfg.apiModel,
instrHash: instructionHash,
anchorHash: anchorHash,
}
}|
|
||
| // getOrCreateEmbedder 获取或创建 Embedding 客户端（带缓存） | ||
| func getOrCreateEmbedder(cfg embeddingConfig) (*embeddings.EmbedderImpl, error) { | ||
| cacheKey := fmt.Sprintf("emb|%s|%s|%s|%s", cfg.apiType, cfg.apiBase, cfg.apiKey, cfg.apiModel) |
Contributor
There was a problem hiding this comment.
Similar to the anchor vector cache, using a string-formatted key for the embedding client cache is susceptible to delimiter injection. Using a struct key is safer and more efficient.
type embedderCacheKey struct {
apiType, apiBase, apiKey, apiModel string
}
cacheKey := embedderCacheKey{cfg.apiType, cfg.apiBase, cfg.apiKey, cfg.apiModel}
Comment on lines
309
to
310
| effectiveInstruction := resolveInstruction(instruction, cfg) | ||
| instructionHash := util.GetMD5Hash(effectiveInstruction) | ||
|
|
Contributor
There was a problem hiding this comment.
Pre-calculating the hash of the instruction here avoids redundant MD5 computations for every anchor in the list.
Suggested change
| effectiveInstruction := resolveInstruction(instruction, cfg) | |
| instructionHash := util.GetMD5Hash(effectiveInstruction) | |
| effectiveInstruction := resolveInstruction(instruction, cfg) | |
| instructionHash := util.GetMD5Hash(effectiveInstruction) |
Comment on lines
316
to
+317
| for i, anchor := range anchors { | ||
| cacheKey := fmt.Sprintf("anchor|%s|%s|%s", util.GetMD5Hash(anchor), modelName, instructionHash) | ||
| cacheKey := buildAnchorVectorCacheKey(anchor, cfg, effectiveInstruction) |
Contributor
There was a problem hiding this comment.
Update the cache key generation to use the pre-calculated instruction hash and the new struct-based key.
for i, anchor := range anchors {
anchorHash := util.GetMD5Hash(anchor)
cacheKey := buildAnchorVectorCacheKey(anchorHash, cfg, instructionHash)
Comment on lines
346
to
+347
| for j, idx := range uncachedIndices { | ||
| cacheKey := fmt.Sprintf("anchor|%s|%s|%s", util.GetMD5Hash(anchors[idx]), modelName, instructionHash) | ||
| cacheKey := buildAnchorVectorCacheKey(anchors[idx], cfg, effectiveInstruction) |
Contributor
There was a problem hiding this comment.
Update the cache key generation for newly computed vectors to maintain consistency with the lookup logic.
Suggested change
| for j, idx := range uncachedIndices { | |
| cacheKey := fmt.Sprintf("anchor|%s|%s|%s", util.GetMD5Hash(anchors[idx]), modelName, instructionHash) | |
| cacheKey := buildAnchorVectorCacheKey(anchors[idx], cfg, effectiveInstruction) | |
| for j, idx := range uncachedIndices { | |
| anchorHash := util.GetMD5Hash(anchors[idx]) | |
| cacheKey := buildAnchorVectorCacheKey(anchorHash, cfg, instructionHash) |
Comment on lines
+195
to
+197
| keyA := buildAnchorVectorCacheKey("anchor text", cfgA, "instruction") | ||
| keyB := buildAnchorVectorCacheKey("anchor text", cfgB, "instruction") | ||
| keyC := buildAnchorVectorCacheKey("anchor text", cfgC, "instruction") |
Contributor
There was a problem hiding this comment.
The test cases need to be updated to match the new signature of buildAnchorVectorCacheKey which now expects hashes and returns a struct.
Suggested change
| keyA := buildAnchorVectorCacheKey("anchor text", cfgA, "instruction") | |
| keyB := buildAnchorVectorCacheKey("anchor text", cfgB, "instruction") | |
| keyC := buildAnchorVectorCacheKey("anchor text", cfgC, "instruction") | |
| keyA := buildAnchorVectorCacheKey(util.GetMD5Hash("anchor text"), cfgA, util.GetMD5Hash("instruction")) | |
| keyB := buildAnchorVectorCacheKey(util.GetMD5Hash("anchor text"), cfgB, util.GetMD5Hash("instruction")) | |
| keyC := buildAnchorVectorCacheKey(util.GetMD5Hash("anchor text"), cfgC, util.GetMD5Hash("instruction")) |
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 17, 2026 09:40
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? |
WalkthroughAdds embedding-filter preview: backend config/caching fixes and anchor validation, a preview HTTP endpoint with normalization and error humanization, frontend API/types, an admin debug UI for testing/saving embedding-filter atoms, routing/locales, and multilingual docs. ChangesEmbedding Filter Preview & Admin Dashboard
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 |
![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:
- The default threshold (0.6) and max content length (2000) are now hard-coded both in the frontend (parseNumberParam/toCraftParamFormValue) and backend (embedding_filter.go defaults); consider centralizing or deriving these from a single source to avoid the values drifting over time.
- In validateEmbeddingFilterParams you re-encode the allowed modes as string literals; it might be safer to derive the valid values from embeddingFilterModeOptions (or a shared enum/consts) so the UI validation can’t get out of sync with the available mode options.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The default threshold (0.6) and max content length (2000) are now hard-coded both in the frontend (parseNumberParam/toCraftParamFormValue) and backend (embedding_filter.go defaults); consider centralizing or deriving these from a single source to avoid the values drifting over time.
- In validateEmbeddingFilterParams you re-encode the allowed modes as string literals; it might be safer to derive the valid values from embeddingFilterModeOptions (or a shared enum/consts) so the UI validation can’t get out of sync with the available mode options.
## Individual Comments
### Comment 1
<location path="web/admin/src/views/dashboard/craft_atom/craft_atom.vue" line_range="350-351" />
<code_context>
+ if (editedCraftAtom.value.template_name !== 'embedding-filter') {
+ return true;
+ }
+ if (!paramsMap.anchors?.trim()) {
+ Message.error('Embedding filter requires at least one anchor.');
+ return false;
+ }
</code_context>
<issue_to_address>
**suggestion:** Consider localizing the embedding-filter validation error messages instead of hardcoding English strings.
The `Message.error` calls in `validateEmbeddingFilterParams` use hardcoded English strings, while the rest of the form uses `vue-i18n`. Please move these error texts into the i18n resources and reference them with `t(...)` for consistency and localization support.
Suggested implementation:
```
if (!paramsMap.anchors?.trim()) {
Message.error(t('dashboard.craftAtom.embeddingFilter.anchorsRequired'));
return false;
}
```
```
if (paramsMap.mode && !['include', 'exclude'].includes(paramsMap.mode)) {
Message.error(t('dashboard.craftAtom.embeddingFilter.modeInvalid'));
return false;
```
To fully implement this:
1. Ensure that `t` from `vue-i18n` is available in this file. If not, wire it up, e.g. in `<script setup>`: `const { t } = useI18n();`, or in options API: `this.$t(...)` instead of `t(...)`, adjusting the above calls accordingly.
2. Add the two new translation keys to your i18n resources, for example:
- `dashboard.craftAtom.embeddingFilter.anchorsRequired`: `"Embedding filter requires at least one anchor."`
- `dashboard.craftAtom.embeddingFilter.modeInvalid`: `"Embedding filter mode must be include or exclude."`
3. If your project uses a different namespace/path convention for i18n keys, adapt the key names to match your existing structure and update the `t('...')` calls here.
</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
+350
to
+351
| if (!paramsMap.anchors?.trim()) { | ||
| Message.error('Embedding filter requires at least one anchor.'); |
Contributor
There was a problem hiding this comment.
suggestion: Consider localizing the embedding-filter validation error messages instead of hardcoding English strings.
The Message.error calls in validateEmbeddingFilterParams use hardcoded English strings, while the rest of the form uses vue-i18n. Please move these error texts into the i18n resources and reference them with t(...) for consistency and localization support.
Suggested implementation:
if (!paramsMap.anchors?.trim()) {
Message.error(t('dashboard.craftAtom.embeddingFilter.anchorsRequired'));
return false;
}
if (paramsMap.mode && !['include', 'exclude'].includes(paramsMap.mode)) {
Message.error(t('dashboard.craftAtom.embeddingFilter.modeInvalid'));
return false;
To fully implement this:
- Ensure that
tfromvue-i18nis available in this file. If not, wire it up, e.g. in<script setup>:const { t } = useI18n();, or in options API:this.$t(...)instead oft(...), adjusting the above calls accordingly. - Add the two new translation keys to your i18n resources, for example:
dashboard.craftAtom.embeddingFilter.anchorsRequired:"Embedding filter requires at least one anchor."dashboard.craftAtom.embeddingFilter.modeInvalid:"Embedding filter mode must be include or exclude."
- If your project uses a different namespace/path convention for i18n keys, adapt the key names to match your existing structure and update the
t('...')calls here.
Co-authored-by: Colin <Colin_XKL@outlook.com>
Co-authored-by: Colin <Colin_XKL@outlook.com>
Co-authored-by: Colin <Colin_XKL@outlook.com>
Co-authored-by: Colin <Colin_XKL@outlook.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/craft/embedding_filter.go (1)
45-65:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate anchors before the empty-items early return.
Right now, missing anchors are not reported when
feed.Itemsis empty, so configuration can still pass silently in that path. Validate anchors first.Proposed fix
func OptionEmbeddingFilter(anchors []string, threshold float64, maxContentLen int, instruction string, mode EmbeddingFilterMode) CraftOption { return func(feed *feeds.Feed, payload ExtraPayload) error { + // 1. 校验锚点（始终执行，避免空 feed 时静默通过） + if len(anchors) == 0 { + return errEmbeddingFilterAnchorsRequired + } + items := feed.Items if len(items) == 0 { return nil } @@ - // 1. 校验锚点 - if len(anchors) == 0 { - return errEmbeddingFilterAnchorsRequired - } - ctx := context.Background()🤖 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/craft/embedding_filter.go` around lines 45 - 65, The anchors validation currently happens after the early-return for empty feed items in OptionEmbeddingFilter, so missing anchors are not reported when feed.Items is empty; move the check that len(anchors) == 0 (the errEmbeddingFilterAnchorsRequired validation) to occur before the empty-items guard (the if len(items) == 0 { return nil } block) inside OptionEmbeddingFilter so configuration errors are surfaced regardless of feed.Items content, keeping the rest of the function (threshold clamping and subsequent logic) unchanged.
🧹 Nitpick comments (7)
doc-site/src/content/docs/en/guides/advanced/system-craft-atoms.md (2)
201-209: ⚡ Quick winUse
<Steps>for the Admin UI workflow section.This is a multi-step procedure and should use Starlight’s
<Steps>component for consistency.As per coding guidelines, "Use the
<Steps>component for multi-step instructions (e.g., deployment or configuration) in Starlight documentation."🤖 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 `@doc-site/src/content/docs/en/guides/advanced/system-craft-atoms.md` around lines 201 - 209, Replace the plain numbered procedure under the "Admin UI workflow" section with Starlight's <Steps> component: convert the current step list (steps referencing AtomCraft creation, selecting template "embedding-filter", filling "anchors", toggling "mode=include"/"exclude", saving, and usage in FlowCraft/Recipe/Feed Compare) into individual <Steps.Item> entries so the multi-step Admin UI workflow uses the <Steps> component for consistent formatting and rendering.
178-183: ⚡ Quick winPresent supported API types with
<Tabs>/<TabItem>.The three provider options are currently a bullet list; this should be rendered as tabs per docs conventions.
As per coding guidelines, "Use the
<Tabs>and<TabItem>components when showing multiple options (e.g., different deployment methods or model configurations) in Starlight documentation."🤖 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 `@doc-site/src/content/docs/en/guides/advanced/system-craft-atoms.md` around lines 178 - 183, Replace the current bullet list of supported FC_EMBEDDING_API_TYPE values with a Starlight <Tabs> component containing three <TabItem> entries labeled "openai", "gemini", and "ollama"; for each <TabItem> move the corresponding description into the tab body (openai: OpenAI or compatible embedding endpoint; gemini: note to set FC_EMBEDDING_API_BASE and FC_EMBEDDING_API_MODEL explicitly; ollama: note to set FC_EMBEDDING_API_BASE e.g. http://localhost:11434 and an embedding model like nomic-embed-text or bge-m3), preserving inline code formatting for environment variable names and example values and following existing docs conventions for Tabs/TabItem usage.doc-site/src/content/docs/zh-tw/guides/advanced/system-craft-atoms.md (2)
179-184: ⚡ Quick winPresent provider choices with
<Tabs>/<TabItem>.The
openai/gemini/ollamaalternatives are multiple options and should be rendered with tabs for consistency.As per coding guidelines, "Use the
<Tabs>and<TabItem>components when showing multiple options (e.g., different deployment methods or model configurations) in Starlight documentation."🤖 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 `@doc-site/src/content/docs/zh-tw/guides/advanced/system-craft-atoms.md` around lines 179 - 184, Replace the plain bullet list of FC_EMBEDDING_API_TYPE providers with a Tabs UI: create a <Tabs> containing three <TabItem> entries keyed by the provider values ("openai", "gemini", "ollama") and move each provider's explanatory text into the corresponding <TabItem>; reference the FC_EMBEDDING_API_TYPE variable and the specific model/base hints (FC_EMBEDDING_API_BASE, FC_EMBEDDING_API_MODEL, and example models like nomic-embed-text or bge-m3) inside each tab so readers can switch between provider-specific instructions.
204-210: ⚡ Quick winUse
<Steps>for the admin workflow sequence.This section is a multi-step procedure and should use Starlight’s
<Steps>component instead of a plain numbered list.As per coding guidelines, "Use the
<Steps>component for multi-step instructions (e.g., deployment or configuration) in Starlight documentation."🤖 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 `@doc-site/src/content/docs/zh-tw/guides/advanced/system-craft-atoms.md` around lines 204 - 210, The numbered multi-step procedure should be converted to use Starlight's <Steps> component: replace the plain numbered list with a <Steps> wrapper and individual <Step> entries (one per original line) preserving the text (e.g., steps about 打開 **工作台 → AtomCraft**, 新增 AtomCraft, 模板選擇, 填寫 anchors, 使用 mode, 儲存, 以及使用場景). Ensure each original step becomes its own <Step> element and keep any inline formatting (bold, code) intact so the admin workflow renders as the required Starlight steps sequence.doc-site/src/content/docs/zh/guides/advanced/system-craft-atoms.md (2)
204-210: ⚡ Quick winUse
<Steps>for this operation guide.This is a procedural sequence and should be converted from Markdown numbering to Starlight
<Steps>.As per coding guidelines, "Use the
<Steps>component for multi-step instructions (e.g., deployment or configuration) in Starlight documentation."🤖 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 `@doc-site/src/content/docs/zh/guides/advanced/system-craft-atoms.md` around lines 204 - 210, The numbered procedural list should be converted into a Starlight <Steps> component: wrap the sequence in <Steps>...</Steps> and replace each numbered line with a <Step> (or <Step title="...">) element containing that step's text (e.g., create AtomCraft, choose template, fill anchors, set mode include/exclude, save, use in FlowCraft/Recipe/Feed Compare or visit URL). Ensure the original content for anchors and mode (including example names like `ai-news-only` and `embedding-filter`) is preserved exactly inside the corresponding <Step> nodes.
179-184: ⚡ Quick winRender provider alternatives with
<Tabs>/<TabItem>.The three provider options are parallel choices and should be formatted with tabs for doc consistency.
As per coding guidelines, "Use the
<Tabs>and<TabItem>components when showing multiple options (e.g., different deployment methods or model configurations) in Starlight documentation."🤖 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 `@doc-site/src/content/docs/zh/guides/advanced/system-craft-atoms.md` around lines 179 - 184, Replace the current plain list of embedding providers with a Starlight tabbed UI using the <Tabs> and <TabItem> components: create three TabItem panes titled "openai", "gemini", and "ollama", move the respective bullet points under each tab, and keep the environment variable references FC_EMBEDDING_API_TYPE, FC_EMBEDDING_API_BASE, and FC_EMBEDDING_API_MODEL in the appropriate tab content (e.g., note that gemini requires explicit FC_EMBEDDING_API_BASE and FC_EMBEDDING_API_MODEL, and ollama requires FC_EMBEDDING_API_BASE and example models like nomic-embed-text or bge-m3). Ensure the markup uses <Tabs> as the container with each provider in its own <TabItem> for consistency with other Starlight docs.internal/adapter/embedding_test.go (1)
225-237: ⚡ Quick winAdd a direct test for comma-separated
FC_EMBEDDING_API_MODELrejection.
loadEmbeddingConfig()now rejects multi-model embedding values, but this path isn’t asserted yet.Proposed test case
+func TestLoadEmbeddingConfig_RejectsCommaSeparatedEmbeddingModel(t *testing.T) { + t.Setenv("FC_EMBEDDING_API_TYPE", "openai") + t.Setenv("FC_EMBEDDING_API_MODEL", "text-embedding-3-small,text-embedding-3-large") + + _, err := loadEmbeddingConfig() + assert.Error(t, err) + assert.Contains(t, err.Error(), "single embedding model") +}🤖 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/adapter/embedding_test.go` around lines 225 - 237, Add a new unit test that asserts loadEmbeddingConfig rejects comma-separated embedding model values: create TestLoadEmbeddingConfig_RejectsCommaSeparatedEmbeddingModel which sets FC_EMBEDDING_API_MODEL to a comma-separated string (e.g., "embed-a,embed-b") using t.Setenv, calls loadEmbeddingConfig(), and asserts that an error is returned (and that no valid cfg.apiModel is produced). Reference loadEmbeddingConfig and the new test name so reviewers can locate and validate the added rejection path.
🤖 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 @.env.example:
- Around line 30-31: The current comment is ambiguous about fallback semantics;
update the .env.example text to state explicitly that the embedding endpoint
fields (embedding type, embedding base, and embedding key) only fall back to the
LLM type/base/key when all three embedding endpoint fields are unset—if any of
EMBEDDING_TYPE, EMBEDDING_BASE, or EMBEDDING_KEY is provided, that value is used
and no partial per-field fallback occurs; keep the existing note that the
embedding model name is independent and must be set to an embedding model, not a
chat model.
In `@doc-site/src/content/docs/en/guides/advanced/system-craft-atoms.md`:
- Line 184: Clarify that the fallback to FC_LLM_API_TYPE, FC_LLM_API_BASE, and
FC_LLM_API_KEY only happens when all three embedding-specific env vars
(FC_EMBEDDING_API_TYPE, FC_EMBEDDING_API_BASE, FC_EMBEDDING_API_KEY) are unset —
not on a per-field basis — and update the sentence in system-craft-atoms.md to
state that partial presence of any FC_EMBEDDING_API_* value prevents using the
FC_LLM_API_* values for embeddings; also keep the note that
FC_EMBEDDING_API_MODEL is independent and that FC_LLM_API_MODEL is not reused
because it’s typically a chat model.
In `@web/admin/src/locale/zh-CN/embeddingFilterDebug.ts`:
- Line 14: The locale key embeddingFilterDebug.instruction currently uses
English "Instruction"; replace its value with the appropriate Chinese
translation (e.g., "说明" or "指令") so the zh-CN locale is consistent—update the
value for 'embeddingFilterDebug.instruction' in embeddingFilterDebug.ts to the
chosen Chinese string.
In `@web/admin/src/views/dashboard/craft_atom/craft_atom.vue`:
- Around line 351-359: Replace the hardcoded English messages passed to
Message.error in the embedding-filter validation block (the calls around
paramsMap.mode and the "requires at least one anchor" check) with i18n keys
using the app's translation helper (e.g. this.$t or i18n.t) and add
corresponding keys (e.g. errors.embeddingFilter.noAnchor and
errors.embeddingFilter.invalidMode) to the locale files; keep the existing logic
(trim/lowercase and inclusion check against ['include','exclude']) but call
Message.error with the translated string instead of the literal English text.
In `@web/admin/src/views/dashboard/craft_atom/paramOptions.ts`:
- Around line 95-98: parseNumberParam currently treats empty strings as "0"
because Number('') === 0; update parseNumberParam to treat blank/whitespace
strings as unset by returning the fallback immediately when value is undefined
or value.trim() === ''. After that, parse the value (e.g., Number(value)) and
return parsed if Number.isFinite(parsed) else fallback; reference
parseNumberParam to locate and change the logic.
In `@web/admin/src/views/dashboard/embedding_filter_debug/index.vue`:
- Around line 155-158: The modeOptions array currently hardcodes English labels;
replace the literal labels with localized strings by calling the app's i18n
translator (e.g., use this.$t or the composition API useI18n/t) when
constructing modeOptions so labels like 'include' and 'exclude' use translated
keys (e.g., t('embedding_filter.mode.include'),
t('embedding_filter.mode.exclude')) while keeping the value fields unchanged;
update where modeOptions is created (symbol: modeOptions) and ensure
corresponding translation keys are added to the locale files.
---
Outside diff comments:
In `@internal/craft/embedding_filter.go`:
- Around line 45-65: The anchors validation currently happens after the
early-return for empty feed items in OptionEmbeddingFilter, so missing anchors
are not reported when feed.Items is empty; move the check that len(anchors) == 0
(the errEmbeddingFilterAnchorsRequired validation) to occur before the
empty-items guard (the if len(items) == 0 { return nil } block) inside
OptionEmbeddingFilter so configuration errors are surfaced regardless of
feed.Items content, keeping the rest of the function (threshold clamping and
subsequent logic) unchanged.
---
Nitpick comments:
In `@doc-site/src/content/docs/en/guides/advanced/system-craft-atoms.md`:
- Around line 201-209: Replace the plain numbered procedure under the "Admin UI
workflow" section with Starlight's <Steps> component: convert the current step
list (steps referencing AtomCraft creation, selecting template
"embedding-filter", filling "anchors", toggling "mode=include"/"exclude",
saving, and usage in FlowCraft/Recipe/Feed Compare) into individual <Steps.Item>
entries so the multi-step Admin UI workflow uses the <Steps> component for
consistent formatting and rendering.
- Around line 178-183: Replace the current bullet list of supported
FC_EMBEDDING_API_TYPE values with a Starlight <Tabs> component containing three
<TabItem> entries labeled "openai", "gemini", and "ollama"; for each <TabItem>
move the corresponding description into the tab body (openai: OpenAI or
compatible embedding endpoint; gemini: note to set FC_EMBEDDING_API_BASE and
FC_EMBEDDING_API_MODEL explicitly; ollama: note to set FC_EMBEDDING_API_BASE
e.g. http://localhost:11434 and an embedding model like nomic-embed-text or
bge-m3), preserving inline code formatting for environment variable names and
example values and following existing docs conventions for Tabs/TabItem usage.
In `@doc-site/src/content/docs/zh-tw/guides/advanced/system-craft-atoms.md`:
- Around line 179-184: Replace the plain bullet list of FC_EMBEDDING_API_TYPE
providers with a Tabs UI: create a <Tabs> containing three <TabItem> entries
keyed by the provider values ("openai", "gemini", "ollama") and move each
provider's explanatory text into the corresponding <TabItem>; reference the
FC_EMBEDDING_API_TYPE variable and the specific model/base hints
(FC_EMBEDDING_API_BASE, FC_EMBEDDING_API_MODEL, and example models like
nomic-embed-text or bge-m3) inside each tab so readers can switch between
provider-specific instructions.
- Around line 204-210: The numbered multi-step procedure should be converted to
use Starlight's <Steps> component: replace the plain numbered list with a
<Steps> wrapper and individual <Step> entries (one per original line) preserving
the text (e.g., steps about 打開 **工作台 → AtomCraft**, 新增 AtomCraft, 模板選擇, 填寫
anchors, 使用 mode, 儲存, 以及使用場景). Ensure each original step becomes its own <Step>
element and keep any inline formatting (bold, code) intact so the admin workflow
renders as the required Starlight steps sequence.
In `@doc-site/src/content/docs/zh/guides/advanced/system-craft-atoms.md`:
- Around line 204-210: The numbered procedural list should be converted into a
Starlight <Steps> component: wrap the sequence in <Steps>...</Steps> and replace
each numbered line with a <Step> (or <Step title="...">) element containing that
step's text (e.g., create AtomCraft, choose template, fill anchors, set mode
include/exclude, save, use in FlowCraft/Recipe/Feed Compare or visit URL).
Ensure the original content for anchors and mode (including example names like
`ai-news-only` and `embedding-filter`) is preserved exactly inside the
corresponding <Step> nodes.
- Around line 179-184: Replace the current plain list of embedding providers
with a Starlight tabbed UI using the <Tabs> and <TabItem> components: create
three TabItem panes titled "openai", "gemini", and "ollama", move the respective
bullet points under each tab, and keep the environment variable references
FC_EMBEDDING_API_TYPE, FC_EMBEDDING_API_BASE, and FC_EMBEDDING_API_MODEL in the
appropriate tab content (e.g., note that gemini requires explicit
FC_EMBEDDING_API_BASE and FC_EMBEDDING_API_MODEL, and ollama requires
FC_EMBEDDING_API_BASE and example models like nomic-embed-text or bge-m3).
Ensure the markup uses <Tabs> as the container with each provider in its own
<TabItem> for consistency with other Starlight docs.
In `@internal/adapter/embedding_test.go`:
- Around line 225-237: Add a new unit test that asserts loadEmbeddingConfig
rejects comma-separated embedding model values: create
TestLoadEmbeddingConfig_RejectsCommaSeparatedEmbeddingModel which sets
FC_EMBEDDING_API_MODEL to a comma-separated string (e.g., "embed-a,embed-b")
using t.Setenv, calls loadEmbeddingConfig(), and asserts that an error is
returned (and that no valid cfg.apiModel is produced). Reference
loadEmbeddingConfig and the new test name so reviewers can locate and validate
the added rejection path.
🪄 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: a51cfe74-2226-4798-af87-2cd4ed2287ec
📒 Files selected for processing (22)
.env.exampledoc-site/src/content/docs/en/guides/advanced/system-craft-atoms.mddoc-site/src/content/docs/zh-tw/guides/advanced/system-craft-atoms.mddoc-site/src/content/docs/zh/guides/advanced/system-craft-atoms.mdinternal/adapter/embedding.gointernal/adapter/embedding_test.gointernal/controller/feed_viewer.gointernal/controller/feed_viewer_test.gointernal/craft/embedding_filter.gointernal/craft/embedding_filter_test.gointernal/router/registry.goweb/admin/src/api/embedding_filter.tsweb/admin/src/locale/en-US.tsweb/admin/src/locale/en-US/embeddingFilterDebug.tsweb/admin/src/locale/en-US/menu.tsweb/admin/src/locale/zh-CN.tsweb/admin/src/locale/zh-CN/embeddingFilterDebug.tsweb/admin/src/locale/zh-CN/menu.tsweb/admin/src/router/routes/modules/tools.tsweb/admin/src/views/dashboard/craft_atom/craft_atom.vueweb/admin/src/views/dashboard/craft_atom/paramOptions.tsweb/admin/src/views/dashboard/embedding_filter_debug/index.vue
Comment on lines
+30
to
+31
| # If type/base/key are not set, they fall back to the LLM type/base/key above. | ||
| # The embedding model name is independent; set it to an embedding model, not a chat model. |
There was a problem hiding this comment.
Clarify all-or-none fallback semantics.
Line 30 is ambiguous and can be read as per-field fallback. Please state explicitly that fallback happens only when all embedding endpoint fields are unset.
Suggested wording
-# If type/base/key are not set, they fall back to the LLM type/base/key above.
+# If embedding type/base/key are all unset, FeedCraft falls back to the LLM type/base/key above.📝 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
| # If type/base/key are not set, they fall back to the LLM type/base/key above. | |
| # The embedding model name is independent; set it to an embedding model, not a chat model. | |
| # If embedding type/base/key are all unset, FeedCraft falls back to the LLM type/base/key above. | |
| # The embedding model name is independent; set it to an embedding model, not a chat model. |
🤖 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 @.env.example around lines 30 - 31, The current comment is ambiguous about
fallback semantics; update the .env.example text to state explicitly that the
embedding endpoint fields (embedding type, embedding base, and embedding key)
only fall back to the LLM type/base/key when all three embedding endpoint fields
are unset—if any of EMBEDDING_TYPE, EMBEDDING_BASE, or EMBEDDING_KEY is
provided, that value is used and no partial per-field fallback occurs; keep the
existing note that the embedding model name is independent and must be set to an
embedding model, not a chat model.
| 'embeddingFilterDebug.mode': '模式', | ||
| 'embeddingFilterDebug.threshold': '阈值', | ||
| 'embeddingFilterDebug.maxContentLength': '最大正文长度', | ||
| 'embeddingFilterDebug.instruction': 'Instruction', |
There was a problem hiding this comment.
Localize the zh-CN label for instruction.
Line 14 uses English (Instruction) while this locale file is Chinese; this causes inconsistent UI language.
Suggested fix
- 'embeddingFilterDebug.instruction': 'Instruction',
+ 'embeddingFilterDebug.instruction': '指令',📝 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
| 'embeddingFilterDebug.instruction': 'Instruction', | |
| 'embeddingFilterDebug.instruction': '指令', |
🤖 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/locale/zh-CN/embeddingFilterDebug.ts` at line 14, The locale
key embeddingFilterDebug.instruction currently uses English "Instruction";
replace its value with the appropriate Chinese translation (e.g., "说明" or "指令")
so the zh-CN locale is consistent—update the value for
'embeddingFilterDebug.instruction' in embeddingFilterDebug.ts to the chosen
Chinese string.
Comment on lines
+351
to
+359
| Message.error('Embedding filter requires at least one anchor.'); | ||
| return false; | ||
| } | ||
| if (paramsMap.mode) { | ||
| paramsMap.mode = paramsMap.mode.trim().toLowerCase(); | ||
| } | ||
| if (paramsMap.mode && !['include', 'exclude'].includes(paramsMap.mode)) { | ||
| Message.error('Embedding filter mode must be include or exclude.'); | ||
| return false; |
There was a problem hiding this comment.
Use i18n keys for new embedding-filter validation errors.
Line 351 and Line 358 add hardcoded English error messages, which bypass locale support.
Suggested fix
if (!paramsMap.anchors?.trim()) {
- Message.error('Embedding filter requires at least one anchor.');
+ Message.error(t('craftAtom.message.embeddingFilterAnchorsRequired'));
return false;
}
@@
if (paramsMap.mode && !['include', 'exclude'].includes(paramsMap.mode)) {
- Message.error('Embedding filter mode must be include or exclude.');
+ Message.error(t('craftAtom.message.embeddingFilterModeInvalid'));
return false;
}📝 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
| Message.error('Embedding filter requires at least one anchor.'); | |
| return false; | |
| } | |
| if (paramsMap.mode) { | |
| paramsMap.mode = paramsMap.mode.trim().toLowerCase(); | |
| } | |
| if (paramsMap.mode && !['include', 'exclude'].includes(paramsMap.mode)) { | |
| Message.error('Embedding filter mode must be include or exclude.'); | |
| return false; | |
| Message.error(t('craftAtom.message.embeddingFilterAnchorsRequired')); | |
| return false; | |
| } | |
| if (paramsMap.mode) { | |
| paramsMap.mode = paramsMap.mode.trim().toLowerCase(); | |
| } | |
| if (paramsMap.mode && !['include', 'exclude'].includes(paramsMap.mode)) { | |
| Message.error(t('craftAtom.message.embeddingFilterModeInvalid')); | |
| return false; |
🤖 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/craft_atom/craft_atom.vue` around lines 351 -
359, Replace the hardcoded English messages passed to Message.error in the
embedding-filter validation block (the calls around paramsMap.mode and the
"requires at least one anchor" check) with i18n keys using the app's translation
helper (e.g. this.$t or i18n.t) and add corresponding keys (e.g.
errors.embeddingFilter.noAnchor and errors.embeddingFilter.invalidMode) to the
locale files; keep the existing logic (trim/lowercase and inclusion check
against ['include','exclude']) but call Message.error with the translated string
instead of the literal English text.
Comment on lines
+95
to
+98
| function parseNumberParam(value: string | undefined, fallback: number) { | ||
| const parsed = Number(value); | ||
| return Number.isFinite(parsed) ? parsed : fallback; | ||
| } |
There was a problem hiding this comment.
Treat blank numeric params as unset before parsing.
On Line 96, blank values are parsed as 0, so empty threshold/max-content-length can silently become 0 instead of the intended defaults (0.6 / 2000).
Suggested fix
function parseNumberParam(value: string | undefined, fallback: number) {
+ if (value == null || value.trim() === '') {
+ return fallback;
+ }
const parsed = Number(value);
return Number.isFinite(parsed) ? parsed : fallback;
}📝 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
| function parseNumberParam(value: string | undefined, fallback: number) { | |
| const parsed = Number(value); | |
| return Number.isFinite(parsed) ? parsed : fallback; | |
| } | |
| function parseNumberParam(value: string | undefined, fallback: number) { | |
| if (value == null || value.trim() === '') { | |
| return fallback; | |
| } | |
| const parsed = Number(value); | |
| return Number.isFinite(parsed) ? parsed : fallback; | |
| } |
🤖 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/craft_atom/paramOptions.ts` around lines 95 -
98, parseNumberParam currently treats empty strings as "0" because Number('')
=== 0; update parseNumberParam to treat blank/whitespace strings as unset by
returning the fallback immediately when value is undefined or value.trim() ===
''. After that, parse the value (e.g., Number(value)) and return parsed if
Number.isFinite(parsed) else fallback; reference parseNumberParam to locate and
change the logic.
Comment on lines
+155
to
+158
| const modeOptions = [ | ||
| { label: 'include', value: 'include' }, | ||
| { label: 'exclude', value: 'exclude' }, | ||
| ]; |
There was a problem hiding this comment.
Localize mode option labels instead of hardcoding English.
Line 156-Line 157 use fixed English labels, so the dropdown won’t fully localize in non-English admin sessions.
Suggested fix
-const modeOptions = [
- { label: 'include', value: 'include' },
- { label: 'exclude', value: 'exclude' },
-];
+const modeOptions = computed(() => [
+ { label: t('embeddingFilterDebug.modeInclude'), value: 'include' },
+ { label: t('embeddingFilterDebug.modeExclude'), value: 'exclude' },
+]);🤖 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/embedding_filter_debug/index.vue` around lines
155 - 158, The modeOptions array currently hardcodes English labels; replace the
literal labels with localized strings by calling the app's i18n translator
(e.g., use this.$t or the composition API useI18n/t) when constructing
modeOptions so labels like 'include' and 'exclude' use translated keys (e.g.,
t('embedding_filter.mode.include'), t('embedding_filter.mode.exclude')) while
keeping the value fields unchanged; update where modeOptions is created (symbol:
modeOptions) and ensure corresponding translation keys are added to the locale
files.
Co-authored-by: Colin <Colin_XKL@outlook.com>
….com/Colin-XKL/FeedCraft into cursor/improve-embedding-filter-c20c Co-authored-by: Colin <Colin_XKL@outlook.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/controller/feed_viewer.go`:
- Around line 402-410: The isEmbeddingFilterConfigurationError function is too
strict because it requires the "[embedding-filter]" prefix before matching
config-related substrings; update isEmbeddingFilterConfigurationError to treat
messages that contain any of the config hints ("failed to load embedding
config", "fc_embedding_", "anchors parameter is required") as configuration
errors even when the "[embedding-filter]" tag is absent, i.e., remove the
mandatory prefix check and return true if any of those substrings are present
(still using strings.ToLower(msg) and strings.Contains) so misclassified 500s
become actionable 400s.
In `@web/admin/src/views/dashboard/embedding_filter_debug/index.vue`:
- Around line 199-209: The validation warnings use hardcoded English strings via
Message.warning (e.g., "Threshold must be between 0 and 1." and the Max content
length message) which bypass i18n; update those calls to use the app's
localization function (e.g., $t or i18n.t) so messages are translated, e.g.,
replace the literal strings passed to Message.warning in the blocks that
validate threshold and maxContentLength (references: Message.warning,
maxContentLength, maxContentLengthLimit) with localized templates and
interpolate maxContentLengthLimit as needed.
- Around line 230-233: getErrorMessage currently returns only Error.message and
loses server-provided messages from axios responses; update it to detect axios
errors and prefer error.response?.data?.msg when present, falling back to
error.message or t('embeddingFilterDebug.message.unknownError') otherwise.
Specifically, modify getErrorMessage to: (1) accept the unknown error, (2) if
it's an axios error (use axios.isAxiosError or check for error.response),
extract and return response.data.msg (or response.data?.msg) when defined, (3)
else if error is instance of Error return error.message, (4) otherwise return
the existing t('embeddingFilterDebug.message.unknownError'). Ensure this change
covers the callers testFilter and saveAtomCraft so server messages surface in
those error handlers.
🪄 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: 08049a5b-e722-415b-a312-8fb21a46dc3f
📒 Files selected for processing (5)
internal/controller/feed_viewer.gointernal/controller/feed_viewer_test.gointernal/craft/embedding_filter.goweb/admin/src/api/embedding_filter.tsweb/admin/src/views/dashboard/embedding_filter_debug/index.vue
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/craft/embedding_filter.go
Comment on lines
+402
to
+410
| func isEmbeddingFilterConfigurationError(msg string) bool { | ||
| lowerMsg := strings.ToLower(msg) | ||
| if !strings.Contains(lowerMsg, "[embedding-filter]") { | ||
| return false | ||
| } | ||
| return strings.Contains(lowerMsg, "failed to load embedding config") || | ||
| strings.Contains(lowerMsg, "fc_embedding_") || | ||
| strings.Contains(lowerMsg, "anchors parameter is required") | ||
| } |
There was a problem hiding this comment.
Configuration-error detection is too strict and can misclassify as 500.
Line 404 requires "[embedding-filter]" to be present before checking config hints. If the error text is unprefixed (for example, direct "failed to load embedding config: ..."), it falls through to internal-error handling instead of returning actionable 400.
Suggested fix
func isEmbeddingFilterConfigurationError(msg string) bool {
lowerMsg := strings.ToLower(msg)
- if !strings.Contains(lowerMsg, "[embedding-filter]") {
- return false
- }
- return strings.Contains(lowerMsg, "failed to load embedding config") ||
- strings.Contains(lowerMsg, "fc_embedding_") ||
- strings.Contains(lowerMsg, "anchors parameter is required")
+ return strings.Contains(lowerMsg, "failed to load embedding config") ||
+ strings.Contains(lowerMsg, "fc_embedding_") ||
+ strings.Contains(lowerMsg, "anchors parameter is required")
}📝 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 isEmbeddingFilterConfigurationError(msg string) bool { | |
| lowerMsg := strings.ToLower(msg) | |
| if !strings.Contains(lowerMsg, "[embedding-filter]") { | |
| return false | |
| } | |
| return strings.Contains(lowerMsg, "failed to load embedding config") || | |
| strings.Contains(lowerMsg, "fc_embedding_") || | |
| strings.Contains(lowerMsg, "anchors parameter is required") | |
| } | |
| func isEmbeddingFilterConfigurationError(msg string) bool { | |
| lowerMsg := strings.ToLower(msg) | |
| return strings.Contains(lowerMsg, "failed to load embedding config") || | |
| strings.Contains(lowerMsg, "fc_embedding_") || | |
| strings.Contains(lowerMsg, "anchors parameter is required") | |
| } |
🤖 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.go` around lines 402 - 410, The
isEmbeddingFilterConfigurationError function is too strict because it requires
the "[embedding-filter]" prefix before matching config-related substrings;
update isEmbeddingFilterConfigurationError to treat messages that contain any of
the config hints ("failed to load embedding config", "fc_embedding_", "anchors
parameter is required") as configuration errors even when the
"[embedding-filter]" tag is absent, i.e., remove the mandatory prefix check and
return true if any of those substrings are present (still using
strings.ToLower(msg) and strings.Contains) so misclassified 500s become
actionable 400s.
Comment on lines
+199
to
+209
| Message.warning('Threshold must be between 0 and 1.'); | ||
| return null; | ||
| } | ||
| if ( | ||
| !Number.isFinite(maxContentLength) || | ||
| maxContentLength < 1 || | ||
| maxContentLength > maxContentLengthLimit | ||
| ) { | ||
| Message.warning( | ||
| `Max content length must be between 1 and ${maxContentLengthLimit}.` | ||
| ); |
There was a problem hiding this comment.
Localize validation warnings instead of hardcoded English strings.
These warning messages bypass i18n and will remain English in non-English admin sessions.
Suggested direction
- Message.warning('Threshold must be between 0 and 1.');
+ Message.warning(t('embeddingFilterDebug.message.thresholdRange'));
@@
- Message.warning(
- `Max content length must be between 1 and ${maxContentLengthLimit}.`
- );
+ Message.warning(
+ t('embeddingFilterDebug.message.maxContentLengthRange', {
+ max: maxContentLengthLimit,
+ })
+ );🤖 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/embedding_filter_debug/index.vue` around lines
199 - 209, The validation warnings use hardcoded English strings via
Message.warning (e.g., "Threshold must be between 0 and 1." and the Max content
length message) which bypass i18n; update those calls to use the app's
localization function (e.g., $t or i18n.t) so messages are translated, e.g.,
replace the literal strings passed to Message.warning in the blocks that
validate threshold and maxContentLength (references: Message.warning,
maxContentLength, maxContentLengthLimit) with localized templates and
interpolate maxContentLengthLimit as needed.
Comment on lines
+230
to
+233
| function getErrorMessage(error: unknown) { | ||
| return error instanceof Error | ||
| ? error.message | ||
| : t('embeddingFilterDebug.message.unknownError'); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify file exists and get basic info
if [ -f "web/admin/src/views/dashboard/embedding_filter_debug/index.vue" ]; then
wc -l "web/admin/src/views/dashboard/embedding_filter_debug/index.vue"
else
echo "File not found"
fiRepository: Colin-XKL/FeedCraft
Length of output: 130
🏁 Script executed:
#!/bin/bash
# Read the specific lines mentioned in the review comment (230-233 and surrounding context)
head -240 "web/admin/src/views/dashboard/embedding_filter_debug/index.vue" | tail -20Repository: Colin-XKL/FeedCraft
Length of output: 589
🏁 Script executed:
#!/bin/bash
# Check lines 255-256, 261-262, 295 to see if they have similar issues
sed -n '250,270p' "web/admin/src/views/dashboard/embedding_filter_debug/index.vue"Repository: Colin-XKL/FeedCraft
Length of output: 693
🏁 Script executed:
#!/bin/bash
# Check around line 295
sed -n '290,300p' "web/admin/src/views/dashboard/embedding_filter_debug/index.vue"Repository: Colin-XKL/FeedCraft
Length of output: 338
🏁 Script executed:
#!/bin/bash
# Search for axios imports and API calls in the file
head -100 "web/admin/src/views/dashboard/embedding_filter_debug/index.vue" | grep -E "import.*axios|axios\.|\.post|\.get|catch"Repository: Colin-XKL/FeedCraft
Length of output: 45
🏁 Script executed:
#!/bin/bash
# Check the full import section of the file
head -50 "web/admin/src/views/dashboard/embedding_filter_debug/index.vue"Repository: Colin-XKL/FeedCraft
Length of output: 2029
🏁 Script executed:
#!/bin/bash
# Look for axios or API client imports and usage
grep -n "axios\|import.*api\|from.*api" "web/admin/src/views/dashboard/embedding_filter_debug/index.vue" | head -20Repository: Colin-XKL/FeedCraft
Length of output: 239
🏁 Script executed:
#!/bin/bash
# Check the testFilter and saveAtomCraft functions more completely
sed -n '220,280p' "web/admin/src/views/dashboard/embedding_filter_debug/index.vue"Repository: Colin-XKL/FeedCraft
Length of output: 1782
🏁 Script executed:
#!/bin/bash
# Check line 295 and surrounding context more carefully
sed -n '285,306p' "web/admin/src/views/dashboard/embedding_filter_debug/index.vue"Repository: Colin-XKL/FeedCraft
Length of output: 652
🏁 Script executed:
#!/bin/bash
# Check the API implementations to see what error structure they return
cat "web/admin/src/api/feed_viewer.ts" | head -80Repository: Colin-XKL/FeedCraft
Length of output: 1006
🏁 Script executed:
#!/bin/bash
# Check embedding_filter API
cat "web/admin/src/api/embedding_filter.ts" | head -100Repository: Colin-XKL/FeedCraft
Length of output: 670
🏁 Script executed:
#!/bin/bash
# Check craft_atom API
cat "web/admin/src/api/craft_atom.ts" | head -100Repository: Colin-XKL/FeedCraft
Length of output: 1483
🏁 Script executed:
#!/antml/bash
# Search for error response patterns in the API directory
grep -r "response.*msg\|data.*msg" "web/admin/src/api/" --include="*.ts" | head -20Repository: Colin-XKL/FeedCraft
Length of output: 144
🏁 Script executed:
#!/bin/bash
# Check the APIResponse type definition
cat "web/admin/src/api/types.ts"Repository: Colin-XKL/FeedCraft
Length of output: 166
🏁 Script executed:
#!/bin/bash
# Check the interceptor to understand error handling
cat "web/admin/src/api/interceptor.ts"Repository: Colin-XKL/FeedCraft
Length of output: 2349
🏁 Script executed:
#!/antml:bash
# Look for test cases or error handling examples that show the msg field
grep -r "\.msg\|response.*msg" "web/admin/src" --include="*.ts" --include="*.tsx" --include="*.vue" -B 2 -A 2 | head -50Repository: Colin-XKL/FeedCraft
Length of output: 3889
🏁 Script executed:
#!/antml/bash
# Check if AxiosError is defined and what it extends
grep -r "AxiosError\|class.*Error" "web/admin/src/api" --include="*.ts" | head -20Repository: Colin-XKL/FeedCraft
Length of output: 45
🏁 Script executed:
#!/an/bash
# Verify the exact error handling flow - check how Promise.allSettled catches errors
sed -n '245,265p' "web/admin/src/views/dashboard/embedding_filter_debug/index.vue"Repository: Colin-XKL/FeedCraft
Length of output: 720
🏁 Script executed:
#!/bin/bash
# Look at what the API functions actually return on error
grep -A 10 "previewEmbeddingFilter\|previewFeed" "web/admin/src/api/embedding_filter.ts" "web/admin/src/api/feed_viewer.ts"Repository: Colin-XKL/FeedCraft
Length of output: 1410
🏁 Script executed:
#!/bin/bash
# Check the exact lines for the "Also applies to" - verify line 255-256
sed -n '253,264p' "web/admin/src/views/dashboard/embedding_filter_debug/index.vue"Repository: Colin-XKL/FeedCraft
Length of output: 450
🏁 Script executed:
#!/bin/bash
# Verify line 295 context for saveAtomCraft
sed -n '290,306p' "web/admin/src/views/dashboard/embedding_filter_debug/index.vue"Repository: Colin-XKL/FeedCraft
Length of output: 426
Extract server error details from axios responses in error handling.
The getErrorMessage function drops server error messages (msg field) from failed API responses. When requests fail with 4xx/5xx responses, axios errors have the actionable server feedback in error.response.data.msg, but the function only returns error.message (generic transport text).
This affects three call sites: lines 255, 261 (in testFilter), and 298 (in saveAtomCraft).
Suggested fix
<script lang="ts" setup>
+ import type { AxiosError } from 'axios';
import { computed, reactive, ref } from 'vue';
import { Message } from '`@arco-design/web-vue`';
@@
function getErrorMessage(error: unknown) {
- return error instanceof Error
- ? error.message
- : t('embeddingFilterDebug.message.unknownError');
+ const axiosError = error as AxiosError<{ msg?: string }>;
+ const serverMsg = axiosError?.response?.data?.msg;
+ if (typeof serverMsg === 'string' && serverMsg.trim()) {
+ return serverMsg;
+ }
+ return error instanceof Error
+ ? error.message
+ : t('embeddingFilterDebug.message.unknownError');
}📝 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
| function getErrorMessage(error: unknown) { | |
| return error instanceof Error | |
| ? error.message | |
| : t('embeddingFilterDebug.message.unknownError'); | |
| <script lang="ts" setup> | |
| import type { AxiosError } from 'axios'; | |
| import { computed, reactive, ref } from 'vue'; | |
| import { Message } from '`@arco-design/web-vue`'; | |
| // ... rest of imports | |
| function getErrorMessage(error: unknown) { | |
| const axiosError = error as AxiosError<{ msg?: string }>; | |
| const serverMsg = axiosError?.response?.data?.msg; | |
| if (typeof serverMsg === 'string' && serverMsg.trim()) { | |
| return serverMsg; | |
| } | |
| return error instanceof Error | |
| ? error.message | |
| : t('embeddingFilterDebug.message.unknownError'); | |
| } |
🤖 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/embedding_filter_debug/index.vue` around lines
230 - 233, getErrorMessage currently returns only Error.message and loses
server-provided messages from axios responses; update it to detect axios errors
and prefer error.response?.data?.msg when present, falling back to error.message
or t('embeddingFilterDebug.message.unknownError') otherwise. Specifically,
modify getErrorMessage to: (1) accept the unknown error, (2) if it's an axios
error (use axios.isAxiosError or check for error.response), extract and return
response.data.msg (or response.data?.msg) when defined, (3) else if error is
instance of Error return error.message, (4) otherwise return the existing
t('embeddingFilterDebug.message.unknownError'). Ensure this change covers the
callers testFilter and saveAtomCraft so server messages surface in those error
handlers.
Co-authored-by: Colin <Colin_XKL@outlook.com>
Co-authored-by: Colin <Colin_XKL@outlook.com>
Co-authored-by: Colin <Colin_XKL@outlook.com>
|
Deployment failed with the following error: Learn More: https://vercel.com/colinxkls-projects?upgradeToPro=build-rate-limit |
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
embedding-filterconfiguration behavior so embedding endpoint settings (type/base/key) fall back to commonFC_LLM_API_TYPE/BASE/KEYonly when no embedding endpoint is configured.FC_LLM_API_MODEL, usingFC_EMBEDDING_API_MODELor the OpenAI embedding default where applicable, and rejects comma-separated embedding model names.FC_EMBEDDING_MAX_INPUT_CHARSas a final per-text input cap before calling the embedding provider, including any instruction prefix.embedding-filterusage, environment variable fallback rules, and input length handling across English, Simplified Chinese, and Traditional Chinese.devand resolves conflicts ininternal/controller/dependency_monitor_test.goandinternal/controller/feed_viewer_test.go, preserving both browser-provider tests fromdevand embedding-filter tests from this branch.Walkthrough
embedding_dependency_health_check.mp4
embedding_filter_debug_page_hardened_flow.mp4
Testing
go test ./internal/controllergo test ./...cd web/admin && pnpm run type:checkgo test ./internal/adapter ./internal/craft ./internal/controller,task fix,task backend-build,task --force frontend-build,cd doc-site && pnpm install --frozen-lockfile && pnpm build.Embedding Serviceas Configured, manualCheck Healthchanged it to Healthy with latency.ai-debug-page-2, and verified the craft endpoint keepsAI breakthroughwhile filtering outFootball match.To show artifacts inline, enable in settings.
Summary by Sourcery
Harden embedding filter configuration and validation across backend and admin UI.
New Features:
Bug Fixes:
Enhancements:
Tests: