Skip to content

Fix diagrams disappearing after switching tabs#4

Closed
petrovo-as wants to merge 1 commit into
scos-lab:mainfrom
petrovo-as:fix/diagram-claim-lock
Closed

Fix diagrams disappearing after switching tabs#4
petrovo-as wants to merge 1 commit into
scos-lab:mainfrom
petrovo-as:fix/diagram-claim-lock

Conversation

@petrovo-as

Copy link
Copy Markdown

Problem

Mermaid and Vega diagrams render on first open, but after switching to
another tab and back they stay as plain code blocks.

Cause

renderMermaidBlocks() marks blocks with data-mermaid-claimed so two
concurrent passes cannot race on the same <pre>:

blocks.forEach((b) => b.setAttribute('data-mermaid-claimed', '1'));

The attribute is never removed, and the block selector excludes claimed
blocks:

'code.language-mermaid:not([data-mermaid-rendered]):not([data-mermaid-claimed])'

So a block left marked is skipped by every later pass.

The MarkdownView effect cancels its render on each renderedHTML or
theme change via the cleanup. When the cancel lands inside the loop —
after the claim, before host.replaceWith(wrapper) — the remaining blocks
keep the claim forever.

That is exactly the tab-switch path: the first open renders uninterrupted,
while switching tabs changes renderedHTML (and often theme right after),
cancelling the in-flight render partway through.

Note the related inconsistency: the early-exit query (initial) does not
exclude claimed blocks, so the function does not bail out at the
initial.length === 0 guard — it proceeds and then finds nothing to do.

Fix

Release the claim in a finally block for every block that was not replaced.

vega.ts has the identical bug with data-vega-claimed and is fixed the
same way.

Verification

Reproduced and fixed under Linux (AppImage build, MarkView 1.0.5) with
vouchers.md and payments.md, both containing mermaid flowcharts.

I also covered the mermaid path with a unit test that drives
renderMermaidBlocks() with a cancel firing inside the loop, then runs a
second pass. It fails against the current code (block stuck in
data-mermaid-claimed, no SVG on the second pass) and passes with this
change. Happy to include it if you want to add a test setup — the project
has none today, so I left it out of this PR.

The vega.ts change is the same pattern, reviewed by reading rather than
executed.


Investigated, fixed and tested with Claude Opus 4.8 (Anthropic) in an
AI-assisted session. I reviewed the diagnosis and verified the fix locally
before opening this PR. The commit carries a Co-Authored-By trailer for
the same reason.

renderMermaidBlocks() and renderVegaBlocks() mark blocks with a
data-*-claimed attribute to keep concurrent passes from racing on the
same <pre>, but the attribute was never removed. The block selector
excludes claimed blocks, so any block left marked is skipped forever.

When a render is cancelled mid-loop -- which the MarkdownView effect
does on every content or theme change via its cleanup -- the remaining
blocks keep the claim. The next pass finds nothing to render and the
diagram stays a plain code block.

This is why diagrams render on first open but vanish after switching
tabs: the initial render completes uninterrupted, while a tab switch
changes renderedHTML (and often theme right after), cancelling the
in-flight render partway through.

Release the claim in a finally block for every block that was not
replaced.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@petrovo-as

Copy link
Copy Markdown
Author

Please hold off on merging this — I no longer think it's the right fix, and I'd rather say so than let it land.

First, what I got wrong: I never verified this in a running build. The comparison I based it on turned out to be invalid — both binaries I was comparing contained the patch, so the "before" case was never actually observed.

More importantly, the finally block does release the claim, but it doesn't close the race it's meant to close. With two passes over the same DOM (a content or theme change while a render is in flight):

  1. Pass A claims the remaining blocks and awaits mermaid.render().
  2. The effect cleanup cancels pass A, and pass B starts.
  3. Pass B re-queries with :not([data-mermaid-claimed]). The blocks are still claimed by A, so B finds nothing to do and returns.
  4. Pass A resumes, hits isCancelled(), returns — and only now does its finally release the claims.

The blocks end up unclaimed, unrendered, and with no further pass to pick them up. The diagram stays a plain code block until something changes renderedHTML again. The cancelled pass still swallows work the superseding pass assumed it had done; the claim/release protocol just moves when it happens.

Two other things surfaced while digging, both independent of this PR:

  • MarkdownView's diagram effect is keyed on [renderedHTML, theme], but the DOM is written from displayHTML. A search highlight changes only displayHTML, so the innerHTML gets replaced — wiping the rendered SVG — without the effect ever re-running.
  • setActiveTab() writes to the store even when the clicked tab is already the active one, re-rendering subscribers for no reason.

Rather than patch the claim protocol further, I'd like to try removing the need for it: render Mermaid and Vega into the HTML string inside the useMarkdown pipeline, the way rehypeKatex already does. Then the DOM has a single writer and nothing mutates it after React commits — the claim attributes, the cancellation plumbing and the revert-then-rerender path all go away. Theme changes would re-run the pipeline instead.

I'll close this or replace it with that once I have it verified in a real build. Sorry for the noise.

@petrovo-as

Copy link
Copy Markdown
Author

Closing in favour of #5, which removes the need for the claim protocol rather than patching it. The race described above goes away with it, along with the cancellation plumbing and the revert-then-rerender path.

@petrovo-as petrovo-as closed this Jul 16, 2026
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