-
Notifications
You must be signed in to change notification settings - Fork 7
ENG-1249 Add a cached relations index and link resolution #1413
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Closed
+302
−28
Closed
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
6db0626
ENG-1249 Add a cached relations index and link resolution
trangdoan982 4921104
ENG-1249 Handle relations.json renames and markdown subpath links
trangdoan982 9bce55c
ENG-1249 Only clear inFlight when still the current load
trangdoan982 509543f
ENG-1249 Trim a comment that restated its function name
trangdoan982 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,40 @@ | ||
| import type { RelationInstance } from "~/types"; | ||
|
|
||
| const asString = (value: unknown): string | undefined => | ||
| typeof value === "string" && value.length > 0 ? value : undefined; | ||
|
|
||
| export const getNodeTypeIdFromFrontmatter = ( | ||
| frontmatter: Record<string, unknown> | undefined, | ||
| ): string | undefined => asString(frontmatter?.nodeTypeId); | ||
|
|
||
| /** An imported node is referenced by both its nodeInstanceId and its importedFromRid. */ | ||
| export const getEndpointIdsFromFrontmatter = ( | ||
| frontmatter: Record<string, unknown> | undefined, | ||
| ): string[] => { | ||
| const endpointIds: string[] = []; | ||
| const nodeInstanceId = asString(frontmatter?.nodeInstanceId); | ||
| const importedFromRid = asString(frontmatter?.importedFromRid); | ||
|
|
||
| if (nodeInstanceId) endpointIds.push(nodeInstanceId); | ||
| if (importedFromRid && importedFromRid !== nodeInstanceId) { | ||
| endpointIds.push(importedFromRid); | ||
| } | ||
|
|
||
| return endpointIds; | ||
| }; | ||
|
|
||
| /** | ||
| * Counts what the panel would list. Excludes unaccepted imports and relations | ||
| * orphaned by a deleted relation type, both of which the panel hides. | ||
| */ | ||
| export const countDisplayableRelations = ({ | ||
| relations, | ||
| isConfiguredType, | ||
| }: { | ||
| relations: RelationInstance[]; | ||
| isConfiguredType: (relationTypeId: string) => boolean; | ||
| }): number => | ||
| relations.filter( | ||
| (relation) => | ||
| relation.tentative !== false && isConfiguredType(relation.type), | ||
| ).length; | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,58 @@ | ||
| import { parseLinktext, TFile } from "obsidian"; | ||
| import type DiscourseGraphPlugin from "~/index"; | ||
| import type { DiscourseNode } from "~/types"; | ||
| import { getNodeTypeById, getRelationTypeById } from "./typeUtils"; | ||
| import { | ||
| countDisplayableRelations, | ||
| getEndpointIdsFromFrontmatter, | ||
| getNodeTypeIdFromFrontmatter, | ||
| } from "./discourseLinkFrontmatter"; | ||
|
|
||
| export type DiscourseLinkTarget = { | ||
| file: TFile; | ||
| nodeType: DiscourseNode; | ||
| relationCount: number; | ||
| }; | ||
|
|
||
| /** | ||
| * In-memory caches only, so this can run per link on a render path. Avoids | ||
| * getNodeTypeIdForFile, which polls 500ms waiting on frontmatter. | ||
| */ | ||
| export const resolveDiscourseLinkTarget = ({ | ||
| plugin, | ||
| linktext, | ||
| sourcePath, | ||
| }: { | ||
| plugin: DiscourseGraphPlugin; | ||
| linktext: string; | ||
| sourcePath: string; | ||
| }): DiscourseLinkTarget | null => { | ||
| // Strips any #heading or #^block subpath. | ||
| const { path } = parseLinktext(linktext); | ||
| if (!path) return null; | ||
|
|
||
| const file = plugin.app.metadataCache.getFirstLinkpathDest(path, sourcePath); | ||
| if (!file) return null; | ||
|
|
||
| const frontmatter = plugin.app.metadataCache.getFileCache(file)?.frontmatter; | ||
|
|
||
| const nodeTypeId = getNodeTypeIdFromFrontmatter(frontmatter); | ||
| if (!nodeTypeId) return null; | ||
|
|
||
| const nodeType = getNodeTypeById(plugin, nodeTypeId); | ||
| if (!nodeType) return null; | ||
|
|
||
| const endpointIds = getEndpointIdsFromFrontmatter(frontmatter); | ||
| if (endpointIds.length === 0) return { file, nodeType, relationCount: 0 }; | ||
|
|
||
| const relations = | ||
| plugin.relationsIndex.getRelationsForEndpointIds(endpointIds); | ||
|
|
||
| const relationCount = countDisplayableRelations({ | ||
| relations, | ||
| isConfiguredType: (relationTypeId) => | ||
| !!getRelationTypeById(plugin, relationTypeId), | ||
| }); | ||
|
|
||
| return { file, nodeType, relationCount }; | ||
| }; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
| // Shared by the CM6 extensions that scan raw markdown for internal links. | ||
|
|
||
| /** Embeds are not matched: the leading `!` sits outside, so callers check it. */ | ||
| export const INTERNAL_LINK_RE = | ||
| /\[\[([^\]]+)\]\]|\[([^\]]+)\]\(([^)]+\.md(?:#[^)]*)?)\)/g; | ||
|
|
||
| /** Target of a wikilink or markdown link; any `#subpath` is left for parseLinktext. */ | ||
| export const extractLinktext = (match: string): string => { | ||
| if (match.startsWith("[[")) { | ||
| const inner = match.slice(2, -2); | ||
| const pipeIndex = inner.indexOf("|"); | ||
| return pipeIndex >= 0 ? inner.slice(0, pipeIndex) : inner; | ||
| } | ||
|
|
||
| const parenOpen = match.lastIndexOf("("); | ||
| const rawPath = match.slice(parenOpen + 1, -1); | ||
| try { | ||
| return decodeURIComponent(rawPath); | ||
| } catch { | ||
| return rawPath; | ||
| } | ||
| }; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,54 @@ | ||
| import type { RelationInstance } from "~/types"; | ||
|
|
||
| /** | ||
| * Groups relations by the ids at either end, so a lookup is a Map hit rather | ||
| * than a scan. Self-relations are filed once, not twice. | ||
| */ | ||
| export const buildEndpointIndex = ( | ||
| relations: Record<string, RelationInstance>, | ||
| ): Map<string, RelationInstance[]> => { | ||
| const index = new Map<string, RelationInstance[]>(); | ||
|
|
||
| const fileUnder = (endpointId: string, relation: RelationInstance): void => { | ||
| const existing = index.get(endpointId); | ||
| if (existing) { | ||
| existing.push(relation); | ||
| return; | ||
| } | ||
| index.set(endpointId, [relation]); | ||
| }; | ||
|
|
||
| for (const relation of Object.values(relations)) { | ||
| if (!relation) continue; | ||
| if (relation.source) fileUnder(relation.source, relation); | ||
| if (relation.destination && relation.destination !== relation.source) { | ||
| fileUnder(relation.destination, relation); | ||
| } | ||
| } | ||
|
|
||
| return index; | ||
| }; | ||
|
|
||
| /** Relations touching any of `endpointIds`, deduped: an imported node matches on two ids. */ | ||
| export const collectRelations = ({ | ||
| index, | ||
| endpointIds, | ||
| }: { | ||
| index: Map<string, RelationInstance[]>; | ||
| endpointIds: Iterable<string>; | ||
| }): RelationInstance[] => { | ||
| const seen = new Set<string>(); | ||
| const collected: RelationInstance[] = []; | ||
|
|
||
| for (const endpointId of endpointIds) { | ||
| const relations = index.get(endpointId); | ||
| if (!relations) continue; | ||
| for (const relation of relations) { | ||
| if (seen.has(relation.id)) continue; | ||
| seen.add(relation.id); | ||
| collected.push(relation); | ||
| } | ||
| } | ||
|
|
||
| return collected; | ||
| }; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,120 @@ | ||
| import { TAbstractFile, TFile } from "obsidian"; | ||
| import type DiscourseGraphPlugin from "~/index"; | ||
| import type { RelationInstance } from "~/types"; | ||
| import { getRelationsFilePath, loadRelations } from "./relationsStore"; | ||
| import { buildEndpointIndex, collectRelations } from "./relationsEndpointIndex"; | ||
|
|
||
| /** | ||
| * Parsed snapshot of relations.json so a render path can ask synchronously, | ||
| * rebuilt from vault events (which covers our own writes and sync alike). | ||
| */ | ||
| export class RelationsIndex { | ||
| private plugin: DiscourseGraphPlugin; | ||
| private index: Map<string, RelationInstance[]> | null = null; | ||
| private inFlight: Promise<void> | null = null; | ||
| private stale = false; | ||
| private unloaded = false; | ||
| /** Lets a ViewPlugin, which only sees transactions, detect a changed snapshot. */ | ||
| private version = 0; | ||
| private subscribers = new Set<() => void>(); | ||
| /** Guards against a load that started before an invalidation overwriting a newer one. */ | ||
| private generation = 0; | ||
|
|
||
| constructor(plugin: DiscourseGraphPlugin) { | ||
| this.plugin = plugin; | ||
| } | ||
|
|
||
| initialize(): void { | ||
| const invalidateIfRelationsFile = (file: TAbstractFile): void => { | ||
| if (!(file instanceof TFile)) return; | ||
| if (file.path !== getRelationsFilePath()) return; | ||
| this.invalidate(); | ||
| }; | ||
|
|
||
| const { vault } = this.plugin.app; | ||
| this.plugin.registerEvent(vault.on("modify", invalidateIfRelationsFile)); | ||
| this.plugin.registerEvent(vault.on("create", invalidateIfRelationsFile)); | ||
| this.plugin.registerEvent(vault.on("delete", invalidateIfRelationsFile)); | ||
|
trangdoan982 marked this conversation as resolved.
|
||
| // Both directions: the file moving out of the root, and one moving in. | ||
| this.plugin.registerEvent( | ||
| vault.on("rename", (file, oldPath) => { | ||
| if (oldPath === getRelationsFilePath()) this.invalidate(); | ||
| else invalidateIfRelationsFile(file); | ||
| }), | ||
| ); | ||
|
|
||
| void this.ensureLoaded(); | ||
| } | ||
|
|
||
| unload(): void { | ||
| this.unloaded = true; | ||
| this.subscribers.clear(); | ||
| this.index = null; | ||
| this.inFlight = null; | ||
| this.generation += 1; | ||
| } | ||
|
|
||
| /** Changes whenever the snapshot is replaced; see the field comment. */ | ||
| getVersion(): number { | ||
| return this.version; | ||
| } | ||
|
|
||
| /** Fires when the snapshot changes. Returns an unsubscribe function. */ | ||
| onChange(subscriber: () => void): () => void { | ||
| this.subscribers.add(subscriber); | ||
| return () => this.subscribers.delete(subscriber); | ||
| } | ||
|
|
||
| async ensureLoaded(): Promise<void> { | ||
| if (this.unloaded) return; | ||
| if (this.index !== null && !this.stale) return; | ||
| if (this.inFlight) return this.inFlight; | ||
|
|
||
| const generation = this.generation; | ||
| this.inFlight = (async () => { | ||
| try { | ||
| const relationsFile = await loadRelations(this.plugin); | ||
| // Superseded mid-read; the invalidation already scheduled a reload. | ||
| if (generation !== this.generation || this.unloaded) return; | ||
| this.index = buildEndpointIndex(relationsFile.relations ?? {}); | ||
| this.stale = false; | ||
| this.version += 1; | ||
| } finally { | ||
| // Only if still the current load: an invalidation mid-read starts a | ||
| // newer one, and clearing unconditionally would discard its tracking. | ||
| if (generation === this.generation) this.inFlight = null; | ||
| } | ||
|
trangdoan982 marked this conversation as resolved.
|
||
| // The skipped invalidation above still needs a load of its own. | ||
| if (this.stale && !this.unloaded) { | ||
| void this.ensureLoaded(); | ||
| return; | ||
| } | ||
| this.notify(); | ||
| })(); | ||
|
|
||
| return this.inFlight; | ||
| } | ||
|
|
||
| /** | ||
| * Empty while cold, so treat that as "not loaded yet", not "no relations". | ||
| * Never schedules a load: that would make notify -> re-render -> read loop. | ||
| */ | ||
| getRelationsForEndpointIds( | ||
| endpointIds: Iterable<string>, | ||
| ): RelationInstance[] { | ||
| if (this.index === null) return []; | ||
| return collectRelations({ index: this.index, endpointIds }); | ||
| } | ||
|
|
||
| /** Keeps the old snapshot while reloading, so badges do not flash to 0. */ | ||
| private invalidate(): void { | ||
| this.generation += 1; | ||
| this.inFlight = null; | ||
| this.stale = true; | ||
| void this.ensureLoaded(); | ||
| } | ||
|
|
||
| private notify(): void { | ||
| for (const subscriber of this.subscribers) subscriber(); | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 Imported local relations disappear
Accepted imported relations vanish from local-node counts because
getEndpointIdsFromFrontmatteromits the local node's canonical RID. The importer stores local endpoints under that RID in getLocalNodeKeyToEndpointId.Prompt for agents
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not fixing here, deliberately — flagging why rather than resolving silently.
You're right that
importRelationsForImportedNodescan store a local node's endpoint as the canonical RID fromgetLocalNodeKeyToEndpointId, and thatgetEndpointIdsFromFrontmatterdoesn't query it.But the same gap already exists in
getRelationsForFile(relationsStore.ts), which is what the Discourse Context panel uses — it also matches onlynodeInstanceIdandimportedFromRid. So this is pre-existing in the data layer, not introduced here.That matters because the overlay's contract is that its count equals what the panel will list; an earlier revision of this work had the badge over-count and it was confusing precisely because the popover then showed fewer rows. Adding the canonical RID only on the badge side would recreate that inconsistency in the other direction.
It should be fixed in
getRelationsForFileso both surfaces gain it together, which is a separate change with its own test surface. I checked this vault: no relation endpoint currently resolves to a local non-imported node this way, so nothing is being dropped today.