ENG-1249 Implement Discourse context overlay feature in Obsidian - #1400
Open
trangdoan982 wants to merge 8 commits into
Open
ENG-1249 Implement Discourse context overlay feature in Obsidian#1400trangdoan982 wants to merge 8 commits into
trangdoan982 wants to merge 8 commits into
Conversation
Reading relations straight 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 the upcoming discourse context overlay asks once per discourse-node link on screen. RelationsIndex keeps a parsed snapshot of relations.json grouped by endpoint id, so a lookup on a render path is a Map hit and can answer synchronously. It rebuilds from the vault's own modify/create/delete events, which covers writes made through saveRelations as well as edits arriving over sync, so relationsStore does not need to know it exists. Also wires apps/obsidian into the root test:unit task, which it was not previously exposed to. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The overlay has to decide, per link and per viewport update, whether a link points at a discourse node and how many relations it has. Every read here hits an in-memory cache so the answer is available without awaiting. Notably this avoids getNodeTypeIdForFile/getNodeInstanceIdForFile, which poll for up to 500ms waiting on frontmatter for a just-created file. That is correct for relation bookkeeping and wrong on a render path, so an uncached file yields no badge and is picked up on the next redraw. Relations awaiting acceptance are excluded from the count: the Discourse Context panel lists those separately, so including them would show a number the panel never repeats back. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The badge is plain DOM rather than React so the Live Preview widget and the Reading view post processor can share one implementation: neither has a React root, and mounting one per link would be far too heavy. Toggling the setting applies immediately to both surfaces. Live Preview needs an empty CM6 transaction to make ViewPlugins re-evaluate; Reading view has no equivalent, so already-rendered content has to be thrown away and rebuilt. Both helpers move to markdownViewRefresh, which also takes over the copy previously inlined in onload. Neither surface renders a badge yet — that follows. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Live Preview goes through a CM6 ViewPlugin, following the existing wikilink drag handle. Reading view needs a markdown post processor, which this plugin had no precedent for; because Obsidian reuses rendered sections and runs post processors over them again — as it also does for hover previews and exports — that path checks each link for an existing badge rather than assuming it runs once. Selecting a badge opens a popover built on RelationshipSection, the same component the Discourse Context panel renders, so the two cannot disagree about a node's relations. InfoTooltip moves out of DiscourseContextView so using it does not pull in the whole ItemView. Two fixes found while verifying against a real vault: - The badge counted relations the panel hides. Deleting a relation type leaves its relations behind in relations.json, and the panel drops those, so a node with 4 stored relations showed a badge reading 4 over a panel listing 2. Counting now mirrors what the panel will display. - Scrolling inside the popover dismissed it, because the scroll listener that follows the badge away could not tell outside scrolling from the popover's own — which made "Add a new relation" unreachable. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Matches Roam: a node with no relations still gets a badge, reading 0, whose popover says "No discourse relation found" above the option to add one. CurrentRelationships renders nothing at all in that case, so without the message the popover would open on an unexplained button. Also drops the node title from the top of the popover — the badge sits directly after the link whose title it was repeating. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Extends the existing discourse context page with the overlay as a fourth way in, and adds the setting to General settings, rather than adding a new page for a feature that is another entry point to something already documented. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
From a delegated review of the full diff. The substantive ones: Reading view badges never updated. Only the CM6 extension watched the relations index, and the post processor skipped links that already had a badge, so a Reading view badge kept its first number for the life of the view — including the 0 it showed when the index was still loading as the note first rendered. Both surfaces now redraw from one debounced handler, and the post processor replaces a stale badge instead of skipping it. Invalidating the index dropped the snapshot outright, so every badge read 0 until the reload landed — on the very action that triggered it, since saving a relation writes relations.json. The old snapshot is now kept until the new one is ready. ensureLoaded left inFlight set when a load was superseded mid-read, which made it hand out a settled promise forever: the snapshot stayed stale and every read re-requested a load that never ran. Reading from a render path also scheduled loads, so notify -> re-render -> read looped and left Reading view permanently blank. The popover outlived the plugin, keeping an Escape handler that swallowed the key for the rest of the session, and it measured itself before React had committed, so it never flipped up near the bottom of the window. It now closes on unload and derives geometry from the window its anchor is in, which also fixes popout windows. Also: badge no longer moves the caret on mousedown, widget eq() accounts for the node type, link parsing moves to a pure module so it can be tested, and unused accessors are gone. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
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. |
Contributor
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:
|
Drops the three test files, vitest.config.mts, and the test:unit script and vitest devDependency, so apps/obsidian is no longer part of the root test:unit task and the lockfile returns to matching main. The pure helper modules stay: their separation from Obsidian and CodeMirror still keeps the counting, grouping and link-parsing rules readable on their own. Their doc comments no longer cite testability as the reason. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Reviewer brief
Result: Links to a discourse node carry an inline badge with that node's relation count, in both Live Preview and Reading view. Selecting one opens a popover built on
RelationshipSection— the same component the Discourse Context panel renders — so the two cannot disagree. A node with no relations still badges, reading0, and its popover says "No discourse relation found". Toggled by Show discourse context overlay in General settings, applied without a reload.Review focus:
ViewPlugin, following the existing wikilink drag handle. Reading view needs a markdown post processor, which this plugin had no precedent for. They sharecreateDiscourseContextBadgeso they cannot drift visually — worth checking that the two paths stay in step.RelationsIndexis the load-bearing piece.loadRelationscosts a full vault file read plus a JSON parse per call, which is fine once per panel open but not once per link on screen. The index keeps a parsed snapshot so a render path can ask synchronously. Its concurrency rules are subtle and commented: a generation counter (relations.json being written mid-read is the normal case, since saving a relation causes it), the snapshot kept rather than dropped on invalidation, and reads deliberately not scheduling loads.relations.jsonand the panel drops them. SeecountDisplayableRelations.Risk or follow-up:
buildGroupedRelationsalso drops relations whose other endpoint no longer resolves to a file. The badge does not model that, so a relation pointing at a deleted note will still be counted. Not fixed here because it needs a file lookup per relation on a render path; worth a follow-up ticket if orphaned endpoints turn out to be common..mdbefore the closing paren.Verification
pnpm install --frozen-lockfile+pnpm ci:validatefrom the repo root: 8/8 check-types, 4/4 test:unit.apps/obsidianhas no test setup, and adding one was deliberately left out of this PR. The counting, grouping and link-parsing rules are isolated in pure modules (relationsEndpointIndex,discourseLinkFrontmatter,internalLinkParsing) so they can be covered later without restructuring.Three bugs were found by that pass rather than by tests, and are fixed here: the badge over-counting orphaned relations,
ignoreEvent()inverted so clicks never reached the badge, and scrolling inside the popover dismissing it — which made "Add a new relation" unreachable.Scope check
$scope-checkagainst the ENG ticket and final diff.Done When: None. The ticket's three criteria — a General setting, the overlay showing the same relations as the panel, and documentation — are each covered.Local delegated full review
Its high-severity findings are fixed in
c1a7b49f2: Reading view badges never refreshing, the index flashing every badge to0on save, aninFlightleak that wedged the snapshot permanently, a read-triggered refresh loop that left Reading view blank, the popover outliving plugin unload with an Escape handler that swallowed the key, and positioning that measured before React had committed so it never flipped up near the window bottom. Remaining findings are listed under Risk or follow-up above.Loom video
Not recorded. Screenshots of both surfaces and the empty state are attached to the Linear ticket discussion instead.