Conversation
📝 WalkthroughWalkthroughCustom edge labels now round-trip through ChangesRepo-local custom edge kinds
Sequence Diagram(s)sequenceDiagram
participant Test as custom edge round trip test
participant Persist as persist_graph_to_lance
participant DB as LanceDB
participant Load as load_graph_from_lance
participant Parse as parse_edge_kind
participant FromLabel as EdgeKind from_label
participant Format as format_neighbors_grouped
Test->>Persist: persist supports edge
Persist->>DB: store edge_type supports
DB-->>Load: reload persisted edge_type
Load->>Parse: parse edge_type
Parse->>FromLabel: delegate label parsing
FromLabel-->>Load: return EdgeKind Other supports
Test->>Format: render grouped traversal
Format-->>Test: return custom edge group
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/server/store/persist.rs (1)
916-973: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMake the workaround assertion exercise the same traversal path.
wrong_workaroundstarts atmetadata_only, but the grouped traversal starts fromclaimant, so!groups.contains_key(&EdgeKind::References)passes even if a same-source workaround edge would leak into the supports traversal. Put the workaround onclaimantand filter traversal to the custom supports kind so the regression test is adversarial.Proposed test tightening
+ let supports_kind = EdgeKind::Other("supports".to_string()); let mut metadata_only = make_test_node("metadata_only"); metadata_only .metadata .insert("relationship".to_string(), "supports".to_string()); let wrong_workaround = Edge { - from: metadata_only.id.clone(), + from: claimant.id.clone(), to: evidence.id.clone(), kind: EdgeKind::References, source: ExtractionSource::Markdown, confidence: Confidence::Detected, }; let custom_edge = Edge { from: claimant.id.clone(), to: evidence.id.clone(), - kind: EdgeKind::Other("supports".to_string()), + kind: supports_kind.clone(), source: ExtractionSource::Markdown, confidence: Confidence::Detected, }; @@ .edges .iter() - .filter(|e| e.kind == EdgeKind::Other("supports".to_string())) + .filter(|e| e.kind == supports_kind) .collect(); @@ - let groups = state.index.neighbors_grouped( + let supports_filter = [supports_kind.clone()]; + let groups = state.index.neighbors_grouped( &claimant.stable_id(), - None, + Some(&supports_filter), petgraph::Direction::Outgoing, ); let supports_group = groups - .get(&EdgeKind::Other("supports".to_string())) + .get(&supports_kind) .expect("custom supports edge missing from grouped traversal");🤖 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 `@src/server/store/persist.rs` around lines 916 - 973, The regression test in persist_graph_to_lance is not exercising the same traversal path as the custom supports edge because wrong_workaround is anchored at metadata_only instead of claimant. Update the test data so the workaround edge originates from claimant, and make the neighbor-group assertion inspect only the custom EdgeKind::Other("supports") path used by neighbors_grouped. Keep the load_graph_from_lance and grouped traversal checks aligned around claimant, evidence, EdgeKind::References, and EdgeKind::Other("supports") so the test fails if a same-source workaround leaks into the supports traversal.
🤖 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.
Nitpick comments:
In `@src/server/store/persist.rs`:
- Around line 916-973: The regression test in persist_graph_to_lance is not
exercising the same traversal path as the custom supports edge because
wrong_workaround is anchored at metadata_only instead of claimant. Update the
test data so the workaround edge originates from claimant, and make the
neighbor-group assertion inspect only the custom EdgeKind::Other("supports")
path used by neighbors_grouped. Keep the load_graph_from_lance and grouped
traversal checks aligned around claimant, evidence, EdgeKind::References, and
EdgeKind::Other("supports") so the test fails if a same-source workaround leaks
into the supports traversal.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5d0478b2-117b-4c24-ab60-6bfa8d33bf42
📒 Files selected for processing (5)
src/graph/index.rssrc/graph/mod.rssrc/server/helpers.rssrc/server/store/mod.rssrc/server/store/persist.rs
Closes #707
Summary
EdgeKind::Other(String)with string-label serde/display parsing for repo-local relationship kinds.Verification
cargo test custom_edge_kind --libcargo test format_neighbors_grouped_preserves_custom_edge_label --libcargo test builtin_edge_kind_json_serialization_stays_string_label --libcargo check --libReview
sg reviewunavailable:error: command not found: sg; performed manual staged-diff review instead.Summary by CodeRabbit