ENG-2128 Express source as a sourceDocument slot/reference of Evidence - #1329
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
|
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}
1a54ca6 to
3095571
Compare
3095571 to
30f5a10
Compare
PR size/scope checkThis PR is over our review-size guideline.
Please split this into smaller PRs unless there is a clear reason the changes need to land together. If keeping it as one PR, please add a brief justification covering:
|
1cf61d6 to
f54d468
Compare
f54d468 to
1d4a208
Compare
1d4a208 to
bd6014f
Compare
03486e7 to
1f3c80c
Compare
bd6014f to
bb080ff
Compare
bb080ff to
8317cba
Compare
e094354 to
f8a4742
Compare
8317cba to
a0f586b
Compare
38a56dc to
a942edd
Compare
a0f586b to
0266b22
Compare
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3d27d958f0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
mdroidian
left a comment
mdroidian
left a comment
There was a problem hiding this comment.
See the codex comment, let's make sure we default Page and Block matchers.
And there's a couple other comments we should address before merging.
mdroidian
left a comment
mdroidian
left a comment
There was a problem hiding this comment.
Approving this but the requested change is blocking and requires fixing. If you choose to go with code other than title: node.node_title ?? node.text,, please re-request a review.
| const localConcept = discourseNodeBlockToLocalConcept(context, { | ||
| nodeUid: node.source_local_id, | ||
| schemaUid: node.type, | ||
| text: node.node_title ? `${node.node_title} ${node.text}` : node.text, |
There was a problem hiding this comment.
It looks like this breaks concept syncing for "page-backed nodes" (read regular nodes). The page query stores the page title in node.text and does not set node.node_title, so this passes undefined. That becomes an undefined concept name, but Concept.name is required, causing the upsert to fail with -2.
Please restore the fallback:
title: node.node_title ?? node.text,For page-backed nodes, node.text is the page title. For "block-backed nodes" (read nodes that include an embedding block ref), node.node_title is the containing page’s title.
We should probably add a test that covers both cases.
There was a problem hiding this comment.
Ah, good catch. I looked at getAllDiscourseNodesSince but not getDiscourseNodeTypeWithSettingsBlockNodes.
I think this is worth a refactor, where node-title is always the title, and optional embed-text is the block if different from the title.
But that's a different scope.
There was a problem hiding this comment.
(And I prefer that to a test that would tend to enshrine the current inconsistency)
2 participants
https://linear.app/discourse-graphs/issue/ENG-2128/express-source-as-a-sourcedocument-slotreference-of-evidence
code walkthrough: https://www.loom.com/share/aacdf3b6a2cb4abe9e7063026f67d521
demo: https://www.loom.com/share/4e6249bca9754bfea7159c6a078e9c02