Feat: artifact compilation pipeline + shared knowlege_compile engines - #15697
Closed
KevinHuSh wants to merge 38 commits into
Closed
Feat: artifact compilation pipeline + shared knowlege_compile engines#15697KevinHuSh wants to merge 38 commits into
KevinHuSh wants to merge 38 commits into
Conversation
ASYNC109 flags async functions that accept a `timeout` parameter (callers should compose with `asyncio.timeout`/`wait_for` instead). Match the existing convention in agent/tools/base.py and rename `FunctionToolSession.tool_call_async`'s `timeout` to `request_timeout`. The sync `tool_call` wrapper keeps `timeout` (ASYNC109 only applies to async defs) and forwards via keyword. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Adds a configurable structured-knowledge extraction pipeline driven by the YAML schema delivered through document.parser_config (mirrors the Hyper-Extract template convention: type, output, guideline, identifiers). Key pieces in rag/prompts/generator.py: - compile_structure_from_text(chunks, parser_config, chat_mdl, embd_mdl, doc_id, language, callback, max_workers) — the public entry point. - Prompt builders for list/set (single-stage) and hypergraph (two-stage node-first then edge with known-entity context), patterned after hyperextract/utils/template_engine/parsers/guideline.py but without any langchain dependency. Multilingual str|list|dict values are localized inline, and observation_time is substituted into time rules. - Per-batch sequential node→edge ordering; batches themselves run in parallel under an asyncio.Semaphore (max_workers, default 10) with the same cancel-on-error gather pattern used by run_toc_from_text. - split_chunks packs chunk texts to (chat_mdl.max_length * INPUT_UTILIZATION - prompt_overhead) tokens per batch. - Extraction goes through gen_json (no Pydantic / structured output required). Embeddings go through LLMBundle.encode via thread_pool_exec. - Cross-chunk merging is intentionally not implemented; deferred to a later pipeline stage. ES doc shape emitted per item: content_with_weight, compile_kwd (list|set|hypergraph), structure_kwd (entity|relation), doc_id, chunk_ids, content_ltks/content_sm_ltks (from payload.description), title_tks (concat of tokenized non-description fields), q_<dim>_vec, src_name_kwd/target_name_kwd (relations, when resolvable via identifiers.relation_members or default source/target fields), id = xxh64(content_with_weight + doc_id). rag/svr/task_executor.py wires the new function in via a knowledge_compilation() helper next to build_TOC(). The call site is left commented out behind the existing toc_extraction flag — activation will be wired through its own dedicated flag in a follow-up.
….flow.extractor
- Move compile_structure_from_text and the supporting _struct_* helpers
from rag/prompts/generator.py into rag/flow/extractor/extractor.py so
the structured-knowledge pipeline lives alongside its caller. The
generator module now re-exports nothing new; existing run_toc_from_text /
gen_json / split_chunks / INPUT_UTILIZATION are imported from there.
- Add merge_compiled_structures(docs, chat_mdl, embd_mdl, tenant_id, kb_id,
similarity_threshold=0.9) which is meant to run on the docs returned by
compile_structure_from_text before they are written to ES:
Phase 1 (local dedup): group docs by (doc_id, compile_kwd,
src_name_kwd?, target_name_kwd?), compute pairwise cosine similarity
over q_<dim>_vec via sklearn.metrics.pairwise.cosine_similarity, and
for each pair above the threshold ask the LLM via _struct_merge_pair
whether they are the same logical entity/relation. On a duplicate
verdict the surviving entry is rebuilt from the merged payload:
chunk_ids are unioned, the description is re-embedded, src/target
are forced back to the existing payload's values for relations, and
the kept entry's id and identity kwds are preserved.
Phase 2 (ES dedup): for each surviving doc, build the same filter
condition and KNN-search ES via MatchDenseExpr (topn=1, similarity >=
threshold). On a hit + duplicate verdict, update the existing ES doc
by its old id via settings.docStoreConn.update; otherwise insert via
settings.docStoreConn.insert. Returns
{"inserted": N, "updated": M, "duplicates_dropped": K}.
- Merge prompts: MERGE_SYSTEM_PROMPT and MERGE_USER_PROMPT are kept
verbatim; a small MERGE_DECISION_INSTRUCTION is appended so the LLM also
emits {"duplicated": bool, "merged": <json|null>} via gen_json,
preserving the user-supplied prompts untouched.
- task_executor.py: import compile_structure_from_text and the new
merge_compiled_structures from rag.flow.extractor.extractor (no longer
rag.prompts.generator). knowledge_compilation now reads the structure
YAML from parser_config["knowledge_compilation"], runs the compile step,
and then calls merge_compiled_structures. The previously dead toc-style
branch is replaced by a dedicated kc_thread gated on
parser_config["knowledge_compilation"] so the new pipeline is opt-in
via its own flag.
…res into rag.advanced_rag.knowlege_compile.structure
The list/set/hypergraph compilation pipeline and its local-plus-ES
deduplicator no longer live alongside the workflow Extractor component
in rag/flow/extractor/extractor.py. They are now in a dedicated package:
rag/advanced_rag/knowlege_compile/
__init__.py — re-exports the two public entry points
structure.py — all _struct_* helpers, MERGE_* prompts,
compile_structure_from_text, and
merge_compiled_structures
No behavior changes: helpers, prompts, and function signatures move
verbatim. Two existing call sites are updated:
- rag/flow/extractor/extractor.py now imports the two entry points
from rag.advanced_rag.knowlege_compile.structure; only the
Extractor / ExtractorParam classes and run_toc_from_text remain in
this file.
- rag/svr/task_executor.py's knowledge_compilation helper imports
from the new package as well.
…/compile-structure-from-text
Adds rag/advanced_rag/knowlege_compile/_common.py and migrates both
pipelines (compile_structure + the four-phase MRP pipeline) onto a small
set of shared utilities and engines:
- ID minting (stable_row_id), tokenize_for_search, ordered union,
token-budget calculator (make_input_budget), defensive LLMBundle
unwrap (ensure_llm_bundle), encode wrapper, ES-IO wrappers
(es_search / es_insert / es_delete / es_upsert_one).
- build_chunk_batches + run_chunked_pipeline: shared chunked-LLM
scaffold (filter empties + resume + pack via split_chunks + parallel
asyncio.gather under a semaphore). Used by both MAP entry points;
the per-batch LLM call shape stays in each pipeline via a callback.
- bulk_dedup_items: exact + embedding + LLM-disambiguation dedup with
parameterised name/type keys, optional aggregate_extra callback.
Replaces the artifact pipeline's _wiki_exact_dedup_entities /
_wiki_exact_dedup_concepts / _wiki_embedding_dedup_entities /
_wiki_resolve_ambiguous_entities / _wiki_apply_merges helpers.
structure.py and the (formerly) wiki pipeline both consume those
helpers; the duplicated plumbing they used to carry shrinks by ~300
lines across the two files.
Renames the wiki module to "artifact" with case-preserving edits
(wiki -> artifact, Wiki -> Artifact, WIKI -> ARTIFACT) across:
- rag/advanced_rag/knowlege_compile/wiki.py -> artifact.py (git mv;
history follows the file)
- rag/advanced_rag/knowlege_compile/_common.py (docstrings/comments)
- rag/advanced_rag/knowlege_compile/__init__.py (re-exports)
- rag/svr/task_executor_refactor/task_handler.py (import + method
name + log strings)
Public entry points: artifact_map_from_chunks,
artifact_reduce_from_extracts, artifact_plan_from_reduction,
artifact_refine_from_plan. Constants: ARTIFACT_*_COMPILE_KWD.
compile_kwd string values, keyword-field names (artifact_slug_kwd,
artifact_kb_id_kwd, ...), and the clickable-link prefix
"artifact/{kb_id}/{slug}" are renamed in step. Existing rows persisted
under the old "wiki_*" compile_kwd values are no longer reachable from
the new code path; the task-handler's artifact_compilation entry
deletes the old kwds at the top of each run so the next pipeline
invocation produces fresh, artifact-keyed data.
Additional fixes folded in:
- Per-batch positional chunk labels (C1, C2, ...) and a known-id
scrubber on chunk bodies so the extraction LLM does not surface
chunk-id hashes as entity names.
- Stronger claim-extraction rules in the MAP prompt, plus an
entity/concept fallback for evidence so source_chunk_ids /
source_doc_ids populate even when MAP produces no claims.
- Synthetic fallback evidence stubs marked with _synthetic so they
carry chunk_ids forward without appearing in the writer prompt.
- Pages-spec dedup in REFINE so duplicate slugs from the planner do
not multiply writer calls or bloat the "Available pages" list.
- DEFAULT_ARTIFACT_PLAN_TIMEOUT raised to 600s for reasoning models.
- _ensure_llm_bundle defensive unwrap at the entry of REFINE (and now
helpful when the same misuse hits REDUCE/PLAN; the centralised
encode keeps tuple-shaped misuse from crashing deep in the stack).
- artifact_compilation in task_handler.py moved to AFTER chunk
insertion so REFINE's chunk-by-id lookup actually finds the source
rows in the doc store.
- _wiki_load_chunks_by_id falls back to per-id docStoreConn.get for
whatever the batch search misses, with diagnostic warnings so future
cross-backend filter quirks are debuggable.
MAP entity & relation schemas plus rule sections are now driven by
parser_config (the same YAML shape compile_structure_from_text
accepts); source_chunk_id is always appended so chunk attribution
survives whatever the user defines.
dosubot
Bot
added
size:XXL
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds compilation templates, structured knowledge compilation, artifact generation, and matching web APIs and pages. ChangesStructured Knowledge Extraction Pipeline
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
rag/advanced_rag/knowlege_compile/__init__.py (1)
17-43: ⚡ Quick winConsider adding a module-level docstring.
This package exposes a public API for structured knowledge extraction and artifact compilation. A module-level docstring would help users understand the package's purpose and the relationship between the exported functions (e.g., the MAP→REDUCE→PLAN→REFINE pipeline flow).
📝 Suggested docstring
# limitations under the License. # +"""Structured knowledge extraction and artifact compilation. + +This package provides two primary workflows: + +1. **Field-level extraction** (via ``compile_structure_from_text`` and + ``merge_compiled_structures``): Extract and deduplicate structured data + (lists, sets, hypergraphs) from document chunks. + +2. **KB-wide artifact compilation** (4-phase pipeline): + - MAP (``artifact_map_from_chunks``): Per-chunk entity/relation extraction + - REDUCE (``artifact_reduce_from_extracts``): KB-scoped deduplication + - PLAN (``artifact_plan_from_reduction``): Generate artifact pages outline + - REFINE (``artifact_refine_from_plan``): Produce searchable artifact pages +""" from .structure import compile_structure_from_text, merge_compiled_structures🤖 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 `@rag/advanced_rag/knowlege_compile/__init__.py` around lines 17 - 43, Add a clear module-level docstring at the top of this package explaining its purpose (structured knowledge extraction and artifact compilation) and the typical MAP→REDUCE→PLAN→REFINE pipeline flow; mention the primary API symbols to orient users: compile_structure_from_text, merge_compiled_structures, artifact_map_from_chunks, artifact_reduce_from_extracts, artifact_plan_from_reduction, artifact_refine_from_plan and the ARTIFACT_* compile keyword constants (ARTIFACT_MAP_COMPILE_KWD, ARTIFACT_REDUCE_COMPILE_KWD, ARTIFACT_PLAN_COMPILE_KWD, ARTIFACT_PAGE_COMPILE_KWD, ARTIFACT_DRAFT_COMPILE_KWD); keep it short (2–4 sentences) and place it as the first statement in the module so tools and IDEs surface it.api/db/services/dialog_service.py (1)
756-760: ⚡ Quick winAdd logging for the new empty_response streaming flow.
The empty_response path now emits an intermediate non-final yield (line 758) before the final yield. Per coding guidelines, new flows in
**/*.pyshould include logging. Consider adding a debug or info log when entering this path to aid troubleshooting.As per coding guidelines: "
**/*.py: Add logging for new flows".📝 Suggested logging addition
if not knowledges and prompt_config.get("empty_response"): empty_res = prompt_config["empty_response"] + logging.debug("Emitting empty_response for query '%s': no knowledges retrieved", " ".join(questions)) yield {"answer": empty_res, "reference": {}, "prompt": "", "audio_binary": None, "final": False} yield {"answer": empty_res, "reference": kbinfos, "prompt": "\n\n### Query:\n%s" % " ".join(questions), "audio_binary": tts(tts_mdl, empty_res), "final": True} return🤖 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 `@api/db/services/dialog_service.py` around lines 756 - 760, Add a log entry when the empty_response streaming branch is taken: inside the block that checks "if not knowledges and prompt_config.get('empty_response')" (where you yield the intermediate and final empty_responses and call tts(tts_mdl,...)), call the module logger (e.g., logger.debug or logger.info) to record that the empty_response flow was entered and include contextual data such as prompt_config keys, questions, and kbinfos identifiers; place the log before the first yield so the event is recorded for troubleshooting and follow existing logging conventions used in dialog_service.py.
🤖 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 `@rag/advanced_rag/knowlege_compile/structure.py`:
- Around line 551-589: The code calls _build_chunk_batches and
_run_chunked_pipeline in compile_structure but they are not imported, causing a
NameError; add imports for _build_chunk_batches and _run_chunked_pipeline from
the module that exports them (the same _common where encode, find_vec_field,
stable_row_id, tokenize_for_search, union_ordered are imported) so the functions
used in this file (references: _build_chunk_batches and _run_chunked_pipeline)
are available at runtime.
In `@rag/flow/extractor/extractor.py`:
- Around line 81-98: The async method _knowledge_compile incorrectly calls
merge_compiled_structures without awaiting it and also sends a misleading
callback message; change the callback text in _knowledge_compile to reflect
"Start knowledge compilation ..." and add await before
merge_compiled_structures(...) so the coroutine runs (ensure you keep existing
args: docs, self.chat_mdl, embedding_model, self._canvas.get_tenant_id(),
DocumentService.get_knowledgebase_id(self._canvas._doc_id)); verify
compile_structure_from_text is still awaited as-is.
In `@rag/svr/task_executor_refactor/task_handler.py`:
- Around line 575-581: The code is deleting the same five compile_kwd entries
twice; remove the duplicate set so each marker is deleted only once—either
delete the explicit five individual calls or remove the for-loop that repeats
them. Locate the block using settings.docStoreConn.delete and
search.index_name(ctx.tenant_id) (referencing ctx.kb_id and the kwd values
"artifact_map_extract", "artifact_reduce_result", "artifact_compilation_plan",
"artifact_page_draft", "artifact_page") and keep just a single deletion
implementation (preferably the concise for kwd in (...) loop) and remove the
other set.
- Around line 650-654: Inside the async method _artifact_compilation, don't call
asyncio.run(_run()) (which raises RuntimeError when the event loop is already
running); instead await the coroutine returned by _run() (i.e., pages = await
_run()), and keep the existing try/except around that await so the
logging.exception("artifact_compilation: pipeline failed for doc %s",
ctx.doc_id) path still runs on errors—update the call site in
_artifact_compilation to use await _run() and preserve error handling.
---
Nitpick comments:
In `@api/db/services/dialog_service.py`:
- Around line 756-760: Add a log entry when the empty_response streaming branch
is taken: inside the block that checks "if not knowledges and
prompt_config.get('empty_response')" (where you yield the intermediate and final
empty_responses and call tts(tts_mdl,...)), call the module logger (e.g.,
logger.debug or logger.info) to record that the empty_response flow was entered
and include contextual data such as prompt_config keys, questions, and kbinfos
identifiers; place the log before the first yield so the event is recorded for
troubleshooting and follow existing logging conventions used in
dialog_service.py.
In `@rag/advanced_rag/knowlege_compile/__init__.py`:
- Around line 17-43: Add a clear module-level docstring at the top of this
package explaining its purpose (structured knowledge extraction and artifact
compilation) and the typical MAP→REDUCE→PLAN→REFINE pipeline flow; mention the
primary API symbols to orient users: compile_structure_from_text,
merge_compiled_structures, artifact_map_from_chunks,
artifact_reduce_from_extracts, artifact_plan_from_reduction,
artifact_refine_from_plan and the ARTIFACT_* compile keyword constants
(ARTIFACT_MAP_COMPILE_KWD, ARTIFACT_REDUCE_COMPILE_KWD,
ARTIFACT_PLAN_COMPILE_KWD, ARTIFACT_PAGE_COMPILE_KWD,
ARTIFACT_DRAFT_COMPILE_KWD); keep it short (2–4 sentences) and place it as the
first statement in the module so tools and IDEs surface it.
🪄 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: 39e42760-70bc-4298-9095-0d0a88e66e57
📒 Files selected for processing (9)
api/db/services/dialog_service.pyrag/advanced_rag/knowlege_compile/__init__.pyrag/advanced_rag/knowlege_compile/_common.pyrag/advanced_rag/knowlege_compile/artifact.pyrag/advanced_rag/knowlege_compile/structure.pyrag/flow/extractor/extractor.pyrag/prompts/generator.pyrag/svr/task_executor.pyrag/svr/task_executor_refactor/task_handler.py
Comment on lines
+81
to
+98
| async def _knowledge_compile(self, docs): | ||
| embedding_model = LLMBundle(self._canvas.get_tenant_id(), LLMType.EMBEDDING, | ||
| max_retries=self._param.max_retries, | ||
| retry_interval=self._param.delay_after_error) | ||
| self.callback(0.2,message="Start to generate table of content ...") | ||
| docs = sorted(docs, key=lambda d:( | ||
| d.get("page_num_int", 0)[0] if isinstance(d.get("page_num_int", 0), list) else d.get("page_num_int", 0), | ||
| d.get("top_int", 0)[0] if isinstance(d.get("top_int", 0), list) else d.get("top_int", 0) | ||
| )) | ||
| docs = await compile_structure_from_text(docs, | ||
| self._param.knowledge_compilation, | ||
| self.chat_mdl, embedding_model, | ||
| self._canvas._doc_id) | ||
| info = merge_compiled_structures(docs, self.chat_mdl, | ||
| embedding_model, | ||
| self._canvas.get_tenant_id(), | ||
| DocumentService.get_knowledgebase_id(self._canvas._doc_id)) | ||
| return info |
Contributor
There was a problem hiding this comment.
Missing await on async function call causes merge to never execute.
merge_compiled_structures is an async function, but line 94 does not await it. This returns a coroutine object instead of executing the merge, meaning deduplication will silently be skipped.
Also, the callback message on line 85 says "table of content" but should reflect knowledge compilation.
🐛 Proposed fix
async def _knowledge_compile(self, docs):
embedding_model = LLMBundle(self._canvas.get_tenant_id(), LLMType.EMBEDDING,
max_retries=self._param.max_retries,
retry_interval=self._param.delay_after_error)
- self.callback(0.2,message="Start to generate table of content ...")
+ self.callback(0.2, message="Start knowledge compilation...")
docs = sorted(docs, key=lambda d:(
d.get("page_num_int", 0)[0] if isinstance(d.get("page_num_int", 0), list) else d.get("page_num_int", 0),
d.get("top_int", 0)[0] if isinstance(d.get("top_int", 0), list) else d.get("top_int", 0)
))
docs = await compile_structure_from_text(docs,
self._param.knowledge_compilation,
self.chat_mdl, embedding_model,
self._canvas._doc_id)
- info = merge_compiled_structures(docs, self.chat_mdl,
+ info = await merge_compiled_structures(docs, self.chat_mdl,
embedding_model,
self._canvas.get_tenant_id(),
DocumentService.get_knowledgebase_id(self._canvas._doc_id))
return info🤖 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 `@rag/flow/extractor/extractor.py` around lines 81 - 98, The async method
_knowledge_compile incorrectly calls merge_compiled_structures without awaiting
it and also sends a misleading callback message; change the callback text in
_knowledge_compile to reflect "Start knowledge compilation ..." and add await
before merge_compiled_structures(...) so the coroutine runs (ensure you keep
existing args: docs, self.chat_mdl, embedding_model,
self._canvas.get_tenant_id(),
DocumentService.get_knowledgebase_id(self._canvas._doc_id)); verify
compile_structure_from_text is still awaited as-is.
…taset-scope Artifact tab Adds a configurable compilation pipeline whose template (kind + entity + relation + global rules) is authored under /knowledge-compilation and attached to a document via parser_config.compilation_template_id. - Document-scope kinds (timeline / page_index / knowledge_graph / empty) run at the tail of _run_standard_chunking and merge within one doc. - Dataset-scope kind (artifacts) is triggered KB-wide by a new "Artifact" generate button and a task_type="artifact" task that runs MAP per eligible doc with batch_size_cap=8 / window_fraction=0.5, then REDUCE/PLAN/REFINE/persist KB-wide. PIPELINE_SPECIAL_PROGRESS_FREEZE set extended so TaskContext picks up tenant_id for fan-out tasks. Surfaces results via a new dataset Artifact tab: paginated page list ordered by outlinks_int, side-by-side markdown viewer with intra-KB artifactlink interception and See-also chips, and an entity graph reusing the existing antv/g6 ForceGraph renderer. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
rag/svr/task_executor.py (1)
134-141:⚠️ Potential issue | 🟠 Major | ⚡ Quick winArtifact tasks are registered but never handled in this executor path.
Line 140 adds
"artifact"to the pipeline-task mapping, butdo_handle_task()has notask_type == "artifact"branch. In the dry-run/original paths at Lines 1722-1739, artifact work still goes throughdo_handle_task(), so it falls into the standard chunking branch instead of the KB-wide artifact pipeline.Also applies to: 1426-1545, 1722-1739
🤖 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 `@rag/svr/task_executor.py` around lines 134 - 141, TASK_TYPE_TO_PIPELINE_TASK_TYPE includes "artifact" but do_handle_task() lacks a task_type == "artifact" branch, so artifact work falls through into the generic chunking path; add an explicit branch in do_handle_task() that detects task_type == "artifact" and routes to the KB-wide artifact pipeline handler (call the existing artifact pipeline function or implement a new handle_artifact_task/run_artifact_pipeline function and invoke it), and mirror the same routing in the dry-run/original execution paths so artifact tasks use the artifact pipeline rather than the standard chunking flow; reference TASK_TYPE_TO_PIPELINE_TASK_TYPE and do_handle_task() when making the change.api/db/services/document_service.py (1)
975-976:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winInclude artifact in the queued-progress suffix check.
Line 1074 now queues
"artifact"here, but Lines 975-976 still only append"... tasks are ahead in the queue"for graphrag/raptor/mindmap. Artifact jobs will keep the bare"created task artifact"message until some later progress update arrives.Also applies to: 1074-1074
🤖 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 `@api/db/services/document_service.py` around lines 975 - 976, The progress-message suffix check in document_service.py only matches "created task graphrag", "created task raptor", and "created task mindmap" so "created task artifact" never gets the queued-count suffix; update the condition that builds info["progress_msg"] (the msg.endswith(...) checks) to also include "created task artifact" (or better, replace the repeated ors with a tuple membership like msg.endswith(tuple_of_suffixes)) and keep using get_queue_length(priority) to append "\n%d tasks are ahead in the queue..." so artifact jobs receive the same queued-progress text.
🧹 Nitpick comments (8)
web/src/pages/user-setting/knowledge-compilation/index.tsx (1)
25-41: ⚡ Quick winInconsistent callback patterns for the same operation.
showEdit(lines 25-30) is a higher-order function that returns a callback, whilehandleEditFromCard(lines 38-41) directly performs the action. Both seteditingIdandeditVisibleto show the edit modal, but use different patterns:
- Line 76:
onClick={showEdit('')}calls the HOF and uses the returned function.- Line 88:
onEdit={handleEditFromCard}passes the direct action.Prefer one consistent pattern for clarity. Recommend converting
showEditto a direct action likehandleEditFromCardand wrapping it inline when needed:onClick={() => showEdit('')}.🤖 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/src/pages/user-setting/knowledge-compilation/index.tsx` around lines 25 - 41, The two edit handlers use inconsistent patterns: convert showEdit to a direct action (remove the higher-order function) so it mirrors handleEditFromCard by calling setEditingId and setEditVisible directly; update any callers that currently do onClick={showEdit('')} to use an inline wrapper like onClick={() => showEdit('')} (or pass the function reference for handlers expecting an id), and keep hideEdit unchanged — search for showEdit, handleEditFromCard, setEditingId, and setEditVisible to make the consistent change.web/src/services/compilation-template-service.ts (1)
10-13: ⚡ Quick winUse typed request interfaces instead of
Record<string, any>.The
createandupdatemethods use looseRecord<string, any>typing, while proper request interfaces (ICreateCompilationTemplateRequest,IUpdateCompilationTemplateRequest) are already defined in the imports. Using these types would provide better type safety and IDE support.🔧 Suggested type improvement
+import { + ICreateCompilationTemplateRequest, + IListCompilationTemplatesRequest, + IUpdateCompilationTemplateRequest, +} from '`@/interfaces/request/compilation-template`'; -import { IListCompilationTemplatesRequest } from '`@/interfaces/request/compilation-template`'; import api from '`@/utils/api`'; import request from '`@/utils/request`'; const compilationTemplateService = { list: (params?: IListCompilationTemplatesRequest) => request.get(api.listCompilationTemplates, { params }), get: (params: { id: string }) => request.get(api.getCompilationTemplate(params.id)), - create: (params?: Record<string, any>) => + create: (params?: ICreateCompilationTemplateRequest) => request.post(api.createCompilationTemplate, { data: params }), - update: ({ id, ...params }: Record<string, any>) => + update: ({ id, ...params }: IUpdateCompilationTemplateRequest) => request.put(api.updateCompilationTemplate(id), { data: params }), delete: ({ id }: { id: string }) => request.delete(api.deleteCompilationTemplate(id)), builtins: () => request.get(api.listBuiltinCompilationTemplates), };🤖 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/src/services/compilation-template-service.ts` around lines 10 - 13, The create and update methods use loose Record<string, any> types; change their parameter typings to the specific request interfaces: use ICreateCompilationTemplateRequest for create(params) and IUpdateCompilationTemplateRequest for update({ id, ...params }) so the calls to request.post(api.createCompilationTemplate, { data: params }) and request.put(api.updateCompilationTemplate(id), { data: params }) get proper type safety and IDE autocompletion.web/src/components/chunk-method-dialog/index.tsx (1)
69-72: ⚡ Quick winUse
useTranslationfrom react-i18next instead ofuseTranslate.Line 71 uses the project-wrapped
useTranslateutility. Per coding guidelines, preferuseTranslationfromreact-i18nextfor i18n in React components.♻️ Proposed refactor
-import { useTranslate } from '`@/hooks/common-hooks`'; +import { useTranslation } from 'react-i18next'; function KnowledgeCompilationTemplateSelect() { const form = useFormContext(); - const { t } = useTranslate('knowledgeConfiguration'); + const { t } = useTranslation('translation', { keyPrefix: 'knowledgeConfiguration' });As per coding guidelines: "Prefer
useTranslationfromreact-i18nextover project-wrapped utilities likeuseTranslatefor i18n in React components."🤖 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/src/components/chunk-method-dialog/index.tsx` around lines 69 - 72, The component KnowledgeCompilationTemplateSelect is importing/using the project utility useTranslate; replace that with react-i18next's useTranslation: remove useTranslate usage, import useTranslation from 'react-i18next', call const { t } = useTranslation('knowledgeConfiguration') inside KnowledgeCompilationTemplateSelect and ensure all existing t(...) usages remain compatible; keep other hooks (useFormContext, useFetchSavedCompilationTemplates) unchanged.Source: Coding guidelines
web/src/pages/user-setting/knowledge-compilation/edit-template-form.tsx (1)
150-150: 💤 Low valuePrefer a named constant for the name field maxLength.
Line 150 uses a hardcoded
maxLength={128}, while line 173 uses theTEXT_FIELD_MAXconstant for the description field. Extract128into a named constant (e.g.,NAME_FIELD_MAX) for consistency and maintainability.♻️ Proposed refactor
In
interface.ts, add:+export const NAME_FIELD_MAX = 128; export const TEXT_FIELD_MAX = 512;Then in this file:
-import { TEXT_FIELD_MAX } from './interface'; +import { NAME_FIELD_MAX, TEXT_FIELD_MAX } from './interface'; // ... - <Input {...field} maxLength={128} /> + <Input {...field} maxLength={NAME_FIELD_MAX} />🤖 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/src/pages/user-setting/knowledge-compilation/edit-template-form.tsx` at line 150, Replace the hardcoded maxLength on the template name field with a named constant: add a NAME_FIELD_MAX constant (e.g., 128) next to TEXT_FIELD_MAX in interface.ts and export it, then import NAME_FIELD_MAX into edit-template-form.tsx and replace maxLength={128} on the name input with maxLength={NAME_FIELD_MAX} to match the description field pattern and keep consistency (refer to TEXT_FIELD_MAX, NAME_FIELD_MAX, and the name input in edit-template-form.tsx).web/src/pages/dataset/dataset/generate-button/hook.ts (1)
174-178: 💤 Low valueConsider replacing nested ternary with if-else for readability.
The nested ternary operator works correctly but reduces readability. An if-else chain or switch statement would be clearer for this three-way branch.
♻️ Proposed refactor
- const indexType = - type === GenerateType.KnowledgeGraph - ? 'graph' - : type === GenerateType.Artifact - ? 'artifact' - : 'raptor'; + let indexType: string; + if (type === GenerateType.KnowledgeGraph) { + indexType = 'graph'; + } else if (type === GenerateType.Artifact) { + indexType = 'artifact'; + } else { + indexType = 'raptor'; + }🤖 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/src/pages/dataset/dataset/generate-button/hook.ts` around lines 174 - 178, The nested ternary that maps the local variable type (of enum GenerateType) to the strings 'graph' | 'artifact' | 'raptor' is hard to read; replace it with a simple if-else chain or a switch inside the same scope (e.g., compute a const mode or target variable) that checks type === GenerateType.KnowledgeGraph, else if type === GenerateType.Artifact, else default to 'raptor', preserving the exact returned string values and behavior in the surrounding hook (look for the usage near the reference to GenerateType and the existing ternary).web/src/pages/dataset/dataset/generate-button/generate.tsx (1)
232-237: 💤 Low valueConsider replacing nested ternary with if-else for readability.
The nested ternary operator works correctly but reduces readability. A switch statement or if-else chain would be clearer for this three-way branch.
♻️ Proposed refactor
- const data = ( - name === GenerateType.KnowledgeGraph - ? graphRunData - : name === GenerateType.Artifact - ? artifactRunData - : raptorRunData - ) as ITraceInfo; + let data: ITraceInfo; + if (name === GenerateType.KnowledgeGraph) { + data = graphRunData as ITraceInfo; + } else if (name === GenerateType.Artifact) { + data = artifactRunData as ITraceInfo; + } else { + data = raptorRunData as ITraceInfo; + }🤖 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/src/pages/dataset/dataset/generate-button/generate.tsx` around lines 232 - 237, Replace the nested ternary used to select trace data with a clear if-else (or switch) branch: locate the expression that casts the result to ITraceInfo where GenerateType is compared against GenerateType.KnowledgeGraph and GenerateType.Artifact and currently returns graphRunData, artifactRunData, or raptorRunData; change it to an if (name === GenerateType.KnowledgeGraph) { selected = graphRunData } else if (name === GenerateType.Artifact) { selected = artifactRunData } else { selected = raptorRunData } and use selected as the ITraceInfo to improve readability.api/apps/restful_apis/chunk_api.py (1)
503-585: ⚡ Quick winAdd logging for the new endpoint.
Per coding guidelines, new flows should include logging. This endpoint has no logging for debugging graph reconstruction or cache hits/misses.
♻️ Suggested logging additions
+ logging.info("get_document_structure_graph: dataset=%s doc=%s kind=%s", dataset_id, document_id, kind) index_name = search.index_name(dataset_tenant_id) fields = ["content_with_weight"] ... if rows: row = next(iter(rows.values())) graph = json.loads(row.get("content_with_weight") or "{}") if isinstance(graph, dict): + logging.debug("get_document_structure_graph: cache hit for doc=%s kind=%s", document_id, kind) return get_result(data={ "entities": graph.get("entities") or [], "relations": graph.get("relations") or [], }) + logging.debug("get_document_structure_graph: rebuilding graph for doc=%s kind=%s", document_id, kind) graph = await rebuild_structure_graph_json(dataset_tenant_id, dataset_id, document_id, kind)🤖 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 `@api/apps/restful_apis/chunk_api.py` around lines 503 - 585, The new endpoint get_document_structure_graph is missing logs for request entry, cache/search hit or miss, start/end of graph reconstruction and errors; add structured logging calls (including tenant_id, dataset_id, document_id, kind) at the start of get_document_structure_graph, before/after the thread_pool_exec/settings.docStoreConn.search call to record cache hit/miss and returned rows, and around the call to rebuild_structure_graph_json to mark reconstruction start/finish and duration; also log exceptions in the except block before returning server_error_response so failures are visible.Source: Coding guidelines
rag/svr/task_executor_refactor/task_handler.py (1)
84-90: ⚡ Quick winDuplicated
_compilation_template_kindhelper inrag/svr/task_executor_refactor/task_handler.pyandapi/apps/restful_apis/chunk_api.py.Both files define identical
_compilation_template_kindfunctions that normalize template kind strings. Extract this to a shared location (e.g.,api/db/services/compilation_template_service.py) to avoid divergence and reduce maintenance burden.🤖 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 `@rag/svr/task_executor_refactor/task_handler.py` around lines 84 - 90, Duplicate helper _compilation_template_kind is present in task_handler.py and chunk_api.py; extract it into a shared module named compilation_template_service (e.g., add a function normalize_compilation_template_kind in compilation_template_service.py), update both callers to import and call compilation_template_service.normalize_compilation_template_kind (or keep original function name if preferred), remove the duplicate definitions from task_handler.py and chunk_api.py, and run tests to ensure imports and references (places using _compilation_template_kind) are updated accordingly.
🤖 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 `@api/apps/restful_apis/compilation_template_api.py`:
- Around line 179-205: In update(), _validate_template_payload is being called
only on the raw req so artifact-specific checks are skipped for partial updates;
fix by merging req with the existing template before validation (e.g. load
existing via CompilationTemplateService.get_saved(template_id, current_user.id)
and build merged_payload = {**existing_as_dict, **req}) and then call
_validate_template_payload(merged_payload, require_all=False), or alternatively
call _validate_template_payload(req, require_all=False,
existing_kind=existing.kind) if the validator supports a kind param; ensure the
validation happens before the duplicate-name check and before saving via
CompilationTemplateService.filter_update.
- Around line 102-103: Replace uses of current_user.id with the tenant id when
calling CompilationTemplateService methods: e.g. change
CompilationTemplateService.list_saved(current_user.id, ...) to pass the tenant
id (current_user.tenant_id or a tenant_id pulled from request context) and do
the same for other service calls such as any
get_saved/create_saved/update_saved/delete_saved usages in this module; thread
tenant_id through the endpoint handlers the same way other dataset APIs do so
the CompilationTemplate.tenant_id is consistently set and queried by tenant, not
user id, and keep the existing Pydantic request validation / service-layer call
pattern intact.
- Around line 91-228: The handlers use the decorator manager.route (e.g., on
functions list_templates, list_builtin_templates, detail, create, update,
delete) but manager is not defined or imported; fix by importing or defining the
Flask app/Blueprint object named manager at the top of this module before any
route decorators (for example, import the existing manager Blueprint from the
module where it is declared or instantiate manager = Blueprint("manager",
__name__) if this file should own it), then re-run tests to ensure routes
register without NameError.
In `@api/apps/restful_apis/document_api.py`:
- Around line 240-248: The current comparison uses old_parser_config =
dict(req["parser_config"] or {}) and then mutates req["parser_config"], which
compares the request payload to itself instead of the persisted document config;
change the code to read the stored config from the document (doc.parser_config
or dict(doc.parser_config or {})) into old_parser_config, then update
req["parser_config"] with update_doc_req.parser_config.ext, call
_compilation_template_ids_changed(old_parser_config, req["parser_config"]) and,
if true, call reset_document_for_reparse(doc.id) (and keep
DocumentService.update_parser_config(doc.id, req["parser_config"]) as before);
apply the same fix for the block around lines 258-263.
In `@api/db/init_data/compilation_templates/timeline.yaml`:
- Around line 14-15: Update the timeline template rule that currently says "If
only a relative time (e.g., 'yesterday', 'next week'), convert to absolute when
the context allows, else keep as is" so it never invents absolute dates without
an explicit anchor: keep relative expressions verbatim unless the input chunk
contains an explicit anchor/absolute date; only perform conversion when a
concrete anchor date is present in the same source chunk or a validated metadata
field. Locate the rule text in compilation_templates/timeline.yaml (the line
mentioning "relative time" / examples "yesterday", "next week") and change
wording/logic to prohibit implicit date fabrication and require an explicit
anchor before producing absolute timestamps.
In `@rag/advanced_rag/knowlege_compile/structure.py`:
- Around line 86-92: _struct_normalize_kind currently collapses distinct kinds
("knowledge_graph", "page_index") into "timeline", causing different templates
to share the same compile_kwd and be mixed/overwritten; change
_struct_normalize_kind to only perform non-destructive normalization (strip,
lower, replace "-" with "_") and remove the special-case mapping to "timeline",
and update code paths that set the compile_kwd/inferred type to use this
lossless normalized value so each template kind remains unique.
In `@rag/svr/task_executor_refactor/task_handler.py`:
- Around line 677-681: The LLMBundle instance chat_mdl is created without using
a context manager, so it may not be cleaned up on exceptions; modify the
function to instantiate LLMBundle(ctx.tenant_id, chat_model_config,
lang=ctx.language) using a with statement (e.g., with LLMBundle(...) as
chat_mdl:) and move the remainder of the method logic that uses chat_mdl
(everything after the current creation — the code that runs the KB-level
task/pipeline) inside that with block so the bundle is properly closed; locate
the creation by the symbols chat_model_config, chat_mdl and LLMBundle to update
the scope.
- Around line 569-573: The code creates an LLMBundle instance (doc_task_llm_id
via ctx.parser_config/get_model_config_from_provider_instance and chat_mdl =
LLMBundle(...)) without a context manager; wrap the creation in a context
manager "with LLMBundle(ctx.tenant_id, chat_model_config, lang=ctx.language) as
chat_mdl:" and indent the rest of the loop/body that uses chat_mdl accordingly
so the bundle is properly closed; ensure any references to chat_mdl remain
inside the with-block and remove the standalone assignment to avoid resource
leakage.
In `@web/src/hooks/use-chunk-request.ts`:
- Around line 144-162: Create a query key factory (e.g.,
DocumentStructureGraphKeys) that returns an as-const tuple for the
fetchDocumentStructureGraph key and replace the inline queryKey array in
useFetchDocumentStructureGraph with a call to that factory (passing knowledgeId
and documentId). Update the useQuery invocation in
useFetchDocumentStructureGraph to use
DocumentStructureGraphKeys.fetch(knowledgeId, documentId) (or similar) and keep
the enabled, initialData (EMPTY_DOCUMENT_STRUCTURE_GRAPH), gcTime and queryFn
(kbService.getDocumentStructureGraph) behavior unchanged.
- Line 35: The frontend must not rename the existing misspelled field: keep
IDocumentStructureGraph.discription? unchanged (it’s consumed by backend
structure.py and graph UI) and do not change usages of entity.discription;
instead update the React Query usage in useFetchDocumentStructureGraph (and
other hooks in use-chunk-request.ts) to use the centralized query-key factory
(e.g., DocumentStructureGraphKeys or the project’s {Domain}Keys) rather than
inline arrays like ['fetchDocumentStructureGraph', knowledgeId, documentId], and
ensure you use the same keys for both useQuery and invalidateQueries so keys are
stable and reusable across the app.
In `@web/src/hooks/use-compilation-template-request.ts`:
- Around line 177-196: The mutation in useDeleteCompilationTemplate only calls
queryClient.invalidateQueries when all deletions succeed, leaving UI stale on
partial success; update mutationFn in useDeleteCompilationTemplate to always
update the cache after the API calls — either call
queryClient.invalidateQueries({ queryKey: CompilationTemplateKeys.all() })
unconditionally after Promise.all completes, or inspect results to build a list
of successfully deleted ids and use queryClient.setQueryData/patch to remove
those ids from the cached CompilationTemplateKeys.all() data (refer to
mutationFn and queryClient.invalidateQueries / queryClient.setQueryData).
In
`@web/src/pages/chunk/parsed-result/add-knowledge/components/knowledge-chunk/components/document-structure-graph.tsx`:
- Around line 17-18: The code currently falls back to the misspelled property
entity.discription when building the description field; after correcting the
IDocumentStructureGraph interface to use only description, remove the fallback
to entity.discription and use entity.description (and aliases fallback)
exclusively in the description assignment inside the component that references
entity (look for description: entity.discription || entity.description ||
entity.aliases?.join(', ')). Ensure no other references to entity.discription
remain.
In
`@web/src/pages/user-setting/knowledge-compilation/components/builtin-template-popover.tsx`:
- Around line 18-20: Update the header comment in builtin-template-popover.tsx
to match the implementation: it currently claims the list "doesn't refetch on
every open" but the component (BuiltinTemplatePopover) explicitly calls
refetch() when opening; either remove that misleading sentence or change it to
state that the list is cached via React Query but is explicitly refetched on
popover open (mentioning the refetch() call used when opening to make behavior
clear).
In `@web/vite.config.ts`:
- Around line 62-72: The proxy targets currently hardcode a developer LAN IP
(e.g., targets under '/api' and '/v1') which breaks other machines; update the
proxy configuration in vite.config.ts to read host/port from environment
variables (e.g., PROXY_HOST, PROXY_PORT or separate
PROXY_API_HOST/PROXY_V1_HOST) with sensible defaults to 'localhost' and default
ports, and ensure proxy prefixes use the stable project prefixes ('/v1',
'/api/v1'); adjust the code paths that construct the target (where proxyScheme
is used) to compose `${proxyScheme}://${process.env.PROXY_HOST ||
'localhost'}:${process.env.PROXY_PORT || '9382'}/` (or separate vars per prefix)
so startup no longer depends on 192.168.1.24.
---
Outside diff comments:
In `@api/db/services/document_service.py`:
- Around line 975-976: The progress-message suffix check in document_service.py
only matches "created task graphrag", "created task raptor", and "created task
mindmap" so "created task artifact" never gets the queued-count suffix; update
the condition that builds info["progress_msg"] (the msg.endswith(...) checks) to
also include "created task artifact" (or better, replace the repeated ors with a
tuple membership like msg.endswith(tuple_of_suffixes)) and keep using
get_queue_length(priority) to append "\n%d tasks are ahead in the queue..." so
artifact jobs receive the same queued-progress text.
In `@rag/svr/task_executor.py`:
- Around line 134-141: TASK_TYPE_TO_PIPELINE_TASK_TYPE includes "artifact" but
do_handle_task() lacks a task_type == "artifact" branch, so artifact work falls
through into the generic chunking path; add an explicit branch in
do_handle_task() that detects task_type == "artifact" and routes to the KB-wide
artifact pipeline handler (call the existing artifact pipeline function or
implement a new handle_artifact_task/run_artifact_pipeline function and invoke
it), and mirror the same routing in the dry-run/original execution paths so
artifact tasks use the artifact pipeline rather than the standard chunking flow;
reference TASK_TYPE_TO_PIPELINE_TASK_TYPE and do_handle_task() when making the
change.
---
Nitpick comments:
In `@api/apps/restful_apis/chunk_api.py`:
- Around line 503-585: The new endpoint get_document_structure_graph is missing
logs for request entry, cache/search hit or miss, start/end of graph
reconstruction and errors; add structured logging calls (including tenant_id,
dataset_id, document_id, kind) at the start of get_document_structure_graph,
before/after the thread_pool_exec/settings.docStoreConn.search call to record
cache hit/miss and returned rows, and around the call to
rebuild_structure_graph_json to mark reconstruction start/finish and duration;
also log exceptions in the except block before returning server_error_response
so failures are visible.
In `@rag/svr/task_executor_refactor/task_handler.py`:
- Around line 84-90: Duplicate helper _compilation_template_kind is present in
task_handler.py and chunk_api.py; extract it into a shared module named
compilation_template_service (e.g., add a function
normalize_compilation_template_kind in compilation_template_service.py), update
both callers to import and call
compilation_template_service.normalize_compilation_template_kind (or keep
original function name if preferred), remove the duplicate definitions from
task_handler.py and chunk_api.py, and run tests to ensure imports and references
(places using _compilation_template_kind) are updated accordingly.
In `@web/src/components/chunk-method-dialog/index.tsx`:
- Around line 69-72: The component KnowledgeCompilationTemplateSelect is
importing/using the project utility useTranslate; replace that with
react-i18next's useTranslation: remove useTranslate usage, import useTranslation
from 'react-i18next', call const { t } =
useTranslation('knowledgeConfiguration') inside
KnowledgeCompilationTemplateSelect and ensure all existing t(...) usages remain
compatible; keep other hooks (useFormContext, useFetchSavedCompilationTemplates)
unchanged.
In `@web/src/pages/dataset/dataset/generate-button/generate.tsx`:
- Around line 232-237: Replace the nested ternary used to select trace data with
a clear if-else (or switch) branch: locate the expression that casts the result
to ITraceInfo where GenerateType is compared against GenerateType.KnowledgeGraph
and GenerateType.Artifact and currently returns graphRunData, artifactRunData,
or raptorRunData; change it to an if (name === GenerateType.KnowledgeGraph) {
selected = graphRunData } else if (name === GenerateType.Artifact) { selected =
artifactRunData } else { selected = raptorRunData } and use selected as the
ITraceInfo to improve readability.
In `@web/src/pages/dataset/dataset/generate-button/hook.ts`:
- Around line 174-178: The nested ternary that maps the local variable type (of
enum GenerateType) to the strings 'graph' | 'artifact' | 'raptor' is hard to
read; replace it with a simple if-else chain or a switch inside the same scope
(e.g., compute a const mode or target variable) that checks type ===
GenerateType.KnowledgeGraph, else if type === GenerateType.Artifact, else
default to 'raptor', preserving the exact returned string values and behavior in
the surrounding hook (look for the usage near the reference to GenerateType and
the existing ternary).
In `@web/src/pages/user-setting/knowledge-compilation/edit-template-form.tsx`:
- Line 150: Replace the hardcoded maxLength on the template name field with a
named constant: add a NAME_FIELD_MAX constant (e.g., 128) next to TEXT_FIELD_MAX
in interface.ts and export it, then import NAME_FIELD_MAX into
edit-template-form.tsx and replace maxLength={128} on the name input with
maxLength={NAME_FIELD_MAX} to match the description field pattern and keep
consistency (refer to TEXT_FIELD_MAX, NAME_FIELD_MAX, and the name input in
edit-template-form.tsx).
In `@web/src/pages/user-setting/knowledge-compilation/index.tsx`:
- Around line 25-41: The two edit handlers use inconsistent patterns: convert
showEdit to a direct action (remove the higher-order function) so it mirrors
handleEditFromCard by calling setEditingId and setEditVisible directly; update
any callers that currently do onClick={showEdit('')} to use an inline wrapper
like onClick={() => showEdit('')} (or pass the function reference for handlers
expecting an id), and keep hideEdit unchanged — search for showEdit,
handleEditFromCard, setEditingId, and setEditVisible to make the consistent
change.
In `@web/src/services/compilation-template-service.ts`:
- Around line 10-13: The create and update methods use loose Record<string, any>
types; change their parameter typings to the specific request interfaces: use
ICreateCompilationTemplateRequest for create(params) and
IUpdateCompilationTemplateRequest for update({ id, ...params }) so the calls to
request.post(api.createCompilationTemplate, { data: params }) and
request.put(api.updateCompilationTemplate(id), { data: params }) get proper type
safety and IDE autocompletion.
🪄 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: a1ccc6b9-f100-4e32-9cae-a04a59b9cb4c
⛔ Files ignored due to path filters (1)
web/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (66)
api/apps/restful_apis/chunk_api.pyapi/apps/restful_apis/compilation_template_api.pyapi/apps/restful_apis/dataset_api.pyapi/apps/restful_apis/document_api.pyapi/apps/services/dataset_api_service.pyapi/apps/services/document_api_service.pyapi/db/__init__.pyapi/db/db_models.pyapi/db/init_data.pyapi/db/init_data/compilation_templates/artifacts.yamlapi/db/init_data/compilation_templates/empty.yamlapi/db/init_data/compilation_templates/knowledge_graph.yamlapi/db/init_data/compilation_templates/page_index.yamlapi/db/init_data/compilation_templates/timeline.yamlapi/db/services/compilation_template_service.pyapi/db/services/document_service.pycommon/constants.pyrag/advanced_rag/knowlege_compile/_common.pyrag/advanced_rag/knowlege_compile/artifact.pyrag/advanced_rag/knowlege_compile/structure.pyrag/nlp/search.pyrag/svr/task_executor.pyrag/svr/task_executor_refactor/task_handler.pyrag/utils/es_conn.pyweb/src/components/chunk-method-dialog/index.tsxweb/src/components/chunk-method-dialog/use-default-parser-values.tsweb/src/constants/setting.tsweb/src/hooks/use-chunk-request.tsweb/src/hooks/use-compilation-template-request.tsweb/src/hooks/use-dataset-artifact-request.tsweb/src/interfaces/database/compilation-template.tsweb/src/interfaces/database/dataset-artifact.tsweb/src/interfaces/database/document.tsweb/src/interfaces/request/compilation-template.tsweb/src/interfaces/request/document.tsweb/src/locales/en.tsweb/src/pages/chunk/parsed-result/add-knowledge/components/knowledge-chunk/components/document-structure-graph.tsxweb/src/pages/chunk/parsed-result/add-knowledge/components/knowledge-chunk/index.tsxweb/src/pages/dataset/artifact/artifact-graph.tsxweb/src/pages/dataset/artifact/artifact-link-renderer.tsxweb/src/pages/dataset/artifact/artifact-list.tsxweb/src/pages/dataset/artifact/artifact-viewer.tsxweb/src/pages/dataset/artifact/hooks/use-artifact-state.tsweb/src/pages/dataset/artifact/index.tsxweb/src/pages/dataset/dataset-overview/dataset-common.tsweb/src/pages/dataset/dataset/generate-button/generate.tsxweb/src/pages/dataset/dataset/generate-button/hook.tsweb/src/pages/dataset/sidebar/index.tsxweb/src/pages/user-setting/knowledge-compilation/components/artifact-extras.tsxweb/src/pages/user-setting/knowledge-compilation/components/builtin-template-popover.tsxweb/src/pages/user-setting/knowledge-compilation/components/entity-relation-section.tsxweb/src/pages/user-setting/knowledge-compilation/components/field-list-block.tsxweb/src/pages/user-setting/knowledge-compilation/components/global-rules-block.tsxweb/src/pages/user-setting/knowledge-compilation/edit-template-dialog.tsxweb/src/pages/user-setting/knowledge-compilation/edit-template-form.tsxweb/src/pages/user-setting/knowledge-compilation/hooks/use-template-form-state.tsweb/src/pages/user-setting/knowledge-compilation/index.tsxweb/src/pages/user-setting/knowledge-compilation/interface.tsweb/src/pages/user-setting/knowledge-compilation/template-card.tsxweb/src/pages/user-setting/sidebar/index.tsxweb/src/routes.tsxweb/src/services/compilation-template-service.tsweb/src/services/dataset-artifact-service.tsweb/src/services/knowledge-service.tsweb/src/utils/api.tsweb/vite.config.ts
✅ Files skipped from review due to trivial changes (5)
- web/src/interfaces/request/document.ts
- api/db/init_data/compilation_templates/empty.yaml
- web/src/services/dataset-artifact-service.ts
- web/src/pages/user-setting/knowledge-compilation/hooks/use-template-form-state.ts
- web/src/interfaces/database/compilation-template.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- rag/advanced_rag/knowlege_compile/_common.py
Comment on lines
+91
to
+228
| @manager.route("/compilation_templates", methods=["GET"]) # noqa: F821 | ||
| @login_required | ||
| def list_templates() -> Response: | ||
| keywords = request.args.get("keywords", "") | ||
| kind = request.args.get("kind", "") | ||
| page_number = int(request.args.get("page", 0)) | ||
| items_per_page = validate_rest_api_page_size(int(request.args.get("page_size", 0))) | ||
| orderby = request.args.get("orderby", "create_time") | ||
| desc = request.args.get("desc", "true").lower() != "false" | ||
|
|
||
| try: | ||
| templates = CompilationTemplateService.list_saved(current_user.id, keywords, kind, orderby, desc) | ||
| total = len(templates) | ||
| if page_number and items_per_page: | ||
| templates = templates[(page_number - 1) * items_per_page : page_number * items_per_page] | ||
| return get_json_result(data={"templates": templates, "total": total}) | ||
| except Exception as exc: | ||
| return server_error_response(exc) | ||
|
|
||
|
|
||
| @manager.route("/compilation_templates/builtins", methods=["GET"]) # noqa: F821 | ||
| @login_required | ||
| def list_builtin_templates() -> Response: | ||
| try: | ||
| templates = CompilationTemplateService.list_builtins() | ||
| if not templates: | ||
| CompilationTemplateService.seed_builtins_from_files() | ||
| templates = CompilationTemplateService.list_builtins() | ||
| if not templates: | ||
| templates = [ | ||
| { | ||
| "id": template["id"], | ||
| "kind": template["kind"], | ||
| "display_name": template["name"], | ||
| "description": template.get("description", ""), | ||
| "config": template["config"], | ||
| } | ||
| for template in CompilationTemplateService.load_builtins_from_files() | ||
| ] | ||
| return get_json_result(data=templates) | ||
| except Exception as exc: | ||
| return server_error_response(exc) | ||
|
|
||
|
|
||
| @manager.route("/compilation_templates/<template_id>", methods=["GET"]) # noqa: F821 | ||
| @login_required | ||
| def detail(template_id: str) -> Response: | ||
| try: | ||
| template = CompilationTemplateService.get_saved(template_id, current_user.id) | ||
| if template is None: | ||
| return get_data_error_result(message=f"Cannot find compilation template {template_id}.") | ||
| return get_json_result(data=template) | ||
| except Exception as exc: | ||
| return server_error_response(exc) | ||
|
|
||
|
|
||
| @manager.route("/compilation_templates", methods=["POST"]) # noqa: F821 | ||
| @login_required | ||
| @validate_request("name", "kind", "config") | ||
| async def create() -> Response: | ||
| req = await get_request_json() | ||
| error = _validate_template_payload(req) | ||
| if error: | ||
| return get_data_error_result(message=error) | ||
|
|
||
| name = req["name"].strip() | ||
| if CompilationTemplateService.name_exists(current_user.id, name): | ||
| return get_data_error_result(message="Duplicated compilation template name.") | ||
|
|
||
| data = { | ||
| "id": get_uuid(), | ||
| "tenant_id": current_user.id, | ||
| "name": name, | ||
| "description": req.get("description", ""), | ||
| "kind": req["kind"], | ||
| "config": req["config"], | ||
| "is_builtin": False, | ||
| "status": StatusEnum.VALID.value, | ||
| } | ||
| try: | ||
| CompilationTemplateService.insert(**data) | ||
| return get_json_result(data=data) | ||
| except Exception as exc: | ||
| return server_error_response(exc) | ||
|
|
||
|
|
||
| @manager.route("/compilation_templates/<template_id>", methods=["PUT"]) # noqa: F821 | ||
| @login_required | ||
| async def update(template_id: str) -> Response: | ||
| req = await get_request_json() | ||
| error = _validate_template_payload(req, require_all=False) | ||
| if error: | ||
| return get_data_error_result(message=error) | ||
|
|
||
| existing = CompilationTemplateService.get_saved(template_id, current_user.id) | ||
| if existing is None: | ||
| return get_data_error_result(message=f"Cannot find compilation template {template_id}.") | ||
|
|
||
| data = {key: req[key] for key in ["name", "description", "kind", "config"] if key in req} | ||
| if "name" in data: | ||
| data["name"] = data["name"].strip() | ||
| if CompilationTemplateService.name_exists(current_user.id, data["name"], template_id): | ||
| return get_data_error_result(message="Duplicated compilation template name.") | ||
|
|
||
| try: | ||
| CompilationTemplateService.filter_update( | ||
| [ | ||
| CompilationTemplate.id == template_id, | ||
| CompilationTemplate.tenant_id == current_user.id, | ||
| CompilationTemplate.is_builtin == False, | ||
| ], | ||
| data, | ||
| ) | ||
| updated = CompilationTemplateService.get_saved(template_id, current_user.id) | ||
| return get_json_result(data=updated) | ||
| except Exception as exc: | ||
| return server_error_response(exc) | ||
|
|
||
|
|
||
| @manager.route("/compilation_templates/<template_id>", methods=["DELETE"]) # noqa: F821 | ||
| @login_required | ||
| def delete(template_id: str) -> Response: | ||
| existing = CompilationTemplateService.get_saved(template_id, current_user.id) | ||
| if existing is None: | ||
| return get_data_error_result(message=f"Cannot find compilation template {template_id}.") | ||
|
|
||
| try: | ||
| CompilationTemplateService.filter_update( | ||
| [ | ||
| CompilationTemplate.id == template_id, | ||
| CompilationTemplate.tenant_id == current_user.id, | ||
| CompilationTemplate.is_builtin == False, | ||
| ], | ||
| {"status": StatusEnum.INVALID.value}, | ||
| ) | ||
| return get_json_result(data=True) | ||
| except Exception as exc: | ||
| return server_error_response(exc) |
Contributor
There was a problem hiding this comment.
Import or define manager before registering these routes.
Line 91 starts decorating handlers with manager.route(...), but this module never defines or imports manager. Importing the file will raise NameError before any endpoint is registered.
🤖 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 `@api/apps/restful_apis/compilation_template_api.py` around lines 91 - 228, The
handlers use the decorator manager.route (e.g., on functions list_templates,
list_builtin_templates, detail, create, update, delete) but manager is not
defined or imported; fix by importing or defining the Flask app/Blueprint object
named manager at the top of this module before any route decorators (for
example, import the existing manager Blueprint from the module where it is
declared or instantiate manager = Blueprint("manager", __name__) if this file
should own it), then re-run tests to ensure routes register without NameError.
Comment on lines
+102
to
+103
| templates = CompilationTemplateService.list_saved(current_user.id, keywords, kind, orderby, desc) | ||
| total = len(templates) |
Contributor
There was a problem hiding this comment.
Use the tenant id here, not current_user.id.
These handlers read and write CompilationTemplate.tenant_id with the user id. The model is tenant-scoped, and the artifact pipeline later resolves templates by tenant id, so templates created here can disappear from teammate views and from downstream compilation lookups. Thread the tenant id through these endpoints the same way the other dataset APIs do. As per coding guidelines, "api/apps/restful_apis//*.py: Use Pydantic request validation with service layer pattern in newer RESTful APIs under api/apps/restful_apis/" and "api//*.py: Backend uses Flask/Quart framework for API development."
Also applies to: 139-139, 157-163, 185-205, 213-225
🤖 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 `@api/apps/restful_apis/compilation_template_api.py` around lines 102 - 103,
Replace uses of current_user.id with the tenant id when calling
CompilationTemplateService methods: e.g. change
CompilationTemplateService.list_saved(current_user.id, ...) to pass the tenant
id (current_user.tenant_id or a tenant_id pulled from request context) and do
the same for other service calls such as any
get_saved/create_saved/update_saved/delete_saved usages in this module; thread
tenant_id through the endpoint handlers the same way other dataset APIs do so
the CompilationTemplate.tenant_id is consistently set and queried by tenant, not
user id, and keep the existing Pydantic request validation / service-layer call
pattern intact.
Source: Coding guidelines
Comment on lines
+179
to
+205
| async def update(template_id: str) -> Response: | ||
| req = await get_request_json() | ||
| error = _validate_template_payload(req, require_all=False) | ||
| if error: | ||
| return get_data_error_result(message=error) | ||
|
|
||
| existing = CompilationTemplateService.get_saved(template_id, current_user.id) | ||
| if existing is None: | ||
| return get_data_error_result(message=f"Cannot find compilation template {template_id}.") | ||
|
|
||
| data = {key: req[key] for key in ["name", "description", "kind", "config"] if key in req} | ||
| if "name" in data: | ||
| data["name"] = data["name"].strip() | ||
| if CompilationTemplateService.name_exists(current_user.id, data["name"], template_id): | ||
| return get_data_error_result(message="Duplicated compilation template name.") | ||
|
|
||
| try: | ||
| CompilationTemplateService.filter_update( | ||
| [ | ||
| CompilationTemplate.id == template_id, | ||
| CompilationTemplate.tenant_id == current_user.id, | ||
| CompilationTemplate.is_builtin == False, | ||
| ], | ||
| data, | ||
| ) | ||
| updated = CompilationTemplateService.get_saved(template_id, current_user.id) | ||
| return get_json_result(data=updated) |
Contributor
There was a problem hiding this comment.
Artifact-only validation is bypassed on partial updates.
_validate_template_payload() only runs the claim/concept checks when the incoming payload carries kind == "artifacts" (or config.kind). A PUT that updates only config on an existing artifacts template skips that branch entirely, so malformed artifact config can be saved and fail later in compilation. Validate against req merged with existing, or pass the existing kind into the validator.
🤖 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 `@api/apps/restful_apis/compilation_template_api.py` around lines 179 - 205, In
update(), _validate_template_payload is being called only on the raw req so
artifact-specific checks are skipped for partial updates; fix by merging req
with the existing template before validation (e.g. load existing via
CompilationTemplateService.get_saved(template_id, current_user.id) and build
merged_payload = {**existing_as_dict, **req}) and then call
_validate_template_payload(merged_payload, require_all=False), or alternatively
call _validate_template_payload(req, require_all=False,
existing_kind=existing.kind) if the validator supports a kind param; ensure the
validation happens before the duplicate-name check and before saving via
CompilationTemplateService.filter_update.
Comment on lines
+240
to
248
| parser_config_template_ids_changed = False | ||
| # parser config provided (already validated in UpdateDocumentReq), update it. | ||
| # Changing the document-scoped knowledge compilation templates affects | ||
| # parse output, so the document must be parsed again for them to execute. | ||
| if update_doc_req.parser_config: | ||
| old_parser_config = dict(req["parser_config"] or {}) | ||
| req["parser_config"].update(update_doc_req.parser_config.ext) | ||
| parser_config_template_ids_changed = _compilation_template_ids_changed(old_parser_config, req["parser_config"]) | ||
| DocumentService.update_parser_config(doc.id, req["parser_config"]) |
Contributor
There was a problem hiding this comment.
Compare against the stored parser config, not the request payload.
old_parser_config is copied from req["parser_config"], then the same request object is mutated and compared. That only tells you whether ext changed the request shape; it does not compare the persisted doc.parser_config with the new config. Real template-id changes can therefore skip reset_document_for_reparse(), leaving stale compiled output attached to the document.
Suggested fix
- old_parser_config = dict(req["parser_config"] or {})
- req["parser_config"].update(update_doc_req.parser_config.ext)
- parser_config_template_ids_changed = _compilation_template_ids_changed(old_parser_config, req["parser_config"])
- DocumentService.update_parser_config(doc.id, req["parser_config"])
+ old_parser_config = dict(doc.parser_config or {})
+ new_parser_config = dict(old_parser_config)
+ new_parser_config.update(req["parser_config"] or {})
+ new_parser_config.update(update_doc_req.parser_config.ext)
+ parser_config_template_ids_changed = _compilation_template_ids_changed(
+ old_parser_config, new_parser_config
+ )
+ DocumentService.update_parser_config(doc.id, new_parser_config)Also applies to: 258-263
🤖 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 `@api/apps/restful_apis/document_api.py` around lines 240 - 248, The current
comparison uses old_parser_config = dict(req["parser_config"] or {}) and then
mutates req["parser_config"], which compares the request payload to itself
instead of the persisted document config; change the code to read the stored
config from the document (doc.parser_config or dict(doc.parser_config or {}))
into old_parser_config, then update req["parser_config"] with
update_doc_req.parser_config.ext, call
_compilation_template_ids_changed(old_parser_config, req["parser_config"]) and,
if true, call reset_document_for_reparse(doc.id) (and keep
DocumentService.update_parser_config(doc.id, req["parser_config"]) as before);
apply the same fix for the block around lines 258-263.
Comment on lines
+14
to
+15
| - If only a relative time (e.g., "yesterday", "next week"), convert to | ||
| absolute when the context allows, else keep as is. |
Contributor
There was a problem hiding this comment.
Avoid asking the model to invent absolute dates from relative phrases.
Without an explicit reference date, converting "yesterday" or "next week" to an absolute timestamp is underspecified. That will produce inconsistent timeline rows across runs and documents. Keep relative expressions as-is unless the source chunk itself carries the anchor date.
🤖 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 `@api/db/init_data/compilation_templates/timeline.yaml` around lines 14 - 15,
Update the timeline template rule that currently says "If only a relative time
(e.g., 'yesterday', 'next week'), convert to absolute when the context allows,
else keep as is" so it never invents absolute dates without an explicit anchor:
keep relative expressions verbatim unless the input chunk contains an explicit
anchor/absolute date; only perform conversion when a concrete anchor date is
present in the same source chunk or a validated metadata field. Locate the rule
text in compilation_templates/timeline.yaml (the line mentioning "relative time"
/ examples "yesterday", "next week") and change wording/logic to prohibit
implicit date fabrication and require an explicit anchor before producing
absolute timestamps.
Comment on lines
+144
to
+162
| export const useFetchDocumentStructureGraph = (enabled: boolean) => { | ||
| const { knowledgeId, documentId } = useGetKnowledgeSearchParams(); | ||
| const { data, isFetching: loading } = useQuery({ | ||
| queryKey: ['fetchDocumentStructureGraph', knowledgeId, documentId], | ||
| enabled: enabled && !!knowledgeId && !!documentId, | ||
| initialData: EMPTY_DOCUMENT_STRUCTURE_GRAPH, | ||
| gcTime: 0, | ||
| queryFn: async () => { | ||
| const { data } = await kbService.getDocumentStructureGraph({ | ||
| kb_id: knowledgeId, | ||
| doc_id: documentId, | ||
| }); | ||
|
|
||
| return data?.data ?? EMPTY_DOCUMENT_STRUCTURE_GRAPH; | ||
| }, | ||
| }); | ||
|
|
||
| return { data: data ?? EMPTY_DOCUMENT_STRUCTURE_GRAPH, loading }; | ||
| }; |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Replace inline query key with a factory function.
The hook uses an inline queryKey array at line 147, which violates the coding guideline requiring query key factories. As per coding guidelines, every useQuery must use a {Domain}Keys factory that returns as const tuples for type safety and cache invalidation consistency.
♻️ Refactor to use query key factory
Add a factory at the top of the file:
+const ChunkKeys = {
+ documentStructureGraph: (knowledgeId: string, documentId: string) =>
+ ['fetchDocumentStructureGraph', knowledgeId, documentId] as const,
+} as const;
+
export const useFetchDocumentStructureGraph = (enabled: boolean) => {
const { knowledgeId, documentId } = useGetKnowledgeSearchParams();
const { data, isFetching: loading } = useQuery({
- queryKey: ['fetchDocumentStructureGraph', knowledgeId, documentId],
+ queryKey: ChunkKeys.documentStructureGraph(knowledgeId, documentId),
enabled: enabled && !!knowledgeId && !!documentId,As per coding guidelines: "Never write raw queryKey arrays inline in useQuery or invalidateQueries. Always use a query key factory object named {Domain}Keys that returns as const tuples."
📝 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
| export const useFetchDocumentStructureGraph = (enabled: boolean) => { | |
| const { knowledgeId, documentId } = useGetKnowledgeSearchParams(); | |
| const { data, isFetching: loading } = useQuery({ | |
| queryKey: ['fetchDocumentStructureGraph', knowledgeId, documentId], | |
| enabled: enabled && !!knowledgeId && !!documentId, | |
| initialData: EMPTY_DOCUMENT_STRUCTURE_GRAPH, | |
| gcTime: 0, | |
| queryFn: async () => { | |
| const { data } = await kbService.getDocumentStructureGraph({ | |
| kb_id: knowledgeId, | |
| doc_id: documentId, | |
| }); | |
| return data?.data ?? EMPTY_DOCUMENT_STRUCTURE_GRAPH; | |
| }, | |
| }); | |
| return { data: data ?? EMPTY_DOCUMENT_STRUCTURE_GRAPH, loading }; | |
| }; | |
| const ChunkKeys = { | |
| documentStructureGraph: (knowledgeId: string, documentId: string) => | |
| ['fetchDocumentStructureGraph', knowledgeId, documentId] as const, | |
| } as const; | |
| export const useFetchDocumentStructureGraph = (enabled: boolean) => { | |
| const { knowledgeId, documentId } = useGetKnowledgeSearchParams(); | |
| const { data, isFetching: loading } = useQuery({ | |
| queryKey: ChunkKeys.documentStructureGraph(knowledgeId, documentId), | |
| enabled: enabled && !!knowledgeId && !!documentId, | |
| initialData: EMPTY_DOCUMENT_STRUCTURE_GRAPH, | |
| gcTime: 0, | |
| queryFn: async () => { | |
| const { data } = await kbService.getDocumentStructureGraph({ | |
| kb_id: knowledgeId, | |
| doc_id: documentId, | |
| }); | |
| return data?.data ?? EMPTY_DOCUMENT_STRUCTURE_GRAPH; | |
| }, | |
| }); | |
| return { data: data ?? EMPTY_DOCUMENT_STRUCTURE_GRAPH, loading }; | |
| }; |
🤖 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/src/hooks/use-chunk-request.ts` around lines 144 - 162, Create a query
key factory (e.g., DocumentStructureGraphKeys) that returns an as-const tuple
for the fetchDocumentStructureGraph key and replace the inline queryKey array in
useFetchDocumentStructureGraph with a call to that factory (passing knowledgeId
and documentId). Update the useQuery invocation in
useFetchDocumentStructureGraph to use
DocumentStructureGraphKeys.fetch(knowledgeId, documentId) (or similar) and keep
the enabled, initialData (EMPTY_DOCUMENT_STRUCTURE_GRAPH), gcTime and queryFn
(kbService.getDocumentStructureGraph) behavior unchanged.
Source: Coding guidelines
Comment on lines
+177
to
+196
| export const useDeleteCompilationTemplate = () => { | ||
| const queryClient = useQueryClient(); | ||
| const { | ||
| data, | ||
| isPending: loading, | ||
| mutateAsync, | ||
| } = useMutation({ | ||
| mutationKey: ['deleteCompilationTemplate'], | ||
| mutationFn: async (ids: string[]) => { | ||
| const results = await Promise.all( | ||
| ids.map((id) => compilationTemplateService.delete({ id })), | ||
| ); | ||
| const failed = results.find(({ data = {} }) => data.code !== 0); | ||
| const data = failed?.data ?? { code: 0, data: true }; | ||
| if (!failed) { | ||
| message.success(i18n.t('message.deleted')); | ||
| queryClient.invalidateQueries({ | ||
| queryKey: CompilationTemplateKeys.all(), | ||
| }); | ||
| } |
Contributor
There was a problem hiding this comment.
Cache invalidation only on full batch success may cause stale UI.
When deleting multiple templates, the mutation only invalidates the cache if all deletions succeed (line 191). If a batch contains [id1, id2, id3] and id2 fails server-side, id1 and id3 are deleted but remain visible in the UI because the cache isn't invalidated. Users must refresh manually to see the partial success.
Consider invalidating the cache on any deletion attempt (even partial success) or tracking which IDs succeeded and selectively removing them from the cache.
🤖 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/src/hooks/use-compilation-template-request.ts` around lines 177 - 196,
The mutation in useDeleteCompilationTemplate only calls
queryClient.invalidateQueries when all deletions succeed, leaving UI stale on
partial success; update mutationFn in useDeleteCompilationTemplate to always
update the cache after the API calls — either call
queryClient.invalidateQueries({ queryKey: CompilationTemplateKeys.all() })
unconditionally after Promise.all completes, or inspect results to build a list
of successfully deleted ids and use queryClient.setQueryData/patch to remove
those ids from the cached CompilationTemplateKeys.all() data (refer to
mutationFn and queryClient.invalidateQueries / queryClient.setQueryData).
Comment on lines
+17
to
+18
| description: | ||
| entity.discription || entity.description || entity.aliases?.join(', '), |
Contributor
There was a problem hiding this comment.
Remove fallback for typo field once interface is corrected.
Line 18 reads both entity.discription (typo) and entity.description. This mirrors the typo in IDocumentStructureGraph. Once the interface is corrected to use only description, remove the discription fallback here.
📝 Suggested fix after interface correction
description:
- entity.discription || entity.description || entity.aliases?.join(', '),
+ entity.description || entity.aliases?.join(', '),🤖 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/src/pages/chunk/parsed-result/add-knowledge/components/knowledge-chunk/components/document-structure-graph.tsx`
around lines 17 - 18, The code currently falls back to the misspelled property
entity.discription when building the description field; after correcting the
IDocumentStructureGraph interface to use only description, remove the fallback
to entity.discription and use entity.description (and aliases fallback)
exclusively in the description assignment inside the component that references
entity (look for description: entity.discription || entity.description ||
entity.aliases?.join(', ')). Ensure no other references to entity.discription
remain.
Comment on lines
+18
to
+20
| * Top-right popup list of the five server-side built-in templates. | ||
| * The list is cached for the session via React Query — the popover | ||
| * doesn't refetch on every open. |
Contributor
There was a problem hiding this comment.
Comment contradicts implementation.
The comment states the list "doesn't refetch on every open," but line 37 explicitly calls refetch() when the popover opens.
📝 Proposed fix
Remove or correct the misleading comment:
/**
* Top-right popup list of the five server-side built-in templates.
- * The list is cached for the session via React Query — the popover
- * doesn't refetch on every open.
+ * The list is refetched each time the popover opens.
*/📝 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
| * Top-right popup list of the five server-side built-in templates. | |
| * The list is cached for the session via React Query — the popover | |
| * doesn't refetch on every open. | |
| /** | |
| * Top-right popup list of the five server-side built-in templates. | |
| * The list is refetched each time the popover opens. | |
| */ |
🤖 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/src/pages/user-setting/knowledge-compilation/components/builtin-template-popover.tsx`
around lines 18 - 20, Update the header comment in builtin-template-popover.tsx
to match the implementation: it currently claims the list "doesn't refetch on
every open" but the component (BuiltinTemplatePopover) explicitly calls
refetch() when opening; either remove that misleading sentence or change it to
state that the list is cached via React Query but is explicitly refetched on
popover open (mentioning the refetch() call used when opening to make behavior
clear).
Comment on lines
+62
to
+72
| target: 'http://192.168.1.24:9381/', | ||
| changeOrigin: true, | ||
| ws: true, | ||
| }, | ||
| '/api': { | ||
| target: 'http://127.0.0.1:9380/', | ||
| target: 'http://192.168.1.24:9382/', | ||
| changeOrigin: true, | ||
| ws: true, | ||
| }, | ||
| '/v1': { | ||
| target: 'http://127.0.0.1:9380/', | ||
| target: 'http://192.168.1.24:9382/', |
Contributor
There was a problem hiding this comment.
Avoid hardcoding a developer-specific LAN IP in default proxy targets.
With proxyScheme defaulting to python (Line 55), these hardcoded 192.168.1.24 targets make dev startup depend on one machine/network and will fail for other contributors. Externalize host/ports to env vars and default to localhost.
Suggested fix
const proxySchemes = {
python: {
'/api/v1/admin': {
- target: 'http://192.168.1.24:9381/',
+ target: `http://${env.API_PROXY_HOST || '127.0.0.1'}:${env.API_PROXY_ADMIN_PORT || '9381'}/`,
changeOrigin: true,
ws: true,
},
'/api': {
- target: 'http://192.168.1.24:9382/',
+ target: `http://${env.API_PROXY_HOST || '127.0.0.1'}:${env.API_PROXY_PORT || '9382'}/`,
changeOrigin: true,
ws: true,
},
'/v1': {
- target: 'http://192.168.1.24:9382/',
+ target: `http://${env.API_PROXY_HOST || '127.0.0.1'}:${env.API_PROXY_PORT || '9382'}/`,
changeOrigin: true,
ws: true,
},
},As per coding guidelines, API traffic should use stable project proxy prefixes (/v1, /api/v1) and environment-configurable routing rather than machine-specific endpoints.
🤖 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/vite.config.ts` around lines 62 - 72, The proxy targets currently
hardcode a developer LAN IP (e.g., targets under '/api' and '/v1') which breaks
other machines; update the proxy configuration in vite.config.ts to read
host/port from environment variables (e.g., PROXY_HOST, PROXY_PORT or separate
PROXY_API_HOST/PROXY_V1_HOST) with sensible defaults to 'localhost' and default
ports, and ensure proxy prefixes use the stable project prefixes ('/v1',
'/api/v1'); adjust the code paths that construct the target (where proxyScheme
is used) to compose `${proxyScheme}://${process.env.PROXY_HOST ||
'localhost'}:${process.env.PROXY_PORT || '9382'}/` (or separate vars per prefix)
so startup no longer depends on 192.168.1.24.
Source: Coding guidelines
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (12)
web/src/pages/dataset/dataset/generate-button/generate.tsx (1)
300-315:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHandle
Artifactin the delete confirmation copy.
GenerateLogButtoncan now receiveGenerateType.Artifact, but every non-KnowledgeGraphbranch falls through to RAPTOR. Artifact delete confirmations will say “RAPTOR”; use a type-label map.Proposed fix
export const GenerateLogButton = (props: IGenerateLogProps) => { const { t } = useTranslation(); const { message, finish_at, type, onDelete } = props; + const typeLabel = + type === GenerateType.KnowledgeGraph + ? t('knowledgeDetails.knowledgeGraph') + : type === GenerateType.Artifact + ? t('knowledgeDetails.artifact') + : t('knowledgeDetails.raptor'); @@ title: t('common.delete') + ' ' + - (type === GenerateType.KnowledgeGraph - ? t('knowledgeDetails.knowledgeGraph') - : t('knowledgeDetails.raptor')), + typeLabel, @@ type: - type === GenerateType.KnowledgeGraph - ? t('knowledgeDetails.knowledgeGraph') - : t('knowledgeDetails.raptor'), + typeLabel,🤖 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/src/pages/dataset/dataset/generate-button/generate.tsx` around lines 300 - 315, The delete confirmation dialog in the generate.tsx file contains ternary operators that only handle GenerateType.KnowledgeGraph and fall through to RAPTOR for all other types, which causes GenerateType.Artifact to be incorrectly labeled as RAPTOR. Create a type-label map object that maps each GenerateType value (KnowledgeGraph, Raptor, and Artifact) to its corresponding translated label, then replace both the ternary operators in the title calculation and the type parameter passed to the translation function with lookups into this map. This ensures each artifact deletion confirmation displays the correct type label.api/apps/services/dataset_api_service.py (2)
1462-1470:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMove artifact doc-store calls off the Quart event loop.
These async helpers call
index_exist/searchsynchronously, so ES latency blocks the server event loop.get_artifact_graphalready usesthread_pool_exec; apply the same pattern to_artifact_index_or_none,has_any_artifact,list_artifacts, andget_artifact_page.Proposed direction
-def _artifact_index_or_none(tenant_id: str, kb_id: str): +async def _artifact_index_or_none(tenant_id: str, kb_id: str): from rag.nlp import search as _rag_search index_nm = _rag_search.index_name(tenant_id) - if not settings.docStoreConn.index_exist(index_nm, kb_id): + if not await thread_pool_exec(settings.docStoreConn.index_exist, index_nm, kb_id): return None return index_nm, _rag_search- res = settings.docStoreConn.search( + res = await thread_pool_exec( + settings.docStoreConn.search, select_fields=select_fields, highlight_fields=[], condition=condition, match_expressions=[], order_by=order_by, offset=offset, limit=page_size, index_names=index_nm, knowledgebase_ids=[dataset_id], )As per coding guidelines,
api/**/*.py: “Quart-based async HTTP server pattern for API implementation in api/ragflow_server.py.”Also applies to: 1491-1498, 1550-1556, 1608-1618
🤖 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 `@api/apps/services/dataset_api_service.py` around lines 1462 - 1470, The synchronous Elasticsearch doc-store calls in _artifact_index_or_none, has_any_artifact, list_artifacts, and get_artifact_page are blocking the Quart async event loop. Refactor these four functions to wrap their synchronous calls to docStoreConn (such as index_exist and search methods) using thread_pool_exec, following the same pattern already implemented in get_artifact_graph. This ensures that Elasticsearch latency does not block the server event loop by executing the blocking I/O operations in a separate thread pool.Source: Coding guidelines
827-841:⚠️ Potential issue | 🟠 Major | ⚡ Quick winWire
delete_index(..., "artifact")to wipe artifact rows.After adding
"artifact"to_VALID_INDEX_TYPES, this generic delete path clears the task id but leavesartifact_map_extract,artifact_reduce_result,artifact_compilation_plan,artifact_page_draft,artifact_page, andartifact_page_graphrows visible/reusable. Add an artifact wipe branch or share the deletion loop used byclear_artifacts.Proposed fix
elif wipe and index_type == "raptor": from rag.nlp import search settings.docStoreConn.delete({"raptor_kwd": ["raptor"]}, search.index_name(kb.tenant_id), dataset_id) + elif wipe and index_type == "artifact": + from rag.nlp import search + + index_nm = search.index_name(kb.tenant_id) + for kwd in _ARTIFACT_COMPILE_KWDS: + settings.docStoreConn.delete({"compile_kwd": kwd}, index_nm, dataset_id) KnowledgebaseService.update_by_id(kb.id, {task_id_field: "", task_finish_at_field: None})🤖 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 `@api/apps/services/dataset_api_service.py` around lines 827 - 841, The delete_index function has conditional branches for wipe operations when index_type is "graph" or "raptor", but is missing a corresponding branch for index_type == "artifact". Add an elif wipe and index_type == "artifact" branch that deletes the artifact-related rows (artifact_map_extract, artifact_reduce_result, artifact_compilation_plan, artifact_page_draft, artifact_page, and artifact_page_graph) from the document store using settings.docStoreConn.delete, similar to how the graph and raptor branches handle their respective deletions. You can either implement the deletion directly in the new branch or extract and reuse the deletion logic from the clear_artifacts method if one exists.rag/svr/task_executor_refactor/task_handler.py (5)
1009-1010:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDelete stale
artifact_pagerows before bulk insert.Current slugs overwrite by deterministic id, but pages removed or renamed by REFINE are never deleted. Since list/page APIs read every
compile_kwd="artifact_page"row, stale pages remain visible and can pollute artifact links/graphs. Delete existing page rows for the KB before inserting the refreshed set, or delete rows whose ids are not in the new set.Proposed minimal fix
if not rows: return try: + await thread_pool_exec( + settings.docStoreConn.delete, + {"compile_kwd": "artifact_page"}, + index, + ctx.kb_id, + ) await thread_pool_exec(settings.docStoreConn.insert, rows, index, ctx.kb_id) except Exception:🤖 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 `@rag/svr/task_executor_refactor/task_handler.py` around lines 1009 - 1010, The bulk insert of artifact_page rows via the thread_pool_exec call with settings.docStoreConn.insert does not delete stale page rows that were removed or renamed in previous REFINE operations, causing them to remain visible in list/page APIs. Before the thread_pool_exec insert call, add a delete operation to remove existing artifact_page rows for the current KB (identified by ctx.kb_id) so that only the refreshed set of pages is present after the insert, preventing stale pages from polluting artifact links and graphs.
514-548:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStart the TOC task before awaiting
_process_toc_thread.
toc_threadis alwaysNone, so_process_toc_threadnever inserts a TOC chunk even whenparser_config.toc_extractionis enabled._build_tocis already written forasyncio.to_thread; create that task before chunk insertion.Proposed fix
# Build TOC if needed (TOC continues to run in parallel during ingest; # artifact_compilation has been moved to AFTER the chunk insert below # because REFINE needs to look the source chunks up in ES by id). toc_thread = None + if ctx.parser_config.get("toc_extraction"): + toc_thread = asyncio.create_task( + asyncio.to_thread(self._build_toc, ctx, chunks, ctx.progress_cb) + ) # Insert chunks🤖 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 `@rag/svr/task_executor_refactor/task_handler.py` around lines 514 - 548, The `toc_thread` variable is initialized to `None` and never assigned a task, so the TOC chunk is never inserted even when TOC extraction is enabled. Before awaiting the chunk insertion with `chunk_service.insert_chunks`, create the TOC task by using `asyncio.to_thread` to call the `_build_toc` method (which is already written for this purpose). Assign this task to `toc_thread` so that it runs in parallel during chunk insertion, and then the subsequent call to `self._process_toc_thread(toc_thread)` will have the actual task to await instead of `None`.
67-81:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMerge template IDs from top-level and
extconfig.This helper returns as soon as it finds top-level
compilation_template_ids, so legacy or nestedextids are silently ignored. Use ordered de-dupe across both locations, matching the structure-graph endpoint, so migrated documents do not lose selected templates.Proposed fix
def _parser_config_compilation_template_ids(parser_config) -> list[str]: if not isinstance(parser_config, dict): return [] - ids = parser_config.get("compilation_template_ids") - if isinstance(ids, list): - return [str(x).strip() for x in ids if isinstance(x, str) and x.strip()] - legacy = parser_config.get("compilation_template_id") - if isinstance(legacy, str) and legacy.strip(): - return [legacy.strip()] - ext = parser_config.get("ext") - if isinstance(ext, dict): - ids = ext.get("compilation_template_ids") + out: list[str] = [] + for loc in (parser_config, parser_config.get("ext") if isinstance(parser_config.get("ext"), dict) else None): + if not isinstance(loc, dict): + continue + ids = loc.get("compilation_template_ids") if isinstance(ids, list): - return [str(x).strip() for x in ids if isinstance(x, str) and x.strip()] - legacy = ext.get("compilation_template_id") - if isinstance(legacy, str) and legacy.strip(): - return [legacy.strip()] - return [] + for x in ids: + if isinstance(x, str) and x.strip() and x.strip() not in out: + out.append(x.strip()) + legacy = loc.get("compilation_template_id") + if isinstance(legacy, str) and legacy.strip() and legacy.strip() not in out: + out.append(legacy.strip()) + return out🤖 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 `@rag/svr/task_executor_refactor/task_handler.py` around lines 67 - 81, The function currently returns early as soon as it finds top-level compilation_template_ids or legacy compilation_template_id, causing any template IDs from the ext dictionary to be silently ignored. Instead of returning immediately when finding IDs at each level, accumulate all valid template IDs from both the top-level and ext locations into a single collection, then apply ordered de-duplication (preserving insertion order while removing duplicates) before returning the final merged list. This ensures templates from both locations are preserved for migrated documents.
854-879:⚠️ Potential issue | 🟠 Major | ⚡ Quick winActually exclude compiled rows when loading MAP input.
_load_chunks_for_docsays it skipscompile_kwdrows, butcompile_kwdis not selected and the query has nomust_not, sorow.get("compile_kwd")is always empty. Add the field and filter at query time to avoid feeding prior compilation output back into artifact MAP.Proposed fix
select_fields = [ "id", "doc_id", "content_with_weight", - "page_num_int", "top_int", + "page_num_int", "top_int", "compile_kwd", ] @@ settings.docStoreConn.search, - select_fields, [], {"doc_id": [doc_id], "available_int": 1}, + select_fields, [], + { + "doc_id": [doc_id], + "available_int": 1, + "must_not": {"exists": "compile_kwd"}, + }, [], OrderByExpr(), offset, PAGE,🤖 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 `@rag/svr/task_executor_refactor/task_handler.py` around lines 854 - 879, The _load_chunks_for_doc function attempts to skip rows with compile_kwd using row.get("compile_kwd"), but compile_kwd is not included in the select_fields list and there is no query-time filter to exclude such rows, so the check is ineffective. Add "compile_kwd" to the select_fields list and add a must_not filter condition in the query parameters (in the docStoreConn.search call) to exclude rows where compile_kwd exists, ensuring compiled rows are properly filtered out at query time rather than relying on a post-query defensive check.
282-321:⚠️ Potential issue | 🟠 MajorQueue the per-doc RAPTOR task instead of running it inline.
At line 560, the code reads
ctx.parser_config["raptor"]["use_raptor"](doc-scoped setting), but instead of queueing the task viaqueue_per_doc_raptor_task, it callsawait self._run_raptor()directly. Inside_run_raptor, the code reloads and useskb.parser_config(KB-scoped), which ignores the doc-specific prompt, threshold, and scope settings.Replace the inline call with a queued task so the executor picks it up and uses the doc's config:
raptor_cfg = (ctx.parser_config or {}).get("raptor") or {} if raptor_cfg.get("use_raptor"): try: ok_doc, doc_obj = DocumentService.get_by_id(task_doc_id) if ok_doc and doc_obj is not None: ctx.progress_cb(msg="Starting RAPTOR task.") queue_per_doc_raptor_task(doc_obj, ctx.priority) # Queue, don't run inline else: logging.warning(...) except Exception: logging.exception(...)🤖 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 `@rag/svr/task_executor_refactor/task_handler.py` around lines 282 - 321, The per-document RAPTOR task is being executed inline via await self._run_raptor() instead of being queued, causing it to ignore doc-specific settings (prompt, threshold, scope) and use only KB-level configuration. Replace the inline call to await self._run_raptor() with a call to queue_per_doc_raptor_task, retrieving the document object via DocumentService.get_by_id(task_doc_id) and passing it along with ctx.priority to the queue function. Wrap the DocumentService call in appropriate error handling and progress callbacks.api/apps/restful_apis/chunk_api.py (1)
861-864:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject compiled rows in the update/switch paths too.
list_chunks,get_chunk, andrm_chunknow protectcompile_kwdrows, butupdate_chunkandswitch_chunkscan still mutate any known compiled row id under the document. Add the samecompile_kwdrejection inupdate_chunk, and include amust_not exists: compile_kwdguard or prefetch/reject compiled ids inswitch_chunks.Proposed fix
chunk = settings.docStoreConn.get(chunk_id, search.index_name(dataset_tenant_id), [dataset_id]) if chunk is None or str(chunk.get("doc_id", chunk.get("document_id"))) != str(document_id): return get_error_data_result(f"Can't find this chunk {chunk_id}") + if chunk.get("compile_kwd"): + return get_error_data_result(f"Can't update compiled chunk {chunk_id}")if not settings.docStoreConn.update( - {"id": cid}, + {"id": cid, "must_not": {"exists": "compile_kwd"}}, {"available_int": available_int}, search.index_name(dataset_tenant_id), doc.kb_id,Also applies to: 958-965
🤖 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 `@api/apps/restful_apis/chunk_api.py` around lines 861 - 864, Add protection against mutating compiled rows in both the update_chunk and switch_chunks functions. In the update_chunk function (around lines 861-864), after retrieving the chunk and validating the document_id, add a check to reject the request if the chunk contains compile_kwd set, returning an appropriate error message similar to the existing validation patterns in list_chunks, get_chunk, and rm_chunk. For the switch_chunks function (lines 958-965), implement the same compile_kwd rejection logic either through a must_not exists guard condition or by prefetching and rejecting any compiled chunk ids before performing the switch operation. This ensures consistent protection across all chunk mutation paths.web/src/pages/chunk/parsed-result/add-knowledge/components/knowledge-chunk/components/document-structure-graph.tsx (1)
194-257:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winLocalize the new structure-graph UI strings.
Structure graph,Close, empty/loading messages, and count labels are user-facing but hardcoded. Wire them throughuseTranslation()and add locale keys.As per coding guidelines,
web/**/*.{ts,tsx}: Usereact-i18nextfor internationalization supporting 17 languages, and preferuseTranslationfromreact-i18nextover project-wrapped utilities.Example direction
import { X } from 'lucide-react'; import { useEffect, useMemo, useState } from 'react'; +import { useTranslation } from 'react-i18next'; @@ }) { + const { t } = useTranslation(); @@ - Structure graph + {t('chunk.structureGraph.title')} @@ - Close + {t('common.close')} @@ - No generated structure graph. + {t('chunk.structureGraph.empty')} @@ - {loading ? 'Loading…' : 'No data for this template.'} + {loading + ? t('common.loading') + : t('chunk.structureGraph.noTemplateData')}🤖 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/src/pages/chunk/parsed-result/add-knowledge/components/knowledge-chunk/components/document-structure-graph.tsx` around lines 194 - 257, The document-structure-graph component contains hardcoded user-facing strings including "Structure graph", "Close", "No generated structure graph.", "Loading…", "No data for this template.", and the entity/relation count labels. To fix this, import the useTranslation hook from react-i18next at the top of the file, call it within the component to get the translation function t, and replace all hardcoded strings with their corresponding t() calls using appropriate locale keys (for example, t('structure_graph.title') for "Structure graph", t('common.close') for "Close", etc.). Add the corresponding translation keys to your locale JSON files to support all 17 languages as per the project's i18n guidelines.Source: Coding guidelines
rag/raptor.py (1)
750-750:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFinish the RAPTOR 3-tuple provenance migration. The shared root cause is that RAPTOR now normalizes/returns
(text, vector, source_chunk_ids), while classic clustering and task persistence still assume(text, vector).
rag/raptor.py#L750-L750: read embeddings viachunk[1]or unpack with a rest slot.rag/svr/task_executor.py#L1121-L1128: unpack generated summaries withsource_chunk_idsand persist them on the summary chunk.rag/svr/task_executor.py#L1158-L1169: fetch chunkidand seed file-level RAPTOR inputs as(content, vector, [id]).rag/svr/task_executor.py#L1219-L1232: apply the sameidfetch and 3-tuple seed for dataset-level RAPTOR inputs.🤖 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 `@rag/raptor.py` at line 750, Complete the RAPTOR 3-tuple provenance migration across all affected sites. In rag/raptor.py#L750-L750, update the embeddings extraction to read via chunk[1] to correctly access the vector from the new 3-tuple format (text, vector, source_chunk_ids). In rag/svr/task_executor.py#L1121-L1128, unpack the generated summaries to extract all three components including source_chunk_ids and persist the chunk IDs on the summary chunk object. In rag/svr/task_executor.py#L1158-L1169, fetch the chunk id field and construct file-level RAPTOR inputs as 3-tuples in the form (content, vector, [id]). In rag/svr/task_executor.py#L1219-L1232, apply the same chunk id fetch and 3-tuple seeding approach for dataset-level RAPTOR inputs. Ensure all sites consistently use the new 3-tuple format throughout the RAPTOR clustering and task persistence pipeline.rag/advanced_rag/knowlege_compile/structure.py (1)
950-959:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve template stamps when rebuilding merged ES docs.
_struct_rebuild_es_doc()calls_struct_to_es_doc()without the template id/kind that_struct_to_es_doc()now relies on for row identity and template-scoped graph rebuilds. A locally merged doc can losecompilation_template_ids, which makes_struct_es_dedup_one()skip the template filter and lets identical entities merge across template tabs.Preserve template metadata through rebuilds
new_doc = _struct_to_es_doc( payload=payload, compile_kwd=base_doc.get("compile_kwd"), doc_id=base_doc.get("doc_id"), chunk_ids=chunk_ids, vec=vec, kind=kind, src_field=src_field, target_field=target_field, + compilation_template_id=_struct_doc_template_id(base_doc), + compilation_template_kind=base_doc.get("compilation_template_kind_kwd"), ) + if base_doc.get("compilation_template_ids"): + raw_template_ids = base_doc["compilation_template_ids"] + new_doc["compilation_template_ids"] = ( + raw_template_ids + if isinstance(raw_template_ids, list) + else [raw_template_ids] + )select_fields = [ "id", "content_with_weight", "source_id", "knowledge_graph_kwd", "compile_kwd", - "doc_id", "from_entity_kwd", "to_entity_kwd", + "doc_id", "from_entity_kwd", "to_entity_kwd", + "compilation_template_ids", "compilation_template_kind_kwd", ]Also applies to: 1094-1097
🤖 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 `@rag/advanced_rag/knowlege_compile/structure.py` around lines 950 - 959, The `_struct_rebuild_es_doc()` function calls `_struct_to_es_doc()` without passing the template id/kind parameters that are now required by `_struct_to_es_doc()` for maintaining row identity and template-scoped graph rebuilds. Without these parameters, merged ES documents lose their `compilation_template_ids`, causing `_struct_es_dedup_one()` to skip template filtering and incorrectly allow entities to merge across different templates. Extract the template id/kind information from `base_doc` (similar to how `compile_kwd` and `doc_id` are already extracted) and pass them as additional parameters to the `_struct_to_es_doc()` call at lines 950-959. Apply the same fix to the other call location at lines 1094-1097.
🧹 Nitpick comments (2)
web/src/components/chunk-method-dialog/index.tsx (1)
72-75: ⚡ Quick winUse
useTranslationfor the new selector.
KnowledgeCompilationTemplateSelectintroduces a new use of the project-wrappeduseTranslate; switch this component touseTranslationwith the appropriate key prefix/namespace.As per coding guidelines,
web/**/*.{tsx,ts}: “PreferuseTranslationfromreact-i18nextover project-wrapped utilities likeuseTranslatefor i18n in React components.”🤖 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/src/components/chunk-method-dialog/index.tsx` around lines 72 - 75, The KnowledgeCompilationTemplateSelect function is using the project-wrapped useTranslate utility instead of the preferred useTranslation hook from react-i18next. Replace the useTranslate import and call with useTranslation from react-i18next, passing the 'knowledgeConfiguration' namespace as an argument to maintain the same translation context.Source: Coding guidelines
api/db/services/document_service.py (1)
1105-1146: ⚡ Quick winAdd queue logs for the new per-doc RAPTOR flow.
This helper creates a DB task and publishes a Redis message, but emits no log tying
task_id,doc_id, and queue priority together. Add lightweight logs without dumpingparser_config.As per coding guidelines,
**/*.py: Add logging for new flows.Proposed logging addition
def queue_per_doc_raptor_task(doc, priority): """Queue a doc-scoped RAPTOR task. @@ called at most once, which is the only invariant the caller needs. """ + logging.info( + "Queueing doc-scoped RAPTOR task for doc_id=%s priority=%s", + doc.get("id"), + priority, + ) chunking_config = DocumentService.get_chunking_config(doc["id"]) @@ assert REDIS_CONN.queue_product( settings.get_svr_queue_name(priority, "raptor"), message=task, ), "Can't access Redis. Please check the Redis' status." + logging.info( + "Queued doc-scoped RAPTOR task_id=%s doc_id=%s priority=%s", + task["id"], + doc["id"], + priority, + ) return task["id"]🤖 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 `@api/db/services/document_service.py` around lines 1105 - 1146, The queue_per_doc_raptor_task function creates a database task and publishes a Redis message but does not emit any logs to track the operation. Add lightweight logging after the task is inserted into the database with bulk_insert_into_db to log the task_id and doc_id, and add another log after the Redis message is queued with REDIS_CONN.queue_product to log the queue name (derived from priority) along with the task_id. Do not include parser_config or other verbose data in these logs, keeping them focused on the task tracking information needed for monitoring and debugging.Source: Coding guidelines
🤖 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 `@rag/svr/task_executor_refactor/raptor_service.py`:
- Around line 531-584: The `_build_raptor_graph` method creates a full bipartite
layer-by-layer connection (every node at layer K connects to every node at layer
K-1) which misrepresents unrelated summaries. Instead of connecting all
parent-child pairs across layers, load the `source_chunk_ids` field for each
node when processing rows, store it in the by_id dictionary, and modify the
layered fan-out section where relations are built to only create an edge between
a parent and child node if their source_chunk_ids sets overlap (have at least
one common chunk in provenance). This will replace the full bipartite connection
logic with provenance-based filtering. Apply the same provenance-based filtering
logic to the additional affected site at lines 613-620.
In `@web/src/pages/dataset/knowledge-graph/tree-graph.tsx`:
- Around line 108-122: In the getContent function within the tree-graph.tsx
file, the tooltip content is being constructed by directly interpolating
item.id, item.entity_type, and item.description into HTML strings without
escaping. These document/LLM-derived values are untrusted and could contain
malicious HTML or JavaScript. Escape each of these three values (item.id,
item.entity_type, and item.description) using an appropriate HTML escaping
function before inserting them into the template strings to prevent stored XSS
attacks.
- Around line 201-205: Add a cleanup function to the useEffect hook that
destroys the G6 graph instance. The cleanup function should be returned from the
useEffect and should call the destroy method on the graph instance (typically
stored as a ref or state variable). This cleanup will run both when the
component unmounts and whenever the data dependency changes to an empty state,
preventing the canvas instance and event handlers from persisting after the
component is no longer needed.
---
Outside diff comments:
In `@api/apps/restful_apis/chunk_api.py`:
- Around line 861-864: Add protection against mutating compiled rows in both the
update_chunk and switch_chunks functions. In the update_chunk function (around
lines 861-864), after retrieving the chunk and validating the document_id, add a
check to reject the request if the chunk contains compile_kwd set, returning an
appropriate error message similar to the existing validation patterns in
list_chunks, get_chunk, and rm_chunk. For the switch_chunks function (lines
958-965), implement the same compile_kwd rejection logic either through a
must_not exists guard condition or by prefetching and rejecting any compiled
chunk ids before performing the switch operation. This ensures consistent
protection across all chunk mutation paths.
In `@api/apps/services/dataset_api_service.py`:
- Around line 1462-1470: The synchronous Elasticsearch doc-store calls in
_artifact_index_or_none, has_any_artifact, list_artifacts, and get_artifact_page
are blocking the Quart async event loop. Refactor these four functions to wrap
their synchronous calls to docStoreConn (such as index_exist and search methods)
using thread_pool_exec, following the same pattern already implemented in
get_artifact_graph. This ensures that Elasticsearch latency does not block the
server event loop by executing the blocking I/O operations in a separate thread
pool.
- Around line 827-841: The delete_index function has conditional branches for
wipe operations when index_type is "graph" or "raptor", but is missing a
corresponding branch for index_type == "artifact". Add an elif wipe and
index_type == "artifact" branch that deletes the artifact-related rows
(artifact_map_extract, artifact_reduce_result, artifact_compilation_plan,
artifact_page_draft, artifact_page, and artifact_page_graph) from the document
store using settings.docStoreConn.delete, similar to how the graph and raptor
branches handle their respective deletions. You can either implement the
deletion directly in the new branch or extract and reuse the deletion logic from
the clear_artifacts method if one exists.
In `@rag/advanced_rag/knowlege_compile/structure.py`:
- Around line 950-959: The `_struct_rebuild_es_doc()` function calls
`_struct_to_es_doc()` without passing the template id/kind parameters that are
now required by `_struct_to_es_doc()` for maintaining row identity and
template-scoped graph rebuilds. Without these parameters, merged ES documents
lose their `compilation_template_ids`, causing `_struct_es_dedup_one()` to skip
template filtering and incorrectly allow entities to merge across different
templates. Extract the template id/kind information from `base_doc` (similar to
how `compile_kwd` and `doc_id` are already extracted) and pass them as
additional parameters to the `_struct_to_es_doc()` call at lines 950-959. Apply
the same fix to the other call location at lines 1094-1097.
In `@rag/raptor.py`:
- Line 750: Complete the RAPTOR 3-tuple provenance migration across all affected
sites. In rag/raptor.py#L750-L750, update the embeddings extraction to read via
chunk[1] to correctly access the vector from the new 3-tuple format (text,
vector, source_chunk_ids). In rag/svr/task_executor.py#L1121-L1128, unpack the
generated summaries to extract all three components including source_chunk_ids
and persist the chunk IDs on the summary chunk object. In
rag/svr/task_executor.py#L1158-L1169, fetch the chunk id field and construct
file-level RAPTOR inputs as 3-tuples in the form (content, vector, [id]). In
rag/svr/task_executor.py#L1219-L1232, apply the same chunk id fetch and 3-tuple
seeding approach for dataset-level RAPTOR inputs. Ensure all sites consistently
use the new 3-tuple format throughout the RAPTOR clustering and task persistence
pipeline.
In `@rag/svr/task_executor_refactor/task_handler.py`:
- Around line 1009-1010: The bulk insert of artifact_page rows via the
thread_pool_exec call with settings.docStoreConn.insert does not delete stale
page rows that were removed or renamed in previous REFINE operations, causing
them to remain visible in list/page APIs. Before the thread_pool_exec insert
call, add a delete operation to remove existing artifact_page rows for the
current KB (identified by ctx.kb_id) so that only the refreshed set of pages is
present after the insert, preventing stale pages from polluting artifact links
and graphs.
- Around line 514-548: The `toc_thread` variable is initialized to `None` and
never assigned a task, so the TOC chunk is never inserted even when TOC
extraction is enabled. Before awaiting the chunk insertion with
`chunk_service.insert_chunks`, create the TOC task by using `asyncio.to_thread`
to call the `_build_toc` method (which is already written for this purpose).
Assign this task to `toc_thread` so that it runs in parallel during chunk
insertion, and then the subsequent call to
`self._process_toc_thread(toc_thread)` will have the actual task to await
instead of `None`.
- Around line 67-81: The function currently returns early as soon as it finds
top-level compilation_template_ids or legacy compilation_template_id, causing
any template IDs from the ext dictionary to be silently ignored. Instead of
returning immediately when finding IDs at each level, accumulate all valid
template IDs from both the top-level and ext locations into a single collection,
then apply ordered de-duplication (preserving insertion order while removing
duplicates) before returning the final merged list. This ensures templates from
both locations are preserved for migrated documents.
- Around line 854-879: The _load_chunks_for_doc function attempts to skip rows
with compile_kwd using row.get("compile_kwd"), but compile_kwd is not included
in the select_fields list and there is no query-time filter to exclude such
rows, so the check is ineffective. Add "compile_kwd" to the select_fields list
and add a must_not filter condition in the query parameters (in the
docStoreConn.search call) to exclude rows where compile_kwd exists, ensuring
compiled rows are properly filtered out at query time rather than relying on a
post-query defensive check.
- Around line 282-321: The per-document RAPTOR task is being executed inline via
await self._run_raptor() instead of being queued, causing it to ignore
doc-specific settings (prompt, threshold, scope) and use only KB-level
configuration. Replace the inline call to await self._run_raptor() with a call
to queue_per_doc_raptor_task, retrieving the document object via
DocumentService.get_by_id(task_doc_id) and passing it along with ctx.priority to
the queue function. Wrap the DocumentService call in appropriate error handling
and progress callbacks.
In
`@web/src/pages/chunk/parsed-result/add-knowledge/components/knowledge-chunk/components/document-structure-graph.tsx`:
- Around line 194-257: The document-structure-graph component contains hardcoded
user-facing strings including "Structure graph", "Close", "No generated
structure graph.", "Loading…", "No data for this template.", and the
entity/relation count labels. To fix this, import the useTranslation hook from
react-i18next at the top of the file, call it within the component to get the
translation function t, and replace all hardcoded strings with their
corresponding t() calls using appropriate locale keys (for example,
t('structure_graph.title') for "Structure graph", t('common.close') for "Close",
etc.). Add the corresponding translation keys to your locale JSON files to
support all 17 languages as per the project's i18n guidelines.
In `@web/src/pages/dataset/dataset/generate-button/generate.tsx`:
- Around line 300-315: The delete confirmation dialog in the generate.tsx file
contains ternary operators that only handle GenerateType.KnowledgeGraph and fall
through to RAPTOR for all other types, which causes GenerateType.Artifact to be
incorrectly labeled as RAPTOR. Create a type-label map object that maps each
GenerateType value (KnowledgeGraph, Raptor, and Artifact) to its corresponding
translated label, then replace both the ternary operators in the title
calculation and the type parameter passed to the translation function with
lookups into this map. This ensures each artifact deletion confirmation displays
the correct type label.
---
Nitpick comments:
In `@api/db/services/document_service.py`:
- Around line 1105-1146: The queue_per_doc_raptor_task function creates a
database task and publishes a Redis message but does not emit any logs to track
the operation. Add lightweight logging after the task is inserted into the
database with bulk_insert_into_db to log the task_id and doc_id, and add another
log after the Redis message is queued with REDIS_CONN.queue_product to log the
queue name (derived from priority) along with the task_id. Do not include
parser_config or other verbose data in these logs, keeping them focused on the
task tracking information needed for monitoring and debugging.
In `@web/src/components/chunk-method-dialog/index.tsx`:
- Around line 72-75: The KnowledgeCompilationTemplateSelect function is using
the project-wrapped useTranslate utility instead of the preferred useTranslation
hook from react-i18next. Replace the useTranslate import and call with
useTranslation from react-i18next, passing the 'knowledgeConfiguration'
namespace as an argument to maintain the same translation context.
🪄 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: 9484aacf-8ac2-4f5c-bda3-39bd1e3bdc16
📒 Files selected for processing (20)
api/apps/restful_apis/chunk_api.pyapi/apps/services/dataset_api_service.pyapi/db/services/document_service.pyrag/advanced_rag/knowlege_compile/artifact.pyrag/advanced_rag/knowlege_compile/structure.pyrag/raptor.pyrag/svr/task_executor.pyrag/svr/task_executor_refactor/raptor_service.pyrag/svr/task_executor_refactor/task_handler.pyweb/src/components/chunk-method-dialog/index.tsxweb/src/components/parse-configuration/raptor-form-fields.tsxweb/src/hooks/use-chunk-request.tsweb/src/interfaces/database/dataset-artifact.tsweb/src/pages/chunk/parsed-result/add-knowledge/components/knowledge-chunk/components/document-structure-graph.tsxweb/src/pages/chunk/parsed-result/add-knowledge/components/knowledge-chunk/index.tsxweb/src/pages/dataset/artifact/artifact-graph.tsxweb/src/pages/dataset/dataset-setting/index.tsxweb/src/pages/dataset/dataset/generate-button/generate.tsxweb/src/pages/dataset/knowledge-graph/tree-graph.tsxweb/src/pages/dataset/knowledge-graph/util.ts
✅ Files skipped from review due to trivial changes (1)
- web/src/pages/dataset/dataset-setting/index.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- web/src/interfaces/database/dataset-artifact.ts
- web/src/pages/chunk/parsed-result/add-knowledge/components/knowledge-chunk/index.tsx
- web/src/pages/dataset/artifact/artifact-graph.tsx
Comment on lines
+531
to
+584
| Relations: full bipartite layer-by-layer fan-out — every node at | ||
| layer K gets an edge to every node at layer K-1 (because we only | ||
| loaded ``content_with_weight`` + ``raptor_layer_int`` we don't | ||
| have the specific parent linkage). Self-edges and dangling | ||
| targets are dropped (the latter only matters if the layer-int | ||
| values are non-contiguous). | ||
| """ | ||
| # Build entities. Dedup by id so two identical-content summaries | ||
| # collapse to one node — the canvas can't render multiple nodes | ||
| # at the same id anyway, and identical content is a defensible | ||
| # collapse. | ||
| by_id: Dict[str, Dict] = {} | ||
| by_layer: Dict[int, List[str]] = {} | ||
|
|
||
| for row in rows: | ||
| content = row.get("content_with_weight") | ||
| if not isinstance(content, str) or not content.strip(): | ||
| continue | ||
| try: | ||
| layer = int(row.get("raptor_layer_int") or 0) | ||
| except (TypeError, ValueError): | ||
| layer = 0 | ||
| if layer <= 0: | ||
| # Layer 0 would be the original leaf chunks; RAPTOR | ||
| # summaries start at layer 1. Anything claiming layer 0 | ||
| # here is malformed; skip. | ||
| continue | ||
|
|
||
| name = " ".join(content.split()[:16]) | ||
| nid = xxhash.xxh128( | ||
| content.encode("utf-8", "surrogatepass"), | ||
| ).hexdigest() # 32-char hex | ||
| if nid in by_id: | ||
| continue | ||
| by_id[nid] = { | ||
| "id": nid, | ||
| "name": name, | ||
| "description": content, | ||
| "source_chunk_ids": [], | ||
| } | ||
| by_layer.setdefault(layer, []).append(nid) | ||
|
|
||
| # Layered fan-out from parent (higher layer) → child (lower layer). | ||
| relations: List[Dict] = [] | ||
| layers_sorted = sorted(by_layer.keys()) | ||
| for layer in layers_sorted: | ||
| child_layer = layer - 1 | ||
| if child_layer not in by_layer: | ||
| continue | ||
| for parent in by_layer[layer]: | ||
| for child in by_layer[child_layer]: | ||
| if parent == child: | ||
| continue | ||
| relations.append({"from": parent, "to": child}) |
Contributor
There was a problem hiding this comment.
Use RAPTOR provenance instead of full layer fan-out.
_generate_raptor now persists source_chunk_ids, but the graph loader drops that field and _build_raptor_graph connects every layer K node to every layer K-1 node. That misrepresents unrelated summaries and can create O(parent × child) edges that overwhelm the canvas. Load source_chunk_ids and only connect adjacent-layer summaries with overlapping provenance.
Proposed direction
- select_fields = ["content_with_weight", "raptor_layer_int"]
+ select_fields = ["content_with_weight", "raptor_layer_int", "source_chunk_ids"]- relations.append({"from": parent, "to": child})
+ parent_sources = set(by_id[parent].get("source_chunk_ids") or [])
+ child_sources = set(by_id[child].get("source_chunk_ids") or [])
+ if parent_sources and child_sources and parent_sources.isdisjoint(child_sources):
+ continue
+ relations.append({"from": parent, "to": child})Also applies to: 613-620
🤖 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 `@rag/svr/task_executor_refactor/raptor_service.py` around lines 531 - 584, The
`_build_raptor_graph` method creates a full bipartite layer-by-layer connection
(every node at layer K connects to every node at layer K-1) which misrepresents
unrelated summaries. Instead of connecting all parent-child pairs across layers,
load the `source_chunk_ids` field for each node when processing rows, store it
in the by_id dictionary, and modify the layered fan-out section where relations
are built to only create an edge between a parent and child node if their
source_chunk_ids sets overlap (have at least one common chunk in provenance).
This will replace the full bipartite connection logic with provenance-based
filtering. Apply the same provenance-based filtering logic to the additional
affected site at lines 613-620.
Comment on lines
+108
to
+122
| getContent: (_e: IElementEvent, items: ElementDatum) => { | ||
| if (!Array.isArray(items)) return undefined; | ||
| return items | ||
| .flatMap((item) => [ | ||
| `<div id="${tooltipId}" role="tooltip" aria-label="${item?.id}">`, | ||
| `<h3 class="font-medium">${item?.id}</h3>`, | ||
| item?.entity_type | ||
| ? `<div class="text-xs"><b>Type:</b> ${item.entity_type}</div>` | ||
| : '', | ||
| item?.description | ||
| ? `<p class="text-xs whitespace-pre-wrap">${item.description}</p>` | ||
| : '', | ||
| '</div>', | ||
| ]) | ||
| .join(''); |
Contributor
There was a problem hiding this comment.
Escape tooltip HTML to prevent stored XSS.
item.id, item.entity_type, and item.description are document/LLM-derived values and are interpolated directly into an HTML string. Escape them before returning tooltip content.
Escape untrusted tooltip values
const ROOT_FALLBACK_COLOR = '`#7C3AED`'; // amethyst — distinct from KG palette
+
+const escapeHtml = (value: unknown) =>
+ String(value ?? '').replace(/[&<>"']/g, (char) => {
+ const entities: Record<string, string> = {
+ '&': '&',
+ '<': '<',
+ '>': '>',
+ '"': '"',
+ "'": '&`#39`;',
+ };
+ return entities[char];
+ });
@@
enterable: true,
getContent: (_e: IElementEvent, items: ElementDatum) => {
if (!Array.isArray(items)) return undefined;
return items
- .flatMap((item) => [
- `<div id="${tooltipId}" role="tooltip" aria-label="${item?.id}">`,
- `<h3 class="font-medium">${item?.id}</h3>`,
- item?.entity_type
- ? `<div class="text-xs"><b>Type:</b> ${item.entity_type}</div>`
- : '',
- item?.description
- ? `<p class="text-xs whitespace-pre-wrap">${item.description}</p>`
- : '',
- '</div>',
- ])
+ .flatMap((item) => {
+ const id = escapeHtml(item?.id);
+ const entityType = escapeHtml(item?.entity_type);
+ const description = escapeHtml(item?.description);
+ return [
+ `<div id="${tooltipId}" role="tooltip" aria-label="${id}">`,
+ `<h3 class="font-medium">${id}</h3>`,
+ entityType
+ ? `<div class="text-xs"><b>Type:</b> ${entityType}</div>`
+ : '',
+ description
+ ? `<p class="text-xs whitespace-pre-wrap">${description}</p>`
+ : '',
+ '</div>',
+ ];
+ })
.join('');
},📝 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
| getContent: (_e: IElementEvent, items: ElementDatum) => { | |
| if (!Array.isArray(items)) return undefined; | |
| return items | |
| .flatMap((item) => [ | |
| `<div id="${tooltipId}" role="tooltip" aria-label="${item?.id}">`, | |
| `<h3 class="font-medium">${item?.id}</h3>`, | |
| item?.entity_type | |
| ? `<div class="text-xs"><b>Type:</b> ${item.entity_type}</div>` | |
| : '', | |
| item?.description | |
| ? `<p class="text-xs whitespace-pre-wrap">${item.description}</p>` | |
| : '', | |
| '</div>', | |
| ]) | |
| .join(''); | |
| const escapeHtml = (value: unknown) => | |
| String(value ?? '').replace(/[&<>"']/g, (char) => { | |
| const entities: Record<string, string> = { | |
| '&': '&', | |
| '<': '<', | |
| '>': '>', | |
| '"': '"', | |
| "'": '&`#39`;', | |
| }; | |
| return entities[char]; | |
| }); | |
| getContent: (_e: IElementEvent, items: ElementDatum) => { | |
| if (!Array.isArray(items)) return undefined; | |
| return items | |
| .flatMap((item) => { | |
| const id = escapeHtml(item?.id); | |
| const entityType = escapeHtml(item?.entity_type); | |
| const description = escapeHtml(item?.description); | |
| return [ | |
| `<div id="${tooltipId}" role="tooltip" aria-label="${id}">`, | |
| `<h3 class="font-medium">${id}</h3>`, | |
| entityType | |
| ? `<div class="text-xs"><b>Type:</b> ${entityType}</div>` | |
| : '', | |
| description | |
| ? `<p class="text-xs whitespace-pre-wrap">${description}</p>` | |
| : '', | |
| '</div>', | |
| ]; | |
| }) | |
| .join(''); | |
| }, |
🤖 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/src/pages/dataset/knowledge-graph/tree-graph.tsx` around lines 108 - 122,
In the getContent function within the tree-graph.tsx file, the tooltip content
is being constructed by directly interpolating item.id, item.entity_type, and
item.description into HTML strings without escaping. These document/LLM-derived
values are untrusted and could contain malicious HTML or JavaScript. Escape each
of these three values (item.id, item.entity_type, and item.description) using an
appropriate HTML escaping function before inserting them into the template
strings to prevent stored XSS attacks.
Comment on lines
+201
to
+205
| useEffect(() => { | ||
| if (!isEmpty(data)) { | ||
| render(); | ||
| } | ||
| }, [data, render]); |
Contributor
There was a problem hiding this comment.
Destroy the G6 graph on unmount and empty data.
The component destroys the previous graph only inside render(). Closing the overlay or transitioning to empty data can leave the canvas instance and event handlers alive.
Add lifecycle cleanup
useEffect(() => {
- if (!isEmpty(data)) {
+ if (!isEmpty(annotated.nodes)) {
render();
+ return;
}
- }, [data, render]);
+ graphRef.current?.destroy();
+ graphRef.current = null;
+ }, [annotated.nodes, render]);
+
+ useEffect(() => {
+ return () => {
+ graphRef.current?.destroy();
+ graphRef.current = null;
+ };
+ }, []);📝 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
| useEffect(() => { | |
| if (!isEmpty(data)) { | |
| render(); | |
| } | |
| }, [data, render]); | |
| useEffect(() => { | |
| if (!isEmpty(annotated.nodes)) { | |
| render(); | |
| return; | |
| } | |
| graphRef.current?.destroy(); | |
| graphRef.current = null; | |
| }, [annotated.nodes, render]); | |
| useEffect(() => { | |
| return () => { | |
| graphRef.current?.destroy(); | |
| graphRef.current = null; | |
| }; | |
| }, []); |
🤖 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/src/pages/dataset/knowledge-graph/tree-graph.tsx` around lines 201 - 205,
Add a cleanup function to the useEffect hook that destroys the G6 graph
instance. The cleanup function should be returned from the useEffect and should
call the destroy method on the graph instance (typically stored as a ref or
state variable). This cleanup will run both when the component unmounts and
whenever the data dependency changes to an empty state, preventing the canvas
instance and event handlers from persisting after the component is no longer
needed.
Contributor
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
rag/svr/task_executor_refactor/task_handler.py (2)
64-81:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCollect and dedupe template IDs across both parser-config locations.
This helper returns as soon as it finds root-level IDs, so any legacy/nested
ext.compilation_template_id(s)are ignored; duplicates in the root list also run the same template more than once. Match the API-side merge behavior by accumulating unique IDs from both locations.Suggested fix
def _parser_config_compilation_template_ids(parser_config) -> list[str]: if not isinstance(parser_config, dict): return [] - ids = parser_config.get("compilation_template_ids") - if isinstance(ids, list): - return [str(x).strip() for x in ids if isinstance(x, str) and x.strip()] - legacy = parser_config.get("compilation_template_id") - if isinstance(legacy, str) and legacy.strip(): - return [legacy.strip()] - ext = parser_config.get("ext") - if isinstance(ext, dict): - ids = ext.get("compilation_template_ids") - if isinstance(ids, list): - return [str(x).strip() for x in ids if isinstance(x, str) and x.strip()] - legacy = ext.get("compilation_template_id") - if isinstance(legacy, str) and legacy.strip(): - return [legacy.strip()] - return [] + resolved: list[str] = [] + for candidate_loc in ( + parser_config, + parser_config.get("ext") if isinstance(parser_config.get("ext"), dict) else None, + ): + if not isinstance(candidate_loc, dict): + continue + ids = candidate_loc.get("compilation_template_ids") + if isinstance(ids, list): + for value in ids: + if isinstance(value, str): + value = value.strip() + if value and value not in resolved: + resolved.append(value) + legacy = candidate_loc.get("compilation_template_id") + if isinstance(legacy, str): + legacy = legacy.strip() + if legacy and legacy not in resolved: + resolved.append(legacy) + return resolved🤖 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 `@rag/svr/task_executor_refactor/task_handler.py` around lines 64 - 81, The _parser_config_compilation_template_ids function returns immediately upon finding root-level compilation template IDs, which prevents checking for additional IDs in the ext sub-dictionary, and it also does not deduplicate IDs across locations, allowing the same template to run multiple times. Refactor the function to accumulate template IDs from both the root parser_config level and the ext sub-dictionary (checking both compilation_template_ids and the legacy compilation_template_id fields at each location), then deduplicate the accumulated IDs using a set or similar mechanism to ensure each unique ID appears only once before returning the final deduplicated list.
1198-1219:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPersist artifact pages with the canonical
artifact_*fields.These rows currently use
slug_kwd,title_kwd,page_type_kwd, andoutlinks_kwd, but the artifact contract calls forartifact_slug_kwd,artifact_title_kwd,artifact_page_type_kwd,artifact_kb_id_kwd,artifact_doc_id_kwd,artifact_outlinks_kwd, andartifact_raw_md_kwd. Downstream artifact APIs/search filters expecting the canonical names will miss or partially render these pages. Based on PR objectives, REFINE persists artifact pages withartifact_*metadata fields and raw markdown.Suggested field alignment
rows.append({ "id": row_id, "kb_id": kb_id_str, + "artifact_kb_id_kwd": kb_id_str, "doc_id": kb_id_str, # sentinel; KB-scoped row, real provenance in source_doc_ids "compile_kwd": "artifact_page", - "slug_kwd": slug, - "title_kwd": title, - "page_type_kwd": page.get("page_type") or "concept", + "artifact_slug_kwd": slug, + "artifact_title_kwd": title, + "artifact_page_type_kwd": page.get("page_type") or "concept", "entity_names_kwd": list(page.get("entity_names") or []), - "outlinks_kwd": list(page.get("outlinks") or []), + "artifact_outlinks_kwd": list(page.get("outlinks") or []), "outlinks_int": len(list(page.get("outlinks") or [])), "related_kb_pages_kwd": list(page.get("related_kb_pages") or []), "source_chunk_ids": list(page.get("source_chunk_ids") or []), "source_doc_ids": list(page.get("source_doc_ids") or []), + "artifact_doc_id_kwd": list(page.get("source_doc_ids") or []), + "artifact_raw_md_kwd": page.get("content_md_raw") or page.get("content_md") or "", "content_with_weight": content_md,🤖 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 `@rag/svr/task_executor_refactor/task_handler.py` around lines 1198 - 1219, The artifact page rows dictionary needs to use the canonical artifact field names to match the artifact contract and ensure downstream APIs can properly access these fields. In the rows.append block, rename the following fields: change slug_kwd to artifact_slug_kwd, title_kwd to artifact_title_kwd, page_type_kwd to artifact_page_type_kwd, and outlinks_kwd to artifact_outlinks_kwd. Additionally, add three new fields: artifact_kb_id_kwd with value kb_id_str, artifact_doc_id_kwd with value kb_id_str, and artifact_raw_md_kwd with the raw markdown content (likely content_md based on existing context). This ensures the persisted artifact pages conform to the expected contract and will be properly recognized by downstream artifact search and rendering systems.rag/advanced_rag/knowlege_compile/structure.py (4)
220-256:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftRestore the list/set extraction path.
Line 706 always builds hypergraph prompts, and Line 604 always calls
_struct_extract_hypergraph. For documentedlist/setconfigs that provideoutput.fieldsandguideline.rules, Lines 237-256 ignore those fields and instead produce an empty entity skeleton, so those modes can emit empty graph-shaped rows instead of the configured list/set items.Also applies to: 604-611, 706-707
🤖 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 `@rag/advanced_rag/knowlege_compile/structure.py` around lines 220 - 256, The code currently always routes to hypergraph prompt generation regardless of whether a list/set extraction mode is configured. For documented list/set configs that provide output.fields and guideline.rules, the _struct_hypergraph_prompts function and its callers should check the extraction mode and conditionally use an appropriate extraction path instead of always calling _struct_extract_hypergraph. Identify where the extraction mode is determined (check for list/set versus hypergraph configuration in output or parser_config), then conditionally branch at the call sites (around line 604 where _struct_extract_hypergraph is called and around line 706 where _struct_hypergraph_prompts is called) to use the list/set extraction logic for those modes while preserving the hypergraph path for graph-based configurations.
917-926:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve template metadata when rebuilding merged docs.
_struct_rebuild_es_docrebuilds merged rows withoutcompilation_template_idsorcompilation_template_kind_kwd. A locally merged survivor can then be inserted without template scope, and ES-merge updates cannot preserve those fields because they are not selected intoold_doc.🛠️ Proposed fix
select_fields = [ "id", "content_with_weight", "source_id", "knowledge_graph_kwd", "compile_kwd", - "doc_id", "from_entity_kwd", "to_entity_kwd", + "doc_id", "from_entity_kwd", "to_entity_kwd", + "compilation_template_ids", "compilation_template_kind_kwd", ] @@ - new_doc = _struct_to_es_doc( + template_ids = base_doc.get("compilation_template_ids") + template_kind = base_doc.get("compilation_template_kind_kwd") + + new_doc = _struct_to_es_doc( payload=payload, compile_kwd=base_doc.get("compile_kwd"), doc_id=base_doc.get("doc_id"), chunk_ids=chunk_ids, vec=vec, kind=kind, src_field=src_field, target_field=target_field, + compilation_template_id=_struct_doc_template_id(base_doc), + compilation_template_kind=template_kind, ) + if template_ids: + new_doc["compilation_template_ids"] = ( + template_ids if isinstance(template_ids, list) else [template_ids] + )Also applies to: 1061-1064
🤖 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 `@rag/advanced_rag/knowlege_compile/structure.py` around lines 917 - 926, The _struct_rebuild_es_doc function is not preserving compilation_template_ids and compilation_template_kind_kwd when rebuilding merged documents. At both the _struct_to_es_doc call around line 917-926 and the corresponding call around line 1061-1064, extract the compilation_template_ids and compilation_template_kind_kwd from base_doc and pass them as additional parameters to the _struct_to_es_doc function calls. You may need to add these parameters to the _struct_to_es_doc function signature if they are not already defined, ensuring the template metadata is preserved during document reconstruction.
1273-1274:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReturn
graphsin the empty-input summary too.The non-empty path now returns
graphs, but the early return omits it. Callers that consume the new summary shape can hit aKeyErroronly when there are no docs to merge.🐛 Proposed fix
if not docs: - return {"inserted": 0, "updated": 0, "duplicates_dropped": 0} + return {"inserted": 0, "updated": 0, "duplicates_dropped": 0, "graphs": 0}🤖 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 `@rag/advanced_rag/knowlege_compile/structure.py` around lines 1273 - 1274, The early return statement when `not docs` is true returns a dictionary missing the "graphs" key that the non-empty code path includes in its return value. This causes callers expecting the complete summary shape to hit a KeyError only when there are no documents to merge. Add the "graphs" key to the dictionary returned in the early return statement (at lines 1273-1274) to match the structure of the non-empty return path.
1139-1166:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse stored relation endpoints when rebuilding graph edges.
Graph rebuild only selects
content_with_weight,knowledge_graph_kwd, andsource_id, then_struct_graph_relationreads onlysource/src/fromandtarget/tgt/tofrom the payload. Relations extracted with configured member fields can already havefrom_entity_kwd/to_entity_kwd, but those stored endpoints are ignored here, so valid edges are dropped from the graph JSON.🛠️ Proposed fix
-def _struct_graph_relation(payload: dict) -> dict | None: - src = payload.get("source") or payload.get("src") or payload.get("from") - tgt = payload.get("target") or payload.get("tgt") or payload.get("to") +def _struct_graph_relation(payload: dict, row: dict | None = None) -> dict | None: + row = row or {} + src = payload.get("source") or payload.get("src") or payload.get("from") or row.get("from_entity_kwd") + tgt = payload.get("target") or payload.get("tgt") or payload.get("to") or row.get("to_entity_kwd") @@ - fields = ["content_with_weight", "knowledge_graph_kwd", "source_id"] + fields = [ + "content_with_weight", + "knowledge_graph_kwd", + "source_id", + "from_entity_kwd", + "to_entity_kwd", + ] @@ - relation = _struct_graph_relation(payload) + relation = _struct_graph_relation(payload, row)🤖 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 `@rag/advanced_rag/knowlege_compile/structure.py` around lines 1139 - 1166, The graph rebuild logic in the search section is not fetching or using the stored relation endpoint fields (from_entity_kwd/to_entity_kwd) that are configured for relations. Update the fields list to include the stored endpoint field names alongside content_with_weight, knowledge_graph_kwd, and source_id, then modify the _struct_graph_relation function or the logic that processes it to prioritize using these stored endpoints from the payload when they are available, falling back to source/src/from and target/tgt/to only when the stored endpoints are missing. This ensures that valid edges with configured member fields are preserved in the graph JSON during rebuild.
🧹 Nitpick comments (2)
test/unit_test/api/db/services/test_task_service_chunking_counter.py (1)
27-48: ⚡ Quick winAdd priority markers to the new pytest tests. The repository test guideline requires p1/p2/p3 markers, but the new tests are unmarked.
test/unit_test/api/db/services/test_task_service_chunking_counter.py#L27-L48: importpytestand mark both new counter tests with the appropriate priority, e.g.@pytest.mark.p2.test/unit_test/rag/svr/task_executor_refactor/test_raptor_service.py#L166-L190: marktest_build_raptor_graph_preserves_source_chunk_idswith the appropriate priority.test/unit_test/rag/svr/task_executor_refactor/test_task_handler_post_chunking_gate.py#L22-L63: mark both async post-chunking gate tests with the appropriate priority.As per coding guidelines,
test/**/*.py: Use pytest with priority markers (p1/p2/p3) for Python testing.🤖 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 `@test/unit_test/api/db/services/test_task_service_chunking_counter.py` around lines 27 - 48, The new test functions are missing the required pytest priority markers (p1/p2/p3) as per repository testing guidelines. In test/unit_test/api/db/services/test_task_service_chunking_counter.py (lines 27-48), add pytest import at the top and decorate both test_credit_doc_chunking_task_decrements_once and test_clear_doc_chunking_counter_deletes_pending_key with `@pytest.mark.p2`. In test/unit_test/rag/svr/task_executor_refactor/test_raptor_service.py (lines 166-190), add the same decorator to test_build_raptor_graph_preserves_source_chunk_ids. In test/unit_test/rag/svr/task_executor_refactor/test_task_handler_post_chunking_gate.py (lines 22-63), add the decorator to both async post-chunking gate test functions. Add pytest import to any of these files that lack it already.Source: Coding guidelines
api/apps/restful_apis/chunk_api.py (1)
737-753: ⚡ Quick winAdd an audit log for structure-graph deletions.
This new destructive flow deletes graph/entity/relation rows but only returns the count to the caller. Log
dataset_id,document_id,template_id, anddeletedafter the delete branch so operators can trace accidental graph-tab removals. As per coding guidelines,**/*.py: Add logging for new flows.Suggested logging addition
- return get_result(data={"deleted": deleted}, message=f"deleted {deleted} structure graph rows") + logging.info( + "structure_graph_delete: dataset=%s document=%s template=%s deleted=%s", + dataset_id, + document_id, + template_id, + deleted, + ) + return get_result(data={"deleted": deleted}, message=f"deleted {deleted} structure graph rows")🤖 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 `@api/apps/restful_apis/chunk_api.py` around lines 737 - 753, The delete operations for structure graphs lack audit logging to track destructive operations. After the deletion operations complete (after both _delete calls in the non-raptor branch), add logging that captures dataset_id, document_id, template_id, and the deleted count. This audit log should be added before the final return statement in the non-raptor case to ensure all structure graph deletions are traceable for operational purposes.Source: Coding guidelines
🤖 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 `@api/db/services/task_service.py`:
- Around line 71-78: The set_if_absent call on _doc_chunking_done_key and the
decrby call on _doc_chunking_pending_key are separate Redis operations, creating
a race condition where a failure in decrby after a successful set_if_absent will
prevent retries from decrementing the counter. Combine these two operations into
a single atomic Redis operation using a Lua script that checks the done key
existence, sets it if absent with the specified TTL, and atomically decrements
the pending counter, all in one transaction, ensuring both operations succeed or
fail together and retries can safely re-execute the script without skipping
document post-processing.
In `@rag/advanced_rag/knowlege_compile/structure.py`:
- Around line 403-405: The function _struct_union_chunk_ids is being called with
only one argument at line 405, but its signature requires two parameters (a and
b). Update the call to _struct_union_chunk_ids to include both required
arguments by identifying what the second argument should be based on the
function's definition and the surrounding context of how source_chunk_ids should
be merged with another set of chunk IDs. Apply the same fix at the second
affected location mentioned in the comment (line 1169) where
_struct_union_chunk_ids is also called with insufficient arguments.
In `@rag/svr/task_executor_refactor/task_handler.py`:
- Around line 1084-1100: The check for `compile_kwd` in the row data is
ineffective because `compile_kwd` is not included in the `select_fields` being
requested from the search function. Add `compile_kwd` to the `select_fields`
parameter passed to the thread_pool_exec call for the docStoreConn.search method
so that the field is actually returned in the results, allowing the subsequent
`if row.get("compile_kwd"): continue` check to properly filter out previously
compiled rows and prevent contamination of the chunk streaming results.
---
Outside diff comments:
In `@rag/advanced_rag/knowlege_compile/structure.py`:
- Around line 220-256: The code currently always routes to hypergraph prompt
generation regardless of whether a list/set extraction mode is configured. For
documented list/set configs that provide output.fields and guideline.rules, the
_struct_hypergraph_prompts function and its callers should check the extraction
mode and conditionally use an appropriate extraction path instead of always
calling _struct_extract_hypergraph. Identify where the extraction mode is
determined (check for list/set versus hypergraph configuration in output or
parser_config), then conditionally branch at the call sites (around line 604
where _struct_extract_hypergraph is called and around line 706 where
_struct_hypergraph_prompts is called) to use the list/set extraction logic for
those modes while preserving the hypergraph path for graph-based configurations.
- Around line 917-926: The _struct_rebuild_es_doc function is not preserving
compilation_template_ids and compilation_template_kind_kwd when rebuilding
merged documents. At both the _struct_to_es_doc call around line 917-926 and the
corresponding call around line 1061-1064, extract the compilation_template_ids
and compilation_template_kind_kwd from base_doc and pass them as additional
parameters to the _struct_to_es_doc function calls. You may need to add these
parameters to the _struct_to_es_doc function signature if they are not already
defined, ensuring the template metadata is preserved during document
reconstruction.
- Around line 1273-1274: The early return statement when `not docs` is true
returns a dictionary missing the "graphs" key that the non-empty code path
includes in its return value. This causes callers expecting the complete summary
shape to hit a KeyError only when there are no documents to merge. Add the
"graphs" key to the dictionary returned in the early return statement (at lines
1273-1274) to match the structure of the non-empty return path.
- Around line 1139-1166: The graph rebuild logic in the search section is not
fetching or using the stored relation endpoint fields
(from_entity_kwd/to_entity_kwd) that are configured for relations. Update the
fields list to include the stored endpoint field names alongside
content_with_weight, knowledge_graph_kwd, and source_id, then modify the
_struct_graph_relation function or the logic that processes it to prioritize
using these stored endpoints from the payload when they are available, falling
back to source/src/from and target/tgt/to only when the stored endpoints are
missing. This ensures that valid edges with configured member fields are
preserved in the graph JSON during rebuild.
In `@rag/svr/task_executor_refactor/task_handler.py`:
- Around line 64-81: The _parser_config_compilation_template_ids function
returns immediately upon finding root-level compilation template IDs, which
prevents checking for additional IDs in the ext sub-dictionary, and it also does
not deduplicate IDs across locations, allowing the same template to run multiple
times. Refactor the function to accumulate template IDs from both the root
parser_config level and the ext sub-dictionary (checking both
compilation_template_ids and the legacy compilation_template_id fields at each
location), then deduplicate the accumulated IDs using a set or similar mechanism
to ensure each unique ID appears only once before returning the final
deduplicated list.
- Around line 1198-1219: The artifact page rows dictionary needs to use the
canonical artifact field names to match the artifact contract and ensure
downstream APIs can properly access these fields. In the rows.append block,
rename the following fields: change slug_kwd to artifact_slug_kwd, title_kwd to
artifact_title_kwd, page_type_kwd to artifact_page_type_kwd, and outlinks_kwd to
artifact_outlinks_kwd. Additionally, add three new fields: artifact_kb_id_kwd
with value kb_id_str, artifact_doc_id_kwd with value kb_id_str, and
artifact_raw_md_kwd with the raw markdown content (likely content_md based on
existing context). This ensures the persisted artifact pages conform to the
expected contract and will be properly recognized by downstream artifact search
and rendering systems.
---
Nitpick comments:
In `@api/apps/restful_apis/chunk_api.py`:
- Around line 737-753: The delete operations for structure graphs lack audit
logging to track destructive operations. After the deletion operations complete
(after both _delete calls in the non-raptor branch), add logging that captures
dataset_id, document_id, template_id, and the deleted count. This audit log
should be added before the final return statement in the non-raptor case to
ensure all structure graph deletions are traceable for operational purposes.
In `@test/unit_test/api/db/services/test_task_service_chunking_counter.py`:
- Around line 27-48: The new test functions are missing the required pytest
priority markers (p1/p2/p3) as per repository testing guidelines. In
test/unit_test/api/db/services/test_task_service_chunking_counter.py (lines
27-48), add pytest import at the top and decorate both
test_credit_doc_chunking_task_decrements_once and
test_clear_doc_chunking_counter_deletes_pending_key with `@pytest.mark.p2`. In
test/unit_test/rag/svr/task_executor_refactor/test_raptor_service.py (lines
166-190), add the same decorator to
test_build_raptor_graph_preserves_source_chunk_ids. In
test/unit_test/rag/svr/task_executor_refactor/test_task_handler_post_chunking_gate.py
(lines 22-63), add the decorator to both async post-chunking gate test
functions. Add pytest import to any of these files that lack it already.
🪄 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: f6a08a5f-5de8-49f1-b902-4912a6e2cef4
📒 Files selected for processing (13)
api/apps/restful_apis/chunk_api.pyapi/db/services/task_service.pyrag/advanced_rag/knowlege_compile/structure.pyrag/raptor.pyrag/svr/task_executor_refactor/raptor_service.pyrag/svr/task_executor_refactor/task_handler.pyrag/utils/redis_conn.pytest/unit_test/api/db/services/test_task_service_chunking_counter.pytest/unit_test/rag/svr/task_executor_refactor/test_raptor_service.pytest/unit_test/rag/svr/task_executor_refactor/test_task_handler_post_chunking_gate.pyweb/src/hooks/use-chunk-request.tsweb/src/pages/chunk/parsed-result/add-knowledge/components/knowledge-chunk/components/document-structure-graph.tsxweb/src/services/knowledge-service.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- web/src/services/knowledge-service.ts
- web/src/pages/chunk/parsed-result/add-knowledge/components/knowledge-chunk/components/document-structure-graph.tsx
- rag/raptor.py
- rag/svr/task_executor_refactor/raptor_service.py
Comment on lines
+71
to
+78
| first_credit = REDIS_CONN.set_if_absent( | ||
| _doc_chunking_done_key(task_id), | ||
| "1", | ||
| exp=DOC_CHUNKING_COUNTER_TTL_SECONDS, | ||
| ) | ||
| if not first_credit: | ||
| return 1 | ||
| return REDIS_CONN.decrby(_doc_chunking_pending_key(doc_id), 1) |
Contributor
There was a problem hiding this comment.
Make task crediting atomic.
set_if_absent(done_key) and decrby(pending_key) are separate Redis operations. If the sentinel write succeeds but decrby fails, retries will see the done key and never decrement the pending counter, so document post-processing can be skipped permanently for that run.
Safer direction
- first_credit = REDIS_CONN.set_if_absent(
- _doc_chunking_done_key(task_id),
- "1",
- exp=DOC_CHUNKING_COUNTER_TTL_SECONDS,
- )
- if not first_credit:
- return 1
- return REDIS_CONN.decrby(_doc_chunking_pending_key(doc_id), 1)
+ # Use one Redis Lua/scripted operation here:
+ # 1. SET done_key NX EX ttl
+ # 2. if newly set, DECR pending_key
+ # 3. keep/refresh pending_key TTL if needed
+ # 4. return the resulting pending count
+ return REDIS_CONN.credit_chunking_task_once(
+ _doc_chunking_done_key(task_id),
+ _doc_chunking_pending_key(doc_id),
+ DOC_CHUNKING_COUNTER_TTL_SECONDS,
+ )🤖 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 `@api/db/services/task_service.py` around lines 71 - 78, The set_if_absent call
on _doc_chunking_done_key and the decrby call on _doc_chunking_pending_key are
separate Redis operations, creating a race condition where a failure in decrby
after a successful set_if_absent will prevent retries from decrementing the
counter. Combine these two operations into a single atomic Redis operation using
a Lua script that checks the done key existence, sets it if absent with the
specified TTL, and atomically decrements the pending counter, all in one
transaction, ensuring both operations succeed or fail together and retries can
safely re-execute the script without skipping document post-processing.
Comment on lines
+403
to
+405
| if isinstance(source_chunk_ids, str): | ||
| source_chunk_ids = [source_chunk_ids] | ||
| source_chunk_ids = _struct_union_chunk_ids(source_chunk_ids) |
Contributor
There was a problem hiding this comment.
Fix the one-argument call to _struct_union_chunk_ids.
Line 405 calls _struct_union_chunk_ids(source_chunk_ids), but the helper requires both a and b. Any graph rebuild that reaches Line 1169 for an entity row will raise TypeError and skip graph generation.
🐛 Proposed fix
-def _struct_graph_entity(payload: dict, source_chunk_ids: list | None = None) -> dict | None:
+def _struct_graph_entity(payload: dict, source_chunk_ids: list | str | None = None) -> dict | None:
@@
- if isinstance(source_chunk_ids, str):
- source_chunk_ids = [source_chunk_ids]
- source_chunk_ids = _struct_union_chunk_ids(source_chunk_ids)
+ if source_chunk_ids is None:
+ source_chunk_ids = []
+ elif isinstance(source_chunk_ids, str):
+ source_chunk_ids = [source_chunk_ids]
+ source_chunk_ids = _struct_union_chunk_ids(source_chunk_ids, [])Also applies to: 1169-1169
🤖 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 `@rag/advanced_rag/knowlege_compile/structure.py` around lines 403 - 405, The
function _struct_union_chunk_ids is being called with only one argument at line
405, but its signature requires two parameters (a and b). Update the call to
_struct_union_chunk_ids to include both required arguments by identifying what
the second argument should be based on the function's definition and the
surrounding context of how source_chunk_ids should be merged with another set of
chunk IDs. Apply the same fix at the second affected location mentioned in the
comment (line 1169) where _struct_union_chunk_ids is also called with
insufficient arguments.
Comment on lines
+1084
to
+1100
| res = await thread_pool_exec( | ||
| settings.docStoreConn.search, | ||
| select_fields, [], {"doc_id": [doc_id], "available_int": 1}, | ||
| [], order_by, offset, batch_size, | ||
| index_nm, [kb_id], | ||
| ) | ||
| field_map = settings.docStoreConn.get_fields(res, select_fields) | ||
| except Exception: | ||
| logging.exception("load_chunks_for_doc: failed to load chunks for doc=%s", doc_id) | ||
| return | ||
| if not field_map: | ||
| return | ||
|
|
||
| batch: List[Dict] = [] | ||
| for row_id, row in field_map.items(): | ||
| if row.get("compile_kwd"): | ||
| continue |
Contributor
There was a problem hiding this comment.
Actually exclude compiled rows from chunk streaming.
compile_kwd is not selected and the search condition does not include must_not exists: compile_kwd, so row.get("compile_kwd") is always empty. Re-runs can feed previously compiled graph/artifact rows back into MAP/structure compilation if they match doc_id and available_int, contaminating generated results.
Suggested fix
select_fields = [
"id", "doc_id", "content_with_weight",
- "page_num_int", "top_int",
+ "page_num_int", "top_int", "compile_kwd",
]
@@
settings.docStoreConn.search,
- select_fields, [], {"doc_id": [doc_id], "available_int": 1},
+ select_fields,
+ [],
+ {
+ "doc_id": [doc_id],
+ "available_int": 1,
+ "must_not": {"exists": "compile_kwd"},
+ },🤖 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 `@rag/svr/task_executor_refactor/task_handler.py` around lines 1084 - 1100, The
check for `compile_kwd` in the row data is ineffective because `compile_kwd` is
not included in the `select_fields` being requested from the search function.
Add `compile_kwd` to the `select_fields` parameter passed to the
thread_pool_exec call for the docStoreConn.search method so that the field is
actually returned in the results, allowing the subsequent `if
row.get("compile_kwd"): continue` check to properly filter out previously
compiled rows and prevent contamination of the chunk streaming results.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Builds out a four-phase artifact compilation pipeline (MAP → REDUCE → PLAN → REFINE) alongside the existing
compile_structure_from_textextractor, and consolidates the plumbing both pipelines share into a small_commonmodule so neither file carries the same code twice.The artifact pipeline lives in
rag/advanced_rag/knowlege_compile/artifact.py(renamed fromwiki.py— bothgit mvandcase-preserving content rename). It turns a KB's ingested chunks into a hyperlinked set of artifact pages, each backed by ES rows that retrievers can serve.Pipeline phases
artifact_map_from_chunksartifact_map_extractrow per source chunkartifact_reduce_from_extractsartifact_reduce_resultrow with canonical entities/conceptsartifact_plan_from_reductionartifact_compilation_planrow withpages[]artifact_refine_from_planartifact_pagerows + non-searchableartifact_page_draftcache rowsEntity / relation schemas and the prompt's Rules section come from
parser_config[\"artifact_compilation\"](same YAML shapecompile_structure_from_textaccepts).source_chunk_idis always appended so chunk attribution survives whatever the user defines.Shared engines in
_common.pybuild_chunk_batches+run_chunked_pipeline— generic chunked-LLM scaffold used by bothcompile_structure_from_textandartifact_map_from_chunks.bulk_dedup_items— three-phase dedup (exact → embedding cosine → LLM disambiguation), used by REDUCE.stable_row_id,encode,tokenize_for_search,union_ordered,make_input_budget,ensure_llm_bundle,es_search/es_insert/es_delete/es_upsert_one,find_vec_field.REFINE / persistence
artifact_pagerows carryartifact_slug_kwd,artifact_title_kwd,artifact_page_type_kwd,artifact_kb_id_kwd,artifact_doc_id_kwd(list of contributing docs),artifact_outlinks_kwd(slugs linked from this page), plusartifact_raw_md_kwd(the LLM's[[slug]]form, used by the merger) andcontent_with_weight(the rendered form with clickable[text](artifact/{kb_id}/{slug})links). UPDATE pages are LLM-merged against the existing content with a 70 % shrink-check fallback.Task-executor wiring
task_handler.py::TaskHandler._artifact_compilationchains the four phases underparser_config[\"toc_extraction\"]for now. The call runs after chunk insertion so REFINE's chunk-by-id lookup resolves the source rows in ES._artifact_load_chunks_by_idfalls back to per-iddocStoreConn.get()for whatever the batch search misses, with diagnostic warnings so future cross-backend filter quirks are debuggable.Test plan
parser_config[\"toc_extraction\"]=trueruns end-to-end on a single doc;artifact_pagerows are produced with non-emptysource_chunk_idsandartifact_doc_id_kwd.artifact_pagewhosesource_doc_idslists both contributing docs.[text](artifact/{kb_id}/{slug})link in a rendered page resolves to anotherartifact_pagerow in ES via(artifact_kb_id_kwd, artifact_slug_kwd).compile_structure_from_text(list/set/hypergraph) still passing the existing extractor tests — i.e. the_commonextraction didn't change behaviour.merge_compiled_structures(deliberately not migrated tobulk_dedup_itemsbecause the algorithm differs; left in place).Caveats
wiki_*compile_kwdvalues is orphaned._artifact_compilationdeletes the five old kwds at the top of each run so the next invocation produces fresh, artifact-keyed data — but any front-end that intercepts the old link prefix needs the path migration too (wiki/{kb}/{slug}→artifact/{kb}/{slug}).compile_structure_from_text's public surface beyond having it consume the shared_commonhelpers.🤖 Generated with Claude Code