Skip to content

ENG-1865 Publish Roam stored relations for shared nodes#1221

Open
maparent wants to merge 3 commits into
eng-2018-add-converter-for-crossapprelation-schema-tofrom
eng-1865-publish-roam-stored-relations-for-shared-nodes
Open

ENG-1865 Publish Roam stored relations for shared nodes#1221
maparent wants to merge 3 commits into
eng-2018-add-converter-for-crossapprelation-schema-tofrom
eng-1865-publish-roam-stored-relations-for-shared-nodes

Conversation

@maparent

@maparent maparent commented Jul 11, 2026

Copy link
Copy Markdown
Collaborator

https://linear.app/discourse-graphs/issue/ENG-1865/publish-roam-stored-relations-for-shared-nodes

Waiting for eng-1856 to work with relations to imported nodes; but functional otherwise.

One thing I expect to discuss: I put everything in one file, and it would be worth distributing code in various utility functions, and maybe breaking up the PR. We should discuss how to break it up, but I wanted to start from end-to-end functionality.

https://www.loom.com/share/74b4c5b9e750404e8ea1f59ace042492

@linear-code

linear-code Bot commented Jul 11, 2026

Copy link
Copy Markdown

ENG-1865

@supabase

supabase Bot commented Jul 11, 2026

Copy link
Copy Markdown

This pull request has been ignored for the connected project zytfjzqyijgagqxrzbmz because there are no changes detected in packages/database/supabase directory. You can change this behaviour in Project Integrations Settings ↗︎.


Preview Branches by Supabase.
Learn more about Supabase Branching ↗︎.

@vercel

vercel Bot commented Jul 11, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
discourse-graph Ready Ready Preview, Comment Jul 15, 2026 8:34pm

Request Review

@maparent maparent force-pushed the eng-1865-publish-roam-stored-relations-for-shared-nodes branch from df8bd66 to 7e9a33d Compare July 11, 2026 13:26
@maparent maparent force-pushed the eng-1865-publish-roam-stored-relations-for-shared-nodes branch from 7e9a33d to e15e0eb Compare July 13, 2026 04:01
@maparent maparent marked this pull request as ready for review July 13, 2026 14:44
devin-ai-integration[bot]

This comment was marked as resolved.

graphite-app[bot]

This comment was marked as resolved.

@maparent maparent force-pushed the eng-1865-publish-roam-stored-relations-for-shared-nodes branch from e15e0eb to 9e93709 Compare July 13, 2026 14:53
@maparent maparent force-pushed the eng-1865-publish-roam-stored-relations-for-shared-nodes branch from 9e93709 to 123104f Compare July 13, 2026 15:04
@maparent maparent force-pushed the eng-1865-publish-roam-stored-relations-for-shared-nodes branch from 123104f to ae3123f Compare July 13, 2026 15:29
Omit<
Database["public"]["CompositeTypes"]["document_local_input"],
"author_inline"
"author_inline" | "contents"

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"content" is of type unknown, so we get typescript errors when using the LocalDocumentDataInput explicitly (even though indirectly through LocalConceptDataInput, LocalContentDataInput.)
It is not used yet, so just removing from input type for now.
I guess this did not arise earlier because we were building objects, not using the type explicitly.

@maparent maparent force-pushed the eng-1865-publish-roam-stored-relations-for-shared-nodes branch from ae3123f to f71e309 Compare July 13, 2026 15:51
@maparent maparent marked this pull request as draft July 13, 2026 19:51
@maparent maparent force-pushed the eng-2017-add-converter-for-crossapp-schemas-to-localconceptdatainput branch from c2ee00a to 1127f9a Compare July 15, 2026 18:49
@maparent maparent force-pushed the eng-1865-publish-roam-stored-relations-for-shared-nodes branch from f71e309 to a96cd20 Compare July 15, 2026 19:06
@maparent maparent changed the base branch from eng-2017-add-converter-for-crossapp-schemas-to-localconceptdatainput to eng-2018-add-converter-for-crossapprelation-schema-to July 15, 2026 19:07
@maparent maparent marked this pull request as ready for review July 15, 2026 19:08

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 2 new potential issues.

View 1 additional finding in Devin Review.

Open in Devin Review

Comment thread apps/roam/src/utils/publishNodesToGroups.ts
Comment thread apps/roam/src/utils/publishNodesToGroups.ts Outdated
@maparent maparent force-pushed the eng-1865-publish-roam-stored-relations-for-shared-nodes branch from a96cd20 to af01ede Compare July 15, 2026 19:14
@maparent maparent force-pushed the eng-1865-publish-roam-stored-relations-for-shared-nodes branch from af01ede to abacebf Compare July 15, 2026 19:55
Comment on lines +255 to +262
const isImportedFrom = (nodeLocalId: string): number => {
let cached = sourceIdOfNodes[nodeLocalId];
if (cached === undefined) {
cached = sourceIdOfNodes[nodeLocalId] =
getSpaceIdOf(nodeLocalId) || spaceId;
}
return cached === spaceId ? 0 : cached;
};

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Critical Logic Bug: The isImportedFrom function always returns 0 because getSpaceIdOf (line 67) always returns undefined.

// Line 259: getSpaceIdOf always returns undefined
cached = sourceIdOfNodes[nodeLocalId] = undefined || spaceId;
// So cached always equals spaceId

// Line 261: Since cached === spaceId is always true
return cached === spaceId ? 0 : cached;  // Always returns 0

This breaks the logic at lines 279-281 where (isImportedFrom(r.sourceUid) || 0) in groupSpaces evaluates to 0 in groupSpaces, checking if property "0" exists instead of the actual space ID. This will cause relations with imported nodes to be incorrectly filtered.

Similarly at lines 102-104, spaceUids[isImportedFrom(r.sourceUid) || 0] always looks up spaceUids[0] which likely doesn't exist, causing all imported nodes to fall back to local UIDs when they should use RIDs.

Impact: Relations involving imported nodes will not be published correctly to groups. The code will either skip valid relations or fail to construct proper RIDs for cross-space references.

Fix: Either implement getSpaceIdOf properly or remove the imported node logic until ENG-1856 is complete (as mentioned in the PR description).

Spotted by Graphite

Fix in Graphite


Is this helpful? React 👍 or 👎 to let us know.

@maparent maparent Jul 15, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is valid, but it's related to function that is expected to be integrated in ENG-1865 (forthcoming.) So the bug is really a placeholder.

@maparent maparent force-pushed the eng-1865-publish-roam-stored-relations-for-shared-nodes branch from abacebf to 0bb9a97 Compare July 15, 2026 20:25
Comment thread apps/roam/src/utils/publishNodesToGroups.ts
@maparent maparent requested review from mdroidian and removed request for mdroidian July 15, 2026 20:30
@maparent maparent force-pushed the eng-1865-publish-roam-stored-relations-for-shared-nodes branch from 0bb9a97 to 3d229a1 Compare July 15, 2026 20:33
@maparent maparent requested a review from mdroidian July 15, 2026 20:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant