Skip to content

eng-1863 Relation cross-app contract#1197

Draft
maparent wants to merge 1 commit into
mainfrom
eng-1863-define-cross-app-relation-contract-and-fixture
Draft

eng-1863 Relation cross-app contract#1197
maparent wants to merge 1 commit into
mainfrom
eng-1863-define-cross-app-relation-contract-and-fixture

Conversation

@maparent

@maparent maparent commented Jul 6, 2026

Copy link
Copy Markdown
Collaborator

@linear-code

linear-code Bot commented Jul 6, 2026

Copy link
Copy Markdown

ENG-1863

@vercel

vercel Bot commented Jul 6, 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 10, 2026 1:42pm

Request Review

@supabase

supabase Bot commented Jul 6, 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 ↗︎.

@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 1 potential issue.

Open in Devin Review

Comment thread packages/database/src/crossAppContracts.ts Outdated
@maparent maparent force-pushed the eng-1984-define-cross-app-schema-contracts branch from 562c561 to f1e49e1 Compare July 6, 2026 14:56
@maparent maparent force-pushed the eng-1863-define-cross-app-relation-contract-and-fixture branch from 4eb544e to a4c65c3 Compare July 6, 2026 15:17
@maparent maparent force-pushed the eng-1984-define-cross-app-schema-contracts branch from f1e49e1 to 3af1c5f Compare July 8, 2026 13:50
@maparent maparent force-pushed the eng-1863-define-cross-app-relation-contract-and-fixture branch from a4c65c3 to 1420c0c Compare July 8, 2026 13:51
@maparent maparent force-pushed the eng-1863-define-cross-app-relation-contract-and-fixture branch from 1420c0c to 45bd5a1 Compare July 9, 2026 18:16
@maparent maparent force-pushed the eng-1984-define-cross-app-schema-contracts branch from 3af1c5f to 48cf038 Compare July 9, 2026 18:16
@graphite-app

graphite-app Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

PR size/scope check

This PR is over our review-size guideline.

  • Recommended: ~200 lines changed
  • Acceptable limit: up to 400 lines when well-scoped/self-contained
  • Preferred file count: fewer than 5 files

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:

  • What single problem this PR solves
  • Why the files/changes are coupled

@maparent maparent force-pushed the eng-1863-define-cross-app-relation-contract-and-fixture branch from 45bd5a1 to e52b69e Compare July 9, 2026 18:28
@maparent maparent force-pushed the eng-1863-define-cross-app-relation-contract-and-fixture branch from e52b69e to 43ae6ef Compare July 9, 2026 18:33
@maparent maparent force-pushed the eng-1984-define-cross-app-schema-contracts branch from d93f832 to a116229 Compare July 9, 2026 18:35
@maparent maparent force-pushed the eng-1863-define-cross-app-relation-contract-and-fixture branch from 43ae6ef to 8ea3d29 Compare July 10, 2026 00:20
Base automatically changed from eng-1984-define-cross-app-schema-contracts to main July 10, 2026 12:52
@maparent maparent force-pushed the eng-1863-define-cross-app-relation-contract-and-fixture branch from 8ea3d29 to ab64936 Compare July 10, 2026 13:01
@maparent maparent force-pushed the eng-1863-define-cross-app-relation-contract-and-fixture branch from ab64936 to f14ab44 Compare July 10, 2026 13:40
@maparent maparent requested review from mdroidian and sid597 July 10, 2026 13:44
Comment on lines +21 to +37
| DbRef
| {
localId: string;
// Treat as LocalRef if space is absent
space?: SpaceRef;
// make the options mutually exclusive
dbId?: never;
rid?: never;
}
| {
// A string that contains combined space and localId
rid: string;
// make the options mutually exclusive
dbId?: never;
localId?: never;
space?: never;
};

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Are all of these union variants required? If so, could we add examples showing where each is already used or needed by upcoming work? Otherwise, I’d prefer to keep the contract limited to the variants we currently need and add others when a concrete use case arises.

Including speculative variants increases the contract’s conceptual surface area and requires additional validation, tests, and converter paths, which may create unnecessary confusion and complexity.

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.

We do use the Rid in

(sourceNode.frontmatter.importedFromRid as string | undefined) ??

And I expect this pattern to hold: We store a Rid and send it back.
We do not currently have a version that requires space directly at the upsert_concept call points, because the burden of computing the Rid from space+localId is currently on the platform (8 calls to spaceUriAndLocalIdToRid in the Obsidian code base, many upstream of upsert_concept.)
Having the space+localId variant in the contract will help centralize the Rid computation in the database package.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks, that confirms the { rid } variant is required.

I agree that centralizing the spaceUri + localId → RID normalization could be useful. What I’m trying to separate is centralizing the RID implementation—which already lives in @repo/database/lib/rid—from expanding the shared relation contract to accept another representation of the same identity.

The existing Obsidian call sites use RIDs for several platform-level purposes, including imported provenance, duplicate detection, previews, and endpoint lookup. Because of that, moving normalization into the upsert converter would not replace all eight call sites.

This rationale also seems to support, at most, a { localId, spaceUri } input. I don’t see the same justification yet for the top-level { dbId }, { localId, space: { dbId } }, or the sourceApp property. The current converter does not use sourceApp, and its space.dbId branch is explicitly unresolved.

I’d prefer to keep the shared endpoint contract as LocalRef | RidRef for now:

type RelationEndpointRef =
  | { localId: string }
  | { rid: string };

This gives the shared boundary one canonical representation for remote identity: an RID. A platform that naturally has spaceUri + localId can normalize it before constructing the contract.

If an upcoming Roam or Obsidian producer genuinely benefits from emitting { localId, spaceUri }, we can add that form alongside the producer and a test demonstrating its normalization to the expected RID.

@maparent maparent removed the request for review from sid597 July 13, 2026 19:53
@maparent maparent marked this pull request as draft July 13, 2026 19:53
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.

2 participants