Repository navigation
[Feat]: tighten KB Indexed badge + folder cascade delete + pmid attribution - #47
Conversation
… drop shared instance state CapturingHandler.currentParentMessageId was a single instance field overwritten by every outermost handleChainStart. Concurrent invokes (main chat + a background_agent / kbAgent dispatched mid-turn) clobbered each other, and inner spans inherited whichever message arrived last — kb-upload inner spans got attributed to the user's NEXT message instead of the trigger. Drop the instance field. handleChainStart now resolves parent_message_id per-call: prefer metadata.parent_message_id (per-run, set by runs.create on kbAgent / background_agent dispatches), fallback to lastHumanMessageId(inputs.messages) for the chat streaming path. Inner LLM / tool spans that don't get either source stay null at capture; bulkInsertSpans' state-fallback backfill fills them at INSERT time from langGraphClient.threads.getState. No shared state, no clobber race.
…hip rows ChunksStatusBadge used to flip to 'Indexed' as soon as every chunk had status='success'. status='success' alone doesn't mean the chunk is queryable: pgvector needs the vector, and the entity-leg of hybrid search needs kb_entity / kb_relationship rows. A doc with all vectors done but no entities yet (extraction still running) used to be 'Indexed' — now it sits in 'Embedding'. Redefine successChunks to mean status='success' AND embedding IS NOT NULL. Add embeddingPendingChunks + entityCount + relationshipCount aggregates (two new subqueries on kb_entity / kb_relationship grouped by document_id). Badge collapses three in-flight lanes into 'Extracting' (chunks being OCR'd / split) and 'Embedding' (chunks done, vector or graph-extract still running); 'Indexed' now requires all three legs of hybrid search populated.
- skipFileToPageNode + pageToMarkdownNode also skip on retryFailed / retryFailedChunks (was chunksOnly only); doc-status preservation test - prepareKBDataNode reads config.configurable.parent_message_id first with lastHumanMessageId fallback - DELETE /api/kb/folders/[id] accepts ?deleteAll=true to cascade-delete every doc in the folder (each doc's chunks/embeddings/attachments) before the folder row; Settings dialog adds a switch + typed "delete all" confirmation - DocDeleteDialog / DocReprocessDialog close themselves via onOpenChange before firing onDeleted / onReprocessed; long titles use break-all - ObservabilityPopover no longer auto-closes on row click — the Sheet is the singleton entry, not the popover - kb-mention popover tweaks: scrollable body, sticky header, padding - folder-sidebar callbacks no longer reset selection on submit (the dialog does it via onOpenChange) - docs: APIS.md documents the new indexing-count fields on GET /api/kb/documents + GET /api/kb/documents/[id], the chunks[].hasEmbedding flag, and the folder ?deleteAll cascade. OBSERVABILITY.md documents per-call pmid resolution from metadata.parent_message_id + langGraphClient.threads.getState fallback. TOOLS.md documents the expanded list_documents payload shape shared with the Settings badges.
- add shrink-0 to icon + min-w-0 max-w-full to badge shell so the
fixed-width badge column in DocRow doesn't push siblings off-grid
- truncate min-w-0 on the label span so long status text ("Parsing
(12/34)") ellipses instead of forcing horizontal scroll
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Review summaryBig PR — three meaningful changes (KB Indexed badge tightening + folder cascade delete + observability pmid Critical (block merge)
High (should fix before merge)
Medium (nice-to-fix)
Low
Good
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 30 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
- lib/kb/queries.ts: deleteKbDocumentForUser + deleteKbFolderForUser
({deleteAllDocs}) now drop the attachment rows for each cascaded
doc. R2 objects still stay (sha-keyed, ref-counted by future same-
content uploads — see CLAUDE.md / docs/ATTACHMENTS.md). The orphan
attachment pointer was the only thing missing from the cascade.
- app/api/kb/documents/[id]/route.ts: replace the leftJoin(kbEntity,
kbRelationship) graph-counts query with two single-table subqueries.
The leftJoin cartesian-multiplied both counts whenever a doc had rows
in both tables, and reported 0/0 when a doc had relationships but no
entities (the from-table was empty). The subqueries count each
independently.
- backend/node/kb/page-to-markdown-node.ts: remove retryFailed from
the cached-pages pass-through guard. retryFailed's whole point is to
re-OCR the failed pages, so this node has to run. chunksOnly and
retryFailedChunks still skip (their pages have valid cached markdown).
- tests: add 7 cases — folder cascade attachment cleanup, graph-count
join corruption (3 shapes: empty / both / rel-only), pageToMarkdown
per-mode skip/run assertions (chunksOnly / retryFailedChunks skip;
retryFailed runs OCR).
| // thread implicitly closes the wait gap that opened on the previous | ||
| // tool's interrupt. | ||
|
|
||
| console.warn( |
There was a problem hiding this comment.
🛑 This looks like leftover debug code that needs to be removed before merge.
console.warn(
2222,
rows.map((item) => ({ threadId: item.threadId, parentMessageId: item.parentMessageId })),
);Two problems:
2222is a magic marker with no function — clearly a personal "find me later" note from the author. Won't survivepnpm lintonce configured forno-console/ no magic numbers, and pollutes production logs forever.- The payload itself is noise — emitting every batch's
threadId + parentMessageIdto stderr on every span flush will fire many times per turn and across every background invoke. None of it has a consumer; the human-readable test would be easier with a single labeled log.
Also worth noting: parentMessageId on this row set is now the post-backfill value (backfillParentMessageIds mutated r.parentMessageId in place above this line on line 121), so the dump won't even surface "captured vs filled" deltas — it's just confirming the backfill did its job on the happy path.
Recommended fix: delete the three lines entirely. If a trace is needed, log a count (e.g. console.warn("[observability] bulkInsert", rows.length, "spans")) without any identifiers.
| char="@" | ||
| adapter={bundle.adapter} | ||
| className="aui-composer-trigger-popover bg-popover text-popover-foreground absolute bottom-full left-0 z-50 mb-4 w-72 max-h-72 overflow-y-auto rounded-xl border bg-background p-1 shadow-lg flex flex-col gap-0.5" | ||
| className="aui-composer-trigger-popover bg-popover text-popover-foreground absolute bottom-full left-0 z-50 mb-4 w-72 max-h-68 overflow-y-auto rounded-xl border bg-background px-1 shadow-lg flex flex-col gap-0.5 pb-1" |
There was a problem hiding this comment.
🐛 max-h-68 is not in the Tailwind v4 default spacing scale — the scale goes ..., 56, 60, 64, 72, 80, 96. With no arbitrary value config this class generates no CSS, so the popover won't constrain its height and can overflow the composer area on short viewports.
The pre-PR value max-h-72 (18rem) was correct. The commit message in 9616f02 (status-badge column layout) doesn't mention this popover file, so this looks like an unrelated near-typo from the concurrent edits.
| className="aui-composer-trigger-popover bg-popover text-popover-foreground absolute bottom-full left-0 z-50 mb-4 w-72 max-h-68 overflow-y-auto rounded-xl border bg-background px-1 shadow-lg flex flex-col gap-0.5 pb-1" | |
| className="aui-composer-trigger-popover bg-popover text-popover-foreground absolute bottom-full left-0 z-50 mb-4 w-72 max-h-72 overflow-y-auto rounded-xl border bg-background px-1 shadow-lg flex flex-col gap-0.5 pb-1" |
| // outputs ready: vector + entity/relationship. If chunks all have | ||
| // vectors but no entities/relationships yet, that's still "Embedding" | ||
| // — the entity-leg of hybrid search has nothing to score against. | ||
| const terminal = success + failed + embeddingPending; |
There was a problem hiding this comment.
📋 Subtle behavioral change worth double-checking with a real example:
Old logic: terminal = success + failed, so a doc with success=5, failed=2, embeddingPending=3, total=10 had terminal=7 → fell into the Indexing (5/10) in-progress path.
New logic (this PR): terminal = success + failed + embeddingPending = 10. Combined with isFailed = failed > 0 && terminal === total, that same doc now flips to the 2 failed badge — even though only 2 of the 10 chunks actually errored, and the other 3 just haven't been vectored yet. The tooltip only mentions the 2 failed chunks and drops the 3 embedding-pending ones.
Net effect: a doc that was "still indexing, 5/10 done" becomes "2 failed", which is more alarming than accurate. The same happens for any mixed state where the user has at least one failed chunk.
Two options:
- Refine
isFailedso a non-zeroembeddingPendingkeeps the doc in the in-progress lane and the badge reflects "mixed: X done, Y embedding-pending, Z failed" — e.g. only setisFailedwhenembeddingPending === 0. - Update the failed-state tooltip to enumerate both buckets so the count in
failedmatches the user's mental model:failed + embeddingPendingchunks outstanding.
The fresh tests cover OCR-failure recovery (good), but there's no test for the mixed (failed>0, embeddingPending>0, total>success) case. Adding one before this lands would prevent regressions either way.
| const missingThreads = new Set<string>(); | ||
| for (const r of rows) { | ||
| if (r.parentMessageId == null) missingThreads.add(r.threadId); | ||
| if (r.parentMessageId == null) { |
There was a problem hiding this comment.
🔁 This fallback is dead code on the happy path. toRow() (lines 65-66) already lifts meta?.parent_message_id into r.parentMessageId whenever it's a non-empty string:
const rawPmid = meta?.parent_message_id;
const parentMessageId =
typeof rawPmid === "string" && rawPmid.length > 0 ? rawPmid : null;So at this point in backfillParentMessageIds, r.parentMessageId == null ⇒ meta?.parent_message_id was either missing, non-string, or empty-string. The only case the new check rescues vs. the existing logic is the empty-string case (""), and zeroing out an explicit empty-string pmid back to "" doesn't improve anything — toCapturedSpan would rehydrate it as null regardless.
Suggested cleanup: drop the inner if (typeof r.meta?.parent_message_id === "string") branch entirely; if a row has parentMessageId == null here, just queue the thread for the state-fallback lookup. Otherwise the comment "looks like real fallback coverage" without delivering it.
| if (r.parentMessageId == null) { | |
| for (const r of rows) { | |
| if (r.parentMessageId == null) { | |
| missingThreads.add(r.threadId); | |
| } | |
| } |
| // spans stealing the pmid of the next user message). Returns null on | ||
| // fetch error so bulkInsertSpans can proceed with null pmids rather | ||
| // than crash the run. | ||
| export async function findLatestParentMessageId(threadId: string): Promise<string | null> { |
There was a problem hiding this comment.
findLatestParentMessageId used to be a single indexed Postgres SELECT. It is now a synchronous HTTP call to langGraphClient.threads.getState(threadId), which goes through the proxy at LANGGRAPH_API_URL (lib/langgraph/client.ts — default http://localhost:2024).
bulkInsertSpans is called from handleChainEnd on the request hot path. The new code does one network call per missing thread per batch, sequentially:
for (const tid of missingThreads) {
const latest = await findLatestParentMessageId(tid);
...
}In a worst-case batch (one span from N different threads that all bypassed the metadata path — e.g. a chat-side regenerate + multiple interrupts while the langgraph dev server is rebuilding), this serially blocks the End hook for N round-trips. Two concrete risks:
- Trace latency — every chat turn that hits this path adds
N × dev-server-RTTto the visible user response time (it's awaited before INSERT). bulkInsertSpanstimeout — if the dev server is busy enough that the call stalls beyond the SDK's default timeout, every span in the batch getsparentMessageId=nulland the per-turn panel 404s (which the design intentionally accepts, but as a side-effect of this slowdown, not the cold-path).
Suggested hardening before this lands:
- Run the per-thread lookups in
Promise.all(one batch, parallel goroutines, deduped by thread) — at least removes the serial wall-clock. - Add a circuit-breaker / short timeout so a stuck dev server doesn't block the entire INSERT (e.g.
Promise.race([getState(...), timeout(500).then(()=>null)])). - Or skip the fallback entirely when the batch came from a chat streaming path that doesn't need it (chat uses
metadata.parent_message_idalready per the newhandleChainStart).
The current change is correct for isolation (no shared state, no cross-thread bleed), but it's strictly more expensive than what it replaced.
|
|
||
| const chunks: KbChunkPreview[] = | ||
| doc.status === "success" ? await findKbChunksContentByDocumentId(user.id, doc.id) : []; | ||
| const [chunks, entityCount, relationshipCount] = await Promise.all([ |
There was a problem hiding this comment.
✅ Good fix on the cartesian-inflation issue. The two independent subqueries + coalesce(...,0) correctly give entityCount and relationshipCount as their true values, including when one side has zero rows. Confirmed via tests/api/kb/documents/[id].test.ts cases 1–3.
Two adjacent points worth flagging while this file is open, even if out of scope for the PR:
-
status === "success"guard onfindKbChunksContentByDocumentIdis the only status filter — if a doc is inparsing/failed,chunksreturns[], butentityCount/relationshipCountstill query. Forparsing/faileddocs that's fine (the badge shows "Pending"/"Not Indexed" anyway), but the badge condition incomponents/tool-ui/kb/status-badge.tsxusesentityCount === 0 && relationshipCount === 0 → hasGraph=falseto flip the badge to "Embedding". For a fresh doc with 0 chunks yet,entityCount/relationshipCountwill both be 0, but the badge will render based on the doc-status branch (pending/parsing) above this and show "Pending" — which is correct by accident, not by design. A status-conditional gating on the count query would make the intent explicit. -
The
Promise.all([...]).then((rows) => rows[0]?.n ?? 0)micro-promise pattern works butcount(*)always returns a row in PG, sorows[0]?.n ?? 0is paranoid — the?? 0is unreachable. Dropping it (or usingNumber(rows[0]!.n)) removes dead fallback code.
| state: KbAgentStateShape, | ||
| ): Promise<Partial<KbAgentStateShape>> { | ||
| if (state.mode === "chunksOnly") { | ||
| if (state.mode === "chunksOnly" || state.mode === "retryFailedChunks") { |
There was a problem hiding this comment.
👀 Quick clarification on the comment-vs-code matchup on line 24:
if (state.mode === "chunksOnly" || state.mode === "retryFailedChunks") {
// ... `retryFailed` is intentionally NOT in this list — its whole point is
// to re-OCR the failed pages, so this node has to run.state.retryFailed falls through to the OCR branch, so the comment is accurate. ✅
But the OCR branch's per-page skip —
if ((p.markdown ?? "").trim().length > 0 && !p.errorMessage) {
return p;
}— only skips successful pages. Pages in status: "failed" (with empty markdown) get sent through OCR again, which is what we want for retryFailed. However, pages with markdown.length > 0 but status: "failed" will be silently skipped (the !p.errorMessage guard passes either way). Worth confirming: in retryFailed we want to re-OCR any page whose prior markdown is empty or whose prior attempt errored — the current logic does this, but pages where the OCR returned non-empty markdown AND set an errorMessage are treated as fine. Tiny edge case, but probably worth a sentence in the mode comment.
Lower priority: split-file-page-node.ts is also skipped for retryFailed, so the pre-condition is that pagesByDocId is already populated by prepareKBDataNode → pulled from doc.pages. The pages column may include mixed-status pages; the new test in tests/backend/node/kb/page-to-markdown-node.test.ts exercises just one failed page — a test that mixes successful + failed pages in pagesByDocId would prove the per-page skip preserves the cached ones while re-OCRing only the failed ones.
| @@ -1442,12 +1516,38 @@ export async function deleteKbDocumentForUser( | |||
| export async function deleteKbFolderForUser( | |||
There was a problem hiding this comment.
✅ The cascade is sound — kb_chunk / kb_entity / kb_relationship / kb_theme all ON DELETE CASCADE off kb_document, so the DELETE kb_document … WHERE folder_id = ? step transitively drops every row of child data. Attachments are dropped explicitly here rather than relying on the ON DELETE SET NULL FK, which is the right call (orphan pointers would be the only thing they'd leave behind).
Two subtle points worth knowing even if the current tests cover the happy path:
-
R2 objects are deliberately retained — both this PR and
deleteKbDocumentForUserkeep theu/<uid>/upload/<sha>.<ext>objects and the per-page PNGs in R2 (sha-keyed dedup, seedocs/ATTACHMENTS.md). The new testdeletes attachment rows for every doc when deleteAll=true (R2 objects stay)covers the row side. There's no test asserting "re-uploading the same bytes re-uses the existing R2 objects after a cascade delete" — that's the refcount guarantee the design rests on. Probably belongs intests/api/kb/folder.test.tsnext to the new ones. -
threadstable cleanup precedeskb_documentdelete. If a child cascade failed midway (e.g. an unenforced FK surfaces), the transaction rolls back, the threads stay attached to their docId strings, and the next attempt re-runs. Correct. But the order is worth a one-liner comment explaining why thread cleanup happens before doc deletion (so a half-finished cascade doesn't leave orphaned threads pointing at docs that the next pass will eventually delete).
| </div> | ||
| )} | ||
|
|
||
| <div className="flex items-center justify-between rounded-lg border p-3 shadow-xs"> |
There was a problem hiding this comment.
-
Confirmation string
delete allis case-insensitive viaconfirmText.trim().toLowerCase() !== "delete all"— meaningDELETE ALL,Delete All, andDELETE ALL\tall satisfy. That's fine UX-wise, but the "Copy" button pastes literally"delete all"(lowercase) into the input vianavigator.clipboard.writeText("delete all")— so a user who clicks "Copy", then retypes, will hit the same lowercased string. Someone who copies and pastes into a chat to share will only see the lowercase form. Consider pasting the canonical phrase ("delete all") so copy/paste round-trips correctly. -
Cosmetic but legal-ish copy: the destructive-card text says "Also destroys all documents, chunks, and vector embeddings in this folder" — but R2 source objects (
u/<uid>/upload/<sha>.<ext>) are intentionally retained and not part of the cascade (see the in-line note inlib/kb/queries.ts:1499-1503). The Settings list / DLQ dialog inDocDeleteDialog(line 89-90) explicitly calls this out: "Source PDF + rendered page PNGs stay in R2". The folder-delete copy doesn't carry the same caveat, so a user cascade-deleting a folder will reasonably assume the PDFs are gone. Either:- add a one-line note "PDFs and rendered page images remain in R2 until the retention sweep" to the folder dialog, or
- move the existing "stay in R2" disclaimer into a shared helper so both
DocDeleteDialogandFolderDeleteDialogreference it consistently.
The destructive flow itself (typed confirmation + switch, fired only after explicit gate) is solid. Just want the language to match the actual blast radius.
Code review — PR #47Walked through the diff and the surrounding files. The Intent here is sound — tighten the Indexed badge, fix parent_message_id concurrency, give users a destructive folder clear — and the test coverage ( Blocking
Worth fixing before merge
Confirmed-correct (no action)
Tests / docs compliance
Quick stats
|
- lib/observability/queries.ts: remove the leftover console.warn(2222, ...) debug artifact in bulkInsertSpans; rewrite the stale "look up the column" comment to describe the actual langGraphClient.threads.getState + lastHumanMessageId fallback path (the column is no longer the canonical source). - lib/kb/source-kind.ts: mimeShortLabel fallback restored — upper- cased subtype (e.g. "application/json" → "JSON") instead of the raw mime, so unmapped types still render readably in the table. - components/tool-ui/kb/status-badge.tsx: drop the hasGraph gate from isCompleted. Vectors being complete is the "Indexed" signal; the graph leg is supplementary and may legitimately produce zero rows (plain-text docs). Tooltip still surfaces entity/relationship counts for transparency. - docs/APIS.md: cascade-delete narrative aligns with the actual semantics — attachment rows ARE deleted (kb cascade); R2 objects stay (sha-keyed, ref-counted). - docs/OBSERVABILITY.md: correct the pmid propagation path — kb ingest passes parent_message_id via the top-level runs.create metadata arg, NOT config.configurable. The handler reads it from metadata.parent_message_id on the LC callback surface. - tests/api/kb/folder.test.ts: extend cascade coverage — verify kb_chunk + kb_entity + kb_relationship FK CASCADE + the explicit threads-row delete in deleteKbFolderForUser's deleteAllDocs branch.
CI fix (round 3 primary): - lib/observability/queries.ts: bound the state-fallback lookup in backfillParentMessageIds to a 500ms timeout AND parallelize the per-thread fetches via Promise.all. CI was hanging 5s on 9 bulkInsertSpans tests because langGraphClient.threads.getState never resolves in a test env (no dev server). Worst-case batch latency is now ~500ms, not N × RTT. Rows that time out stay pmid-less and the per-turn panel 404s them by design. - lib/observability/queries.ts: drop the dead fallback branch — toRow already lifts meta.parent_message_id into r.parentMessageId; the "typeof meta?.parent_message_id === 'string'" guard rescued only the empty-string case which gets re-nulled by toCapturedSpan anyway. Round 3 review fixes (Claude + cubic): - components/tool-ui/kb/status-badge.tsx: isFailed now requires embeddingPending === 0 — otherwise a doc with failed + embedding- pending chunks collapses into the "X failed" badge even though only X of the in-flight chunks actually errored. The mixed state stays in the in-progress lane. - components/assistant-ui/kb-mention.tsx: revert max-h-68 → max-h-72 (68 isn't in Tailwind's default scale, so the class generated no CSS and the popover overflowed on short viewports). - app/api/kb/documents/[id]/route.ts: drop the ?? 0 paranoia — count(*) always returns a row in PG, the fallback was unreachable. - lib/kb/queries.ts: add a comment explaining why thread cleanup precedes kb_document delete (so a mid-cascade FK failure rolls back without orphaning threads). - components/settings/kb-view/dialogs.tsx: folder-delete dialog now states "Source PDFs + rendered page images stay in R2 until the retention sweep" so the destructive copy matches the actual blast radius (matches the DocDeleteDialog caveat). Test: - tests/backend/node/kb/page-to-markdown-node.test.ts: new case — mixed pagesByDocId under retryFailed preserves the cached successful page verbatim and re-OCRs only the failed one.
- bulkInsertSpans → backfillParentMessageIds → findLatestParentMessageId hits langGraphClient.threads.getState. CI has no langgraph dev server, so the call hangs and 9 bulkInsertSpans tests time out at 5s. Stub the HTTP layer in the two affected test files so the state fetch returns immediately (no human messages → null pmid → row persists pmid-less, the panel 404s those by design). - The 500ms timeout in backfillParentMessageIds is kept as defense-in-depth for the real prod server hanging — but the test env no longer depends on it.
There was a problem hiding this comment.
1 issue found across 10 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="components/tool-ui/kb/status-badge.tsx">
<violation number="1" location="components/tool-ui/kb/status-badge.tsx:209">
P2: With this change, a doc that has some permanently failed chunks alongside chunks still awaiting vector embedding now shows the 'Embedding (X/Y)' badge instead of 'Z failed', but the tooltip for that branch never surfaces the failed count (unlike the hasInflightChunks branch, which does include `${failed} chunks failed`). Users lose visibility into failed chunks whenever embeddingPending > 0, which is the opposite problem the comment says this fix is solving.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| // "X failed" badge for a doc that's really "X failed, Y still | ||
| // embedding" and the user can't reconcile the count with the | ||
| // tooltip breakdown. | ||
| const isFailed = failed > 0 && embeddingPending === 0 && terminal === total; |
There was a problem hiding this comment.
P2: With this change, a doc that has some permanently failed chunks alongside chunks still awaiting vector embedding now shows the 'Embedding (X/Y)' badge instead of 'Z failed', but the tooltip for that branch never surfaces the failed count (unlike the hasInflightChunks branch, which does include ${failed} chunks failed). Users lose visibility into failed chunks whenever embeddingPending > 0, which is the opposite problem the comment says this fix is solving.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At components/tool-ui/kb/status-badge.tsx, line 209:
<comment>With this change, a doc that has some permanently failed chunks alongside chunks still awaiting vector embedding now shows the 'Embedding (X/Y)' badge instead of 'Z failed', but the tooltip for that branch never surfaces the failed count (unlike the hasInflightChunks branch, which does include `${failed} chunks failed`). Users lose visibility into failed chunks whenever embeddingPending > 0, which is the opposite problem the comment says this fix is solving.</comment>
<file context>
@@ -199,7 +199,14 @@ export function ChunksStatusBadge({
+ // "X failed" badge for a doc that's really "X failed, Y still
+ // embedding" and the user can't reconcile the count with the
+ // tooltip breakdown.
+ const isFailed = failed > 0 && embeddingPending === 0 && terminal === total;
const hasInflightChunks = pending > 0 || parsing > 0;
const hasInflightExtraction = entities === 0 && relationships === 0;
</file context>
Problem
kb_chunk.status='success'alone, but pgvector hadn't received the embedding yet — the doc was
vector-less when the badge said "Indexed".
kb_observabilityspans for concurrent KB uploads were getting taggedwith the wrong parent_message_id (the next user message, not the
trigger). Root cause was a shared instance field in
CapturingHandler.DELETE /api/kb/folders/[id]refused with 409 NON_EMPTY on any non-emptyfolder; no path for the user to clean up a whole folder.
What changed
fix(observability)—9e39a9cCapturingHandler.handleChainStartresolvesparent_message_idper-callfrom
metadata.parent_message_id(LangGraph-configurable passthrough,set per-run via
runs.create({config:{configurable:{parent_message_id}}}))with
lastHumanMessageId(inputs.messages)as fallback. Dropped theshared instance field.
findLatestParentMessageIdnow callslangGraphClient.threads.getStateand parses
state.values.messagesvialastHumanMessageId— theobservability_spanscolumn is not read for backfill.feat(kb)—feb73bbsuccessChunksredefined: counts chunks withstatus='success' AND embedding IS NOT NULL. NewembeddingPendingChunkscountsstatus='success' AND embedding IS NULL.entityCount > 0 OR relationshipCount > 0(hybrid-search third legneeds graph rows to score against). Two intermediate states:
Extracting (X/Y)(OCR in flight) andEmbedding (X/Y)(vector +graph extract in flight).
entityCount+relationshipCountthrough 4 type files(
components/tool-ui/kb/types.ts,components/settings/kb-view/types.ts,backend/tool/kb/types.ts,backend/tool/kb/list-documents.ts) and theChunksStatusBadgeprop list.feat(kb)—a73a179splitFileToPageNode+pageToMarkdownNodeskip onretryFailed/retryFailedChunks(waschunksOnlyonly);prepareKBDataNodereadsconfig.configurable.parent_message_idfirst.DELETE /api/kb/folders/[id]?deleteAll=truecascades through everydoc in the folder (chunks, embeddings, attachments). Settings dialog
adds a switch + typed "delete all" confirmation.
DocDeleteDialog/DocReprocessDialogclose themselves viaonOpenChange(false)before firingonDeleted/onReprocessed;long titles get
break-all.FolderSidebarcallbacks no longer resetselection on submit (the dialog does it via
onOpenChange).ObservabilityPopoverno longer auto-closes on row click — the Sheetis the singleton entry.
kb-mentionpopover: scrollable body + sticky header padding tweak.fix(kb)—9616f02shrink-0on icons +min-w-0 max-w-fullonshell +
truncate min-w-0on label so the fixed-width badge columnin
DocRowdoesn't push siblings off-grid on long status text.Docs
docs/APIS.md: new indexing-count fields onGET /api/kb/documentsGET /api/kb/documents/[id],chunks[].hasEmbedding, folder?deleteAll=truecascade.docs/OBSERVABILITY.md: per-call pmid resolution from metadata +langGraphClient.threads.getStatefallback.docs/TOOLS.md: expandedlist_documentspayload shape shared withthe Settings badges.
Tests
pnpm typecheck: cleanpnpm lint: cleanpnpm test: 131 files / 1367 passedtests/api/kb/folder.test.ts(cascade delete) +tests/api/kb/reprocess.test.ts(retryFailedChunkspipeline skipsOCR + preserves
doc.status='success').🤖 Generated with Claude Code
Summary by cubic
Tightened the Knowledge Base “Indexed” badge to require embeddings, added entity/relationship counts across APIs, enabled safe folder cascade delete (with attachment cleanup), and fixed
parent_message_idattribution with a 500ms-bounded backfill. Also corrected OCR retry modes and applied small UI polish.New Features
entityCount/relationshipCountare surfaced but not required for “Indexed”. AddedembeddingPendingChunks,entityCount, andrelationshipCounttoGET /api/kb/documents,GET /api/kb/documents/[id], andlist_documents; doc detail exposeschunks[].hasEmbedding.DELETE /api/kb/folders/[id]?deleteAll=truecascades through docs, chunks, embeddings, attachments, and relatedthreads; dialog adds a toggle and typed confirm. Reprocess modes:retryFailedChunksandchunksOnlyskip OCR;retryFailedre-OCRs failed pages.prepareKBDataNodeprefersconfig.configurable.parent_message_id.Bug Fixes
parent_message_idper call from metadata with alangGraphClient.threads.getStatefallback capped at 500ms and fetched in parallel. Removed shared handler state to prevent cross-run clobbering.entityCount/relationshipCountinflation; tighter badge truncation and mixed-state logic (don’t mark as failed while embeddings are pending);ObservabilityPopoverstays open on row click; kb-mention popover height fixed; dialogs close before callbacks.Written for commit d70418b. Summary will update on new commits.
Greptile Summary
The PR tightens KB indexing status, adds folder cascade deletion, corrects observability attribution, and repairs failed-page OCR retries.
Confidence Score: 5/5
The PR appears safe to merge.
No blocking failures remain in the fixes associated with the previous review threads.
Important Files Changed
Reviews (5): Last reviewed commit: "test(observability): mock langGraphClien..." | Re-trigger Greptile