ENG-2129 Populate reference_content from CrossAppNode and roles from CrossAppNodeSchema - #1324
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. |
a90808f to
cca9575
Compare
34d0502 to
dbd71ca
Compare
cca9575 to
2269c8c
Compare
2269c8c to
0684aad
Compare
388295f to
68b7d27
Compare
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
68b7d27 to
b98a419
Compare
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
b98a419 to
a998dc7
Compare
a998dc7 to
ab1b460
Compare
ab1b460 to
03486e7
Compare
|
Got Claude to add tests, which led to a correction. |
03486e7 to
1f3c80c
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:
|
|
@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: 1f3c80ca3c
ℹ️ 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".
| r, | ||
| asSimpleLocalId(schemaMap[referenceContent[r] ?? 0], spaceUrl), | ||
| ]) | ||
| .filter(([, s]) => s !== undefined) as [string, string][], |
There was a problem hiding this comment.
When a schema declares a role without a reference_content target, this filter removes the role entirely. That is a valid representation because CrossAppNodeSchema.slotDefinitions explicitly permits undefined values, and crossAppNodeSchemaToDbConcept reconstructs literal_content.roles from Object.keys(slotDefinitions); consequently, a database-to-CrossApp-to-database round trip silently erases unconstrained roles and changes the schema's arity. Retain each declared role with an undefined value when no target can be resolved.
Useful? React with 👍 / 👎.
| const space = spacesById.get(c.space_id || 0); | ||
| if (!space || !c.source_local_id || !c.id) return [c.id, undefined]; |
There was a problem hiding this comment.
When a shared node's slot targets a concept outside the node's own space, this lookup can resolve the RID only if that referenced space is present in spacesById. Production callers do not guarantee that: getSharedNodeByRid passes only the source space to buildSharedNodes, while listGroupSharedNodes explicitly excludes the current space. Such cross-space slots are therefore silently dropped even though the added unit test succeeds by manually supplying otherSpace; fetch the spaces used by concepts_of_relation before building the node map.
Useful? React with 👍 / 👎.
mdroidian
left a comment
mdroidian
left a comment
There was a problem hiding this comment.
Probably worth looking into the comments.
| template_content: node.template, | ||
| roles: slots.length > 0 ? slots : undefined, | ||
| }); | ||
| const referenceContent = slots.length ? node.slotDefinitions! : undefined; |
There was a problem hiding this comment.
Avoid ! whenever possible. If not possible, document why it is absolutely required.
Looks like it isn't required here so let's not leave ourselves a footgun.
2 participants
https://linear.app/discourse-graphs/issue/ENG-2129/populate-reference-content-from-crossappnode-and-roles-from
https://www.loom.com/share/0b2750242d5c44f3aeead28302d24c17