Skip to content

ENG-2250 Implement Discourse context overlay in Reading view - #1435

Open
trangdoan982 wants to merge 3 commits into
eng-2249-implement-discourse-context-overlay-in-live-previewfrom
eng-2250-implement-discourse-context-overlay-in-reading-view
Open

ENG-2250 Implement Discourse context overlay in Reading view#1435
trangdoan982 wants to merge 3 commits into
eng-2249-implement-discourse-context-overlay-in-live-previewfrom
eng-2250-implement-discourse-context-overlay-in-reading-view

Conversation

@trangdoan982

Copy link
Copy Markdown
Member

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:

Live Preview Reading view
What it is a CodeMirror 6 editor Markdown compiled to static HTML
Decorated via a CM6 ViewPlugin a markdown post processor
A link looks like the characters [[Note]] in a buffer <a class="internal-link" data-href="Note">
Redraw triggered by a CM6 transaction nothing, once rendered
flowchart TD
  LP["Live Preview<br>scans raw [[...]] text"] --> B["shared badge element"]
  RV["Reading view<br>finds a.internal-link"] --> B
Loading

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.

[!IMPORTANT]
Obsidian reuses rendered sections and runs post processors over them again — and the same path serves hover previews and exports. The pass therefore has to be idempotent per link rather than one-shot: it updates or replaces an existing badge instead of assuming there is none.

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"]
Loading

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.
  • Full CDP suite against a real vault, 19 assertions, stable across repeated runs: badges in both surfaces with matching counts, badge count equal to the rows the popover lists, popover open/scroll/dismiss, the empty state, the setting toggling both surfaces, counts following a relation added and removed outside the app, and the popover surviving a relation change with a connected anchor.

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

  • Ran $scope-check against the ENG ticket and final diff.
  • Scope beyond Done When: None.

Local delegated full review

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

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

@linear-code

linear-code Bot commented Sep 11, 2026

Copy link
Copy Markdown

ENG-2250

@vercel

vercel Bot commented Sep 11, 2026

Copy link
Copy Markdown

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

Project Deployment Actions Updated
discourse-graph Ready Ready Preview Sep 11, 2026 3:30am UTC

Request Review

@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 ↗︎.

trangdoan982 and others added 3 commits September 10, 2026 23:26
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
trangdoan982 force-pushed the eng-2250-implement-discourse-context-overlay-in-reading-view branch from c98d27f to c6fc65e Compare September 11, 2026 03:27
@trangdoan982
trangdoan982 added this pull request to stack #1436 September 11, 2026 06:01
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.

1 participant