Skip to content

ENG-2248 Add a cached relations index and link resolution - #1433

Open
trangdoan982 wants to merge 4 commits into
mainfrom
eng-2248-add-a-cached-relations-index-and-link-resolution
Open

ENG-2248 Add a cached relations index and link resolution#1433
trangdoan982 wants to merge 4 commits into
mainfrom
eng-2248-add-a-cached-relations-index-and-link-resolution

Conversation

@trangdoan982

Copy link
Copy Markdown
Member

First of three stacked PRs for ENG-1249, tracked by ENG-2248. Stack: this → ENG-2249 → ENG-2250.

https://www.loom.com/share/2f5badd2168546dba687e2844ebc2d46

First of three stacked PRs for ENG-1249. Stack: this → ENG-2249 → ENG-2250.

Reviewer brief

Result: No user-visible change. This adds the data layer the overlay needs; nothing consumes it yet.

Review focus: the concurrency rules in RelationsIndex, and what the relation count deliberately excludes. Both are expanded below.

Why an index exists at all — the cost argument

getRelationsForFile reads and parses the whole of relations.json, then scans every relation in it:

flowchart LR
  A["vault.read relations.json"] --> B["JSON.parse — 50 relations"]
  B --> C["Object.values .filter"]
Loading

That is fine for the Discourse Context panel, which asks once per file open. The overlay asks once per discourse-node link on screen, per viewport update. On a note with 40 such links that is 40 file reads and 40 parses to answer 40 questions about one document — on every keystroke.

RelationsIndex keeps a parsed snapshot grouped by endpoint id, so a lookup is a Map hit and answers synchronously — which matters as much as speed, because a CodeMirror ViewPlugin cannot await anything while building decorations.

flowchart LR
  subgraph store["relations.json"]
    R1["r1: a to b"]
    R2["r2: c to a"]
    R3["r3: a to a"]
  end
  store --> IDX["endpoint index<br>a → r1, r2, r3<br>b → r1<br>c → r2"]
  IDX --> Q["get('a') → 3<br>O(1), no await"]
Loading

Each relation is filed under both endpoints, so "relations touching X" is one lookup regardless of direction. A self-relation like r3 is filed once, not twice, so one endpoint never yields it twice.

The concurrency rules — each one is load-bearing
Rule Why it exists
Generation counter A load that began before an invalidation is stale when it resolves and must not overwrite a newer snapshot. Not exotic: writing a relation modifies the file while a read may be in flight.
inFlight cleared in a finally If a superseded load returns early without clearing it, ensureLoaded hands out an already-settled promise forever — the snapshot stays stale and every later read requests a load that never runs.
Reads never schedule loads A read that triggers a load triggers a notify → re-render → read. Loading belongs to initialize() and invalidate() only.
Invalidation keeps the old snapshot Dropping it would flash every badge to 0 until the reload lands — on the very action that triggered it, since saving a relation writes relations.json.
A version counter So a ViewPlugin, which can only observe transactions, can detect that counts changed. Used by ENG-2249.
What the count excludes, and why it must

"How many relations does this node have?" is ambiguous. A node in the test vault has 6 relations in relations.json; the panel lists 4.

Relations Type still configured? Panel lists it?
r1, r2 no — the type was deleted no
r3–r6 yes yes

Deleting a relation type does not delete its relations; they stay as orphans and the panel drops them while grouping by type. Imported relations awaiting acceptance (tentative === false) are likewise listed separately.

[!IMPORTANT]
The badge counts what the panel would list, not what the store holds. A badge reading 6 above a popover listing 4 advertises context the next click refuses to show. An earlier revision had exactly that bug.

Verification

  • pnpm install --frozen-lockfile + pnpm ci:validate: 8/8 check-types, 5/5 test:unit.
  • Loaded in a real vault over CDP: the index initializes, reports the expected relation count for a known node, and renders no UI.

Scope check

  • Ran $scope-check against the ENG ticket and final diff.
  • Scope beyond Done When: None. Groundwork for the ticket's overlay requirement.

Local delegated full review

  • Ran a comprehensive review of the entire final diff in a subagent with a fresh context.

Review findings addressed here: relations.json renames now invalidate the snapshot, and markdown links with a #heading subpath now match. One finding is open by design — see the thread on discourseLinkFrontmatter.ts.

🤖 Generated with Claude Code

@linear-code

linear-code Bot commented Sep 11, 2026

Copy link
Copy Markdown

ENG-2248

@supabase

supabase Bot commented Sep 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 Sep 11, 2026

Copy link
Copy Markdown

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

1 Skipped Deployment
Project Deployment Actions Updated
discourse-graph Skipped Skipped Sep 11, 2026 3:27am UTC

Request Review

trangdoan982 and others added 4 commits September 10, 2026 23:25
Reading relations from disk costs a full vault file read plus a JSON
parse per call. That is fine for the Discourse Context panel, which asks
once per file open, but not for anything that renders per link.

RelationsIndex keeps a parsed snapshot grouped by endpoint id so a render
path can ask synchronously, rebuilt from vault events so it covers our
own writes and edits arriving over sync alike.

resolveDiscourseLinkTarget answers, for one link, whether it points at a
discourse node and how many relations that node has — from in-memory
caches only, avoiding getNodeTypeIdForFile, which polls 500ms waiting on
frontmatter.

Link parsing moves to internalLinkParsing, which wikilinkDragHandler had
its own byte-identical copy of.

Nothing consumes this yet; the overlay that does follows.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two review findings:

Renaming relations.json out of the root, or another file into it, left the
snapshot stale; only modify/create/delete were watched.

Markdown links with a #heading or #^block subpath never matched, since the
pattern required .md immediately before the closing paren. Wikilinks with
subpaths already worked, so the two forms behaved differently.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review finding: an invalidation arriving mid-read starts a newer load, but
the superseded load's finally cleared inFlight unconditionally, discarding
the newer load's tracking. Later callers then saw no load in flight and
started redundant ones.

Clearing is now gated on the generation still matching, which is the same
signal that decides whether the load's result is worth keeping.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@trangdoan982
trangdoan982 force-pushed the eng-2248-add-a-cached-relations-index-and-link-resolution branch from 509543f to bbab339 Compare September 11, 2026 03:27
@trangdoan982
trangdoan982 added this pull request to stack #1436 September 11, 2026 06:01
@mdroidian

Copy link
Copy Markdown
Member

@coderabbitai full review

@coderabbitai

coderabbitai Bot commented Sep 11, 2026

Copy link
Copy Markdown
Contributor
⚠️ Action not completed

Review rate limited.


Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 59 minutes.

@coderabbitai

coderabbitai Bot commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

Next included review available in 59 minutes.

Check out review usage here.

View limit details

Limit 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.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Repository UI (base), Organization UI (inherited)

Review profile: CHILL

Plan: Advanced

Run ID: 497b68ed-ea0d-4a05-9e61-fb52b93885e4

📥 Commits

Reviewing files that changed from the base of the PR and between 4760514 and bbab339.

📒 Files selected for processing (7)
  • apps/obsidian/src/index.ts
  • apps/obsidian/src/utils/discourseLinkFrontmatter.ts
  • apps/obsidian/src/utils/discourseLinkUtils.ts
  • apps/obsidian/src/utils/internalLinkParsing.ts
  • apps/obsidian/src/utils/relationsEndpointIndex.ts
  • apps/obsidian/src/utils/relationsIndex.ts
  • apps/obsidian/src/utils/wikilinkDragHandler.ts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

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