ENG-2139 Map the Obsidian source relation to source on push - #1381
ENG-2139 Map the Obsidian source relation to source on push#1381sid597 wants to merge 3 commits into
source on push#1381Conversation
|
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. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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)
sid597
left a comment
sid597
left a comment
There was a problem hiding this comment.
Inline notes. Three are marked "Decision for the group".
| // that type. Relation endpoints are stored as a nodeInstanceId, as this vault's RID for | ||
| // it, or as an imported node's origin RID, so lookups go through an index of all three. | ||
|
|
||
| export const SOURCE_SLOT = "sourceDocument"; |
There was a problem hiding this comment.
SOURCE_SLOT is also defined in apps/roam/src/utils/sourceSlot.ts:12, and crossAppConverters.ts:92 and sharedNodes.ts:212 depend on the same key. Its one home is packages/database/src/crossAppContracts.ts. Hoisting touches roam and database, so this PR stays inside apps/obsidian.
Decision for the group: hoist here, or ticket the hoist and merge with the duplicate?
| return index; | ||
| }; | ||
|
|
||
| const isSourceNodeType = (nodeType: DiscourseNode | undefined): boolean => |
There was a problem hiding this comment.
Name match follows Roam's sourceNodeType (#1329). Obsidian node type ids are generated at load (constants.ts:36), so the name is the only stable handle. A vault that renames the Source type gets no slot until slots become a node type setting.
| const byCreatedThenId = (a: RelationInstance, b: RelationInstance): number => | ||
| a.created - b.created || a.id.localeCompare(b.id); | ||
|
|
||
| const sourceDocumentIdOf = (node: DiscourseNodeInVault): string => { |
There was a problem hiding this comment.
importedFromRid first, nodeInstanceId otherwise: the rule relationInstanceToLocalConcept already uses for relation endpoints. rid_or_local_id_to_concept_db_id resolves either form.
| sourceDocumentNode: DiscourseNodeInVault; | ||
| }; | ||
|
|
||
| export const indexSourceSlotValues = ({ |
There was a problem hiding this comment.
No schema gate. Roam writes the slot only for node types whose format has {source}. Obsidian formats have no placeholder, so any node with a relation to a Source gets the slot, and the earliest relation wins across relation types.
Nothing downstream reads the gate: arity and is_relation derive from the schema's roles, and sharedNodes.ts reads the keys. Obsidian schemas write no roles, which the ticket puts out of scope, so Obsidian Evidence stays arity 0 while Roam Evidence is 1. Flagging the asymmetry in case it matters for the read side.
| const nodesByEndpoint = indexNodesByEndpoint({ nodes, localSpaceUri }); | ||
| const earliestByNodeId: Record<string, SourceCandidate> = {}; | ||
| for (const relation of relations) { | ||
| if (relation.tentative === false) continue; |
There was a problem hiding this comment.
Imported relations not yet accepted (tentative === false) do not define a node's source. Same filter the relation batch applies in convertDgToSupabaseConcepts.
| schema_represented_by_local_id: nodeTypeId as string, | ||
| is_schema: false, | ||
| literal_content, | ||
| // A value the database cannot resolve to a concept fails this row's upsert (-2). |
There was a problem hiding this comment.
upsert_concepts resolves this value with rid_or_local_id_to_concept_db_id. When that returns NULL, the row's reference_content becomes NULL, the insert fails, and the function returns -2 for the row. The client reads only error, so the node's title update is dropped with no signal.
Triggers: the Source was created while sync was off and the Evidence syncs alone; or an imported Source whose origin space later unshares it (the lookup runs under RLS). Relation instances already have this failure mode.
Decision for the group: keep as is and ticket reading the negative returns into PostHog, or check existence before writing the slot, as ENG-2141 plans for the Roam publish path?
![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: d6bae46ad8
ℹ️ 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".
| // A relation added, accepted, or removed changes no note, so the stored slot is compared | ||
| // with the wanted one on full sync. The comparison is by local id: the database stores | ||
| // the Source's concept id, and an imported Source is wanted by its origin RID. | ||
| export const findStaleSourceSlotNodeIds = ({ |
There was a problem hiding this comment.
Compared by local id. The database stores the Source's concept id; the concepts_of_relation embed gives that concept's source_local_id; an imported Source is wanted by its origin RID, reduced to its local part. A Source whose origin concept is hidden (unshared) yields no match, so that node re-queues on every full sync and its row fails with -2 until the concept resolves. Same failure the converter comment names, now repeated per full sync rather than once.
| supabaseClient | ||
| .from("my_concepts") | ||
| .select(CORE_TITLE_PROBE_SELECT) | ||
| .select(`${CORE_TITLE_PROBE_SELECT}, ${SOURCE_SLOT_PROBE_SELECT}`) |
There was a problem hiding this comment.
Same probe ENG-2155 uses to backfill core_title, extended with the slot columns, so the comparison adds no query. The embed is the one sharedNodes.ts uses on the read side.
| missingCoreTitleIds = | ||
| partitionByCoreTitle(existingConceptIds).missingCoreTitleIds; | ||
| if (sourceSlotByNodeId) | ||
| staleSourceSlotIds = findStaleSourceSlotNodeIds({ |
There was a problem hiding this comment.
Stale nodes join changedNodes with changeTypes: []. createNodeContentEntries returns early for them, so only the concept row rewrites; no content or embeddings.
| return { changedNodes }; | ||
| }; | ||
|
|
||
| const indexSourceSlots = async ({ |
There was a problem hiding this comment.
Built once here so full sync can compare, then passed on to convertDgToSupabaseConcepts. The incremental path has no allNodes at this point, so that function keeps an inline fallback.
| return { spaceId: concept.space_id, localId: concept.source_local_id }; | ||
| }; | ||
|
|
||
| const storedMatchesWanted = ({ |
There was a problem hiding this comment.
The stored slot is compared by which side of the space boundary it resolves to, plus local id. A local Source must resolve to a concept in this space; an imported Source, wanted by its origin RID, to a concept in another space. asSimpleLocalId in dbToCrossAppConverters.ts makes the same distinction on the read side. Two foreign spaces sharing a local id would still compare equal; local ids are uuidv7 or Roam uids, so that case is not guarded.
| relationInstancesData ?? (await loadRelations(plugin)); | ||
| const relationInstances = Object.values(relationInstancesData.relations); | ||
| sourceSlotByNodeId = | ||
| sourceSlotByNodeId ?? |
There was a problem hiding this comment.
The index built here keys nodes by all three endpoint forms because an imported Source is stored as its RID. allNodesById above stays keyed by bare nodeInstanceId: relationInstanceToLocalConcept and ensurePublishedRelationsAccuracy look up bare ids and drop RID endpoints today, and widening them changes relation push, which this ticket does not own.
|
Decision: when the slot reaches the database Question: a node concept is rewritten only when its own file changes, so a source relation added, accepted, or removed never reached Options:
Decision (sid, 2026-09-02): option 2, in this PR. Three independent reviewers flagged the same gap, and the Done When line "a node with no matching source relation omits the source value" is false after a removal without it. Result: commits 63a964a and a0160ac. Known limit: Still open for the group, in inline threads: where |
1 participant
Reviewer brief
local_reference_content.sourceDocumentwhen it has a relation to a node of the Source type. The value is the Source'simportedFromRidwhen it was imported, else itsnodeInstanceId. With several such relations, the earliest bycreated(thenid) wins. With none, the key is omitted. Full sync re-upserts every node whose stored slot differs from the one its relations call for, so a relation added, accepted, or removed reaches the database at the next full sync or publish, and existing vaults backfill on their first full sync.apps/obsidian/src/utils/sourceSlot.ts.indexSourceSlotValuesdoes one pass overrelations.jsonand returnsRecord<nodeInstanceId, sourceDocumentId>; the node converter reads one entry.findStaleSourceSlotNodeIdscompares that map with the stored slot, read through the full-sync probe that already backfillscore_title(buildChangedNodesFromNodes), by local id. The Source type is matched by name, as Roam does inapps/roam/src/utils/sourceSlot.ts(ENG-2128 Express source as asourceDocumentslot/reference of Evidence #1329). Two inline comments carry decisions for the group: whereSOURCE_SLOTshould live, and the silent-2when a value does not resolve.publishNewRelationsyncs relations only, so a relation added between two published nodes reaches the slot at the next full sync or publish, not at creation.-2on every full sync until it resolves, so that node's title stops syncing too. Nothing surfaces: the client reads onlyerrorfromupsert_concepts. Relation instances already carry this failure; a ticket for reading the negative returns into PostHog follows. See the inline comment on the converter.apps/obsidianhas no unit-test runner; Use vault fallback while Datacore initializes #1258 and ENG-1910–ENG-1925 v0 content model: canonical ATJSON storage #1366 each add one. The Done When line "Tests cover zero, one, and multiple matching relations" is not met here and goes with the harness question to ENG-2143.dbToCrossAppConverters.tsemits no instanceslots, andsharedNodes.ts:208builds the cross-space slot RID without thenotesubtype Obsidian stores inimportedFromRid. ENG-2140 and ENG-2142.roles, so Obsidian Evidence keepsarity 0while Roam Evidence declaresroles: ["sourceDocument"]. Slot-schema work is out of scope per the ticket.Verification
eslint --max-warnings 0,prettier --check,tsc --noEmit --skipLibCheckon the three files: clean for this change. The two eslint warnings and 20 tsc errors inapps/obsidianare present on main at untouched lines.turbo run test:unit: 32 files pass (roam 25, database 5, content-model 2).pnpm ci:validateis red locally on main as well (obsidian, ui, website type errors from local type resolution). CI on main is green at ef37b20.Loom video
eng-2139-20260907.mp4
Scope check
$scope-checkagainst ENG-2139 and the final diff.Done When: None. The full-sync comparison is what makes "a node with no matching source relation omits the source value" and "repeated pushes produce the same single source value" hold after a relation changes.Local delegated full review
$dg-delegated-full-reviewwhen no other full-review workflow is available.