Repository navigation
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Devin Review found 1 potential issue.
1 flag not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)
| (sameValue(a.localId, b.localId) && | ||
| !ROAM_DEFAULT_NODE_TYPE_IDS.has(a.localId!)) || |
There was a problem hiding this comment.
🟡 Conflicting schema IDs select wrong triples
When distinct schemas reuse a non-default local ID, matchNodeTypes ranks them as a strong match despite conflicting RIDs. pickTripleCandidate can then prefer the wrong triple over a name-matched candidate.
Learn more
A RID identifies a schema together with its space, while a local ID only identifies it within one space. The matcher treats equal non-default local IDs as strong even when both available RIDs identify different schemas. The scorer then favors that candidate over a legitimate name match. The existing import matcher in findLocalNodeTypeMatch also reuses bare IDs, but this new matcher has both qualified identities available and uses its score to decide which source triple to select.
Example: The endpoint has {rid: 'orn:spaceA.schema:x', localId: 'x', name: 'Evidence'}. Candidate one has {rid: 'orn:spaceB.schema:x', localId: 'x', name: 'Question'}; candidate two has {rid: 'orn:spaceB.schema:y', localId: 'y', name: 'Evidence'}. Candidate one wins on ID despite its explicitly different RID and name.
Recommended fix: Restrict bare local-ID matches to cases without conflicting qualified RID or origin evidence; use matching RIDs and origin links to establish a strong cross-space match. Add a test with conflicting RIDs and equal local IDs.
Was this helpful? React with 👍 or 👎 to provide feedback.
5d1b689 to
fc32c8c
Compare
|
|
||
| const normalizeName = (name: string): string => name.trim().toLowerCase(); | ||
|
|
||
| // The same in every Roam graph; keep in sync with `type` in Roam's `defaultDiscourseNodes`. |
There was a problem hiding this comment.
We're now defining these IDs in two places, so changes to Roam's defaults could leave this list out of sync. Could we use a single source of truth? If you decide to go this route, please request review again.
If we keep them separate, can we at least add a note in defaultDiscourseNodes.ts pointing here so it's clear both need updating? If we do that, let's also add the full path.
There was a problem hiding this comment.
Kept them separate and added a note with the full path on both sides. A test in ENG-2340 (apps/roam/src/utils/tests/roamDefaultNodeTypeIds.test.ts) checks that the two lists match.
2 participants
https://entire.io/gh/DiscourseGraphs/discourse-graph/trails/76
Reviewer brief
tripleMatching.ts. Two node types match strongly on the same RID, the same local id, or an import origin that links them (one imported from the other, or both from the same source); otherwise they match weakly on the normalized name. A candidate triple must match on both ends, the one with more strong matches wins, and a tie picks nothing. ThenROAM_DEFAULT_NODE_TYPE_IDS: Roam's default type ids are the same in every graph, so they match only by name. The list replaces Roam'sINITIAL_NODE_VALUESimport, and a guard test in ENG-2340 Import cross-space relations into Roam #1524 keeps the two equal.Loom video
https://www.loom.com/share/93c233a0522c4542a159230a80f3e3a1
Scope check
$scope-checkagainst ENG-2373 and the final diff.Done When: None.Standards check
$dg-pr-adherence-checkagainst the final diff and PR metadata.Local delegated full review
$dg-delegated-full-reviewwhen no other full-review workflow is available.https://linear.app/discourse-graphs/issue/ENG-2373/share-the-relation-triple-matching-rule-in-packagesdatabase