ENG-2250 Implement Discourse context overlay in Reading view - #1435
Open
trangdoan982 wants to merge 3 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
2 tasks
Reading view needs a markdown post processor, which this plugin had no precedent for. Because Obsidian reuses rendered sections and re-runs post processors over them — as it also does for hover previews and exports — the pass is idempotent per link rather than one-shot. It is refreshed by re-applying badges over the already-rendered content rather than by calling previewMode.rerender(), which tears the preview down and does not rebuild it in a pane that is not currently painting. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review finding: the Reading view refresh scans the whole preview container and passes the view's own file path, but links inside a transclusion resolve against the embedded file. Refreshing them that way could drop a badge or show another node's count. The refresh now skips links inside an embed. The post processor still badges them correctly on render, where Obsidian supplies the embedded file's source path; the cost is that a count inside an embed updates on the next render rather than immediately. Also updates an existing badge in place rather than replacing it when the target is unchanged, so an open popover keeps a connected anchor. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
trangdoan982
force-pushed
the
eng-2250-implement-discourse-context-overlay-in-reading-view
branch
from
September 11, 2026 03:27
c98d27f to
c6fc65e
Compare
trangdoan982
added this pull request to stack #1436
September 11, 2026 06:01
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
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.
Third of three stacked PRs for ENG-1249, tracked by ENG-2250. Stack: ENG-2248 → ENG-2249 → this. Based on the ENG-2249 branch — this diff is against it.
https://www.loom.com/share/c5c50f3fa8684bfc8fed6f14ed461b4e
Reviewer brief
Result: The badge from ENG-2249 now also renders in Reading view, with the same counts and the same popover. Completes the ticket.
Review focus: why this is a second implementation rather than a shared one, and why the refresh does not call
rerender(). Both expanded below.Why Reading view needs its own implementation
Live Preview and Reading view are not two skins over one renderer. They are two engines:
ViewPlugin[[Note]]in a buffer<a class="internal-link" data-href="Note">They meet at the badge element, which is why the two surfaces look identical. This plugin had no markdown post processor before this PR, so there was no precedent to copy.
Why the refresh does not call previewMode.rerender()
Reading view has no equivalent of CM6's update cycle, so something has to re-run the pass when a count changes. The obvious call is
previewMode.rerender()— and it is a trap.flowchart TD A["relation changes"] --> B{"how to refresh?"} B -->|"rerender()"| C["preview torn down"] C --> D["pane not painting<br>never rebuilds"] D --> E["Reading view blank, permanently"] B -->|"re-apply in place"| F["badges updated"] F --> G["content untouched"]Re-applying badges over already-rendered content is both non-destructive and far cheaper than re-rendering a document to change one integer.
Embedded content is deliberately skipped on refresh: links inside a transclusion resolve against the embedded file, and only the post processor is handed that file's
sourcePath. Resolving them against the outer note could drop a badge or show another node's count. The cost is that a count inside an embed updates on the next render rather than immediately.Verification
pnpm ci:validate: 8/8 check-types, 5/5 test:unit.Tip
Re-running that suite: Obsidian only renders Reading view while its window is frontmost, so a backgrounded app reads as "no badges" regardless of the code. The harness activates the window first.
Scope check
$scope-checkagainst the ENG ticket and final diff.Done When: None.Local delegated full review
Review findings addressed here: embedded links are left to the post processor on refresh, and an existing badge is updated in place rather than replaced when its target is unchanged.
Warning
Known limitation: badges are emitted into Export to PDF/HTML output, since post processors run there too.
🤖 Generated with Claude Code