fix(memory): graph query returns namespace data and add sync e2e tests (#344)#363
Conversation
tinyhumansai#344) The knowledge graph UI showed empty because graph_query(None) only queried the graph_global table, while ingestion writes to graph_namespace. Now graph_query(None) queries both tables via graph_query_all(), merging results. Changes: - Added graph_query_all() in unified graph store to query across all namespaces - MemoryClient::graph_query(None) now uses graph_query_all() instead of graph_query_global() - MemoryWorkspace passes selectedNamespace to the RPC call - Added diagnostic logging in ingestion pipeline (RelEx model availability, extraction counts) - Added debug logging in tauriCommands for unexpected response shapes - Added 2 integration tests proving document sync populates the graph (ignored by default for CI, run with --ignored) Closes tinyhumansai#344 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds cross-namespace graph querying and logging, updates the frontend to pass the selected namespace to graph queries, and introduces integration tests that verify document ingestion populates namespace and cross-namespace graph results. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/memory_graph_sync_e2e.rs (1)
177-262: Fixed-duration sleep may cause flaky test results.The 5-second sleep at line 213 to wait for background ingestion is timing-dependent. On slower CI runners or under load, the background worker may not complete in time.
Consider adding a polling mechanism or exposing a way to flush/await the ingestion queue for deterministic test behavior. However, since this is an
#[ignore]test meant for manual verification, the current approach is acceptable for initial coverage.The conservative assertion (lines 239-244) that only checks
all_rowswhenns_rowsis non-empty is a pragmatic workaround for environments where the ONNX model may fail silently.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/memory_graph_sync_e2e.rs` around lines 177 - 262, The test put_doc_background_extraction_then_graph_query uses a fixed 5s tokio::time::sleep which makes it flaky; replace the static sleep with a polling loop that repeatedly calls graph_query(Some(namespace), ...) or list_documents(Some(namespace)) until the expected document/relations appear or a sensible timeout elapses (e.g., total wait ~30s with short sleeps between attempts), and fail if the timeout is reached; update the test to reference the existing functions put_doc, graph_query, and list_documents and keep the conservative assertion logic but trigger it only after the poll confirms background ingestion completion or times out.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/utils/tauriCommands.ts`:
- Around line 523-527: Prettier is flagging formatting around the debug log in
app/src/utils/tauriCommands.ts; run the formatter (e.g. prettier --write
app/src/utils/tauriCommands.ts) or remove the trailing comma after the second
argument in the console.debug call that contains '[memoryGraphQuery]' so the
file conforms to project Prettier rules; ensure overall file is reformatted and
committed.
---
Nitpick comments:
In `@tests/memory_graph_sync_e2e.rs`:
- Around line 177-262: The test put_doc_background_extraction_then_graph_query
uses a fixed 5s tokio::time::sleep which makes it flaky; replace the static
sleep with a polling loop that repeatedly calls graph_query(Some(namespace),
...) or list_documents(Some(namespace)) until the expected document/relations
appear or a sensible timeout elapses (e.g., total wait ~30s with short sleeps
between attempts), and fail if the timeout is reached; update the test to
reference the existing functions put_doc, graph_query, and list_documents and
keep the conservative assertion logic but trigger it only after the poll
confirms background ingestion completion or times out.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: e0b7b3c6-bebd-4cfc-8730-8065781c127c
📒 Files selected for processing (6)
app/src/components/intelligence/MemoryWorkspace.tsxapp/src/utils/tauriCommands.tssrc/openhuman/memory/ingestion.rssrc/openhuman/memory/store/client.rssrc/openhuman/memory/store/unified/graph.rstests/memory_graph_sync_e2e.rs
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
tinyhumansai#344) (tinyhumansai#363) * fix(memory): graph query returns namespace data and add sync e2e tests (tinyhumansai#344) The knowledge graph UI showed empty because graph_query(None) only queried the graph_global table, while ingestion writes to graph_namespace. Now graph_query(None) queries both tables via graph_query_all(), merging results. Changes: - Added graph_query_all() in unified graph store to query across all namespaces - MemoryClient::graph_query(None) now uses graph_query_all() instead of graph_query_global() - MemoryWorkspace passes selectedNamespace to the RPC call - Added diagnostic logging in ingestion pipeline (RelEx model availability, extraction counts) - Added debug logging in tauriCommands for unexpected response shapes - Added 2 integration tests proving document sync populates the graph (ignored by default for CI, run with --ignored) Closes tinyhumansai#344 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * style: remove trailing comma for Prettier compliance Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary
graph_query(None)only queried thegraph_globaltable, which is always empty because ingestion writes tograph_namespace. Addedgraph_query_all()that queries both tables and merges results — the knowledge graph now shows entities in the UI.MemoryWorkspacenow passesselectedNamespaceto the graph query RPC instead of calling with no parameters.tauriCommandsfor unexpected RPC response shapes.#[ignore]tests proving the sync-to-graph contract (run with--ignored).Test plan
cargo checkpassescargo test --test memory_graph_sync_e2e -- --ignored— 2 passedcargo fmtcleanCloses #344
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Tests