ENG-2140 Map source to an Obsidian relation on pull - #1411
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. |
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}
| } | ||
|
|
||
| // The metadata cache can lag behind vault writes during a batch import. | ||
| importedFiles.set(importedFromRid, processedFile); |
There was a problem hiding this comment.
The batch map keeps newly imported files available while Obsidian's metadata cache catches up. This prevents two Evidence nodes from importing the same Source twice.
![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: 00ab1cfbec
ℹ️ 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".
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:
|
|
Only |
sid597
left a comment
sid597
left a comment
There was a problem hiding this comment.
Reviewer notes for the importer.
|
|
||
| const { data: sources, error } = await client | ||
| .from("my_concepts") | ||
| .select("id, source_local_id, space_id") |
There was a problem hiding this comment.
sourceDocument contains a database concept ID. This lookup resolves it to a space and local ID before finding a vault file. The Source must be independently shared and visible to the importing user.
| importedFiles.get(rid) ?? | ||
| (source.space_id === localSpaceId | ||
| ? queryEngine | ||
| .getFilesWithNodeTypeId({ excludeImported: true }) |
There was a problem hiding this comment.
A local ID is only unique within its space. Local Source lookup excludes imported notes; remote lookup uses the full origin RID. A same-ID note from another graph must not be reused here.
| } | ||
| } | ||
| if (pendingSources.size > 0) { | ||
| await importNodes({ |
There was a problem hiding this comment.
Automatic Source imports use the normal importer, including schema mapping, decorated titles, and assets. This lookup supplies identity; the importer fetches the title and content. Sources already found in the vault are reused without overwriting their content.
|
|
||
| const localSpaceUri = getLocalSpaceUri(plugin.app); | ||
| const indexedFiles = queryEngine.getFilesWithNodeInstanceId(); | ||
| const legacyEndpointsForFile = ({ |
There was a problem hiding this comment.
Manually created relations may store bare IDs or this vault's RIDs. Those count as aliases only when the ID identifies this file uniquely. Otherwise, a relation between separate local notes could suppress the imported relation.
| const sourceNodeType = plugin.settings.nodeTypes.find( | ||
| (type) => type.id === source.nodeTypeId, | ||
| ); | ||
| if (sourceNodeType?.name.toLowerCase() !== "source") continue; |
There was a problem hiding this comment.
The Source-named type matches Obsidian's push rule in #1381. Both the local triple and relation type must already be accepted. If either is missing or provisional, the notes import without creating a relation or new relation configuration.
| const { frontmatter } = parseFrontmatter( | ||
| await plugin.app.vault.read(occupiedFile), | ||
| ); | ||
| if (frontmatter.importedFromRid === importedFromRid) break; |
There was a problem hiding this comment.
Two Sources can have the same title, so an occupied filename is reused only for the same origin RID. Other Sources get a suffix. The refresh path also recognizes its own suffixed file to avoid repeated renames.
| nodeImportInfoByInstance, | ||
| importedFiles, | ||
| }); | ||
| } catch (error) { |
There was a problem hiding this comment.
Source relations are added after the selected notes have been written. A Source lookup or relation failure must not turn those successful imports into failures. Keep the notes and report the incomplete Source step.
|
@coderabbitai full review |
|
|
Warning Review limit reachedNext included review available in 59 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
2 participants
eng-2140.mp4
Reviewer brief
Review focus: Source identity across spaces, accepted relation configuration, and partial import failures. Inline notes explain the decisions at each step.
Verification
pnpm ci:validatepassed after removing the test setup.Loom video
37-second feature demo above.
Scope check
$scope-checkagainst ENG-2140 and the final diff, with Sid's clarified scope.Done When: None.Local delegated full review