Skip to content

Fix Obsidian deep links and DeepLore drawer icon#30

Open
DJLegends1011 wants to merge 1 commit into
pixelnull:mainfrom
DJLegends1011:claude/wizardly-mclean-5036bd
Open

Fix Obsidian deep links and DeepLore drawer icon#30
DJLegends1011 wants to merge 1 commit into
pixelnull:mainfrom
DJLegends1011:claude/wizardly-mclean-5036bd

Conversation

@DJLegends1011
Copy link
Copy Markdown

@DJLegends1011 DJLegends1011 commented May 8, 2026

Edit:

Summary

  • Fixes Obsidian deep links so clicking Open in Obsidian no longer navigates the SillyTavern browser tab away from the UI.
  • Launches obsidian:// URIs through a hidden iframe instead of direct anchor navigation.
  • Applies the same handling across drawer browse links, browse popup links, and graph entry links.
  • Adds regression coverage for Obsidian link rendering, escaping, iframe launch behavior, and invalid URI handling.

Notes

This is scoped to the long-standing direct obsidian:// anchor issue. The earlier drawer-icon guard was intentionally left out after testing showed the Obsidian navigation was the confirmed repro path.

Test plan

  • npm run test:regression
  • npm run test:imports
  • npm run test:all

🤖 found the issue by the help of codex, made pr with the help of claude.

@Ryah
Copy link
Copy Markdown

Ryah commented May 8, 2026

Found a slight bug in this PR when I tested this that should probably be noted. If your Obsidian vault is not the same name as the name in DeepLore, it fails to open the entry.

Both the name in Obsidian and DeepLore need to match for the entry to open.

@DJLegends1011
Copy link
Copy Markdown
Author

Found a slight bug in this PR when I tested this that should probably be noted. If your Obsidian vault is not the same name as the name in DeepLore, it fails to open the entry.

Both the name in Obsidian and DeepLore need to match for the entry to open.

Could've swore that was a vanilla deep lore bug🤣.

Since if you don't name it in the vanilla ext nsion correctly it will not open.

My PR tested it WITH that fixes in the settings

@pixelnull
Copy link
Copy Markdown
Owner

Heello @DJLegends1011 :D

I did a static pass on this before reproducing live, and the Obsidian-link half of the PR holds up well. Confirmed four spots in main that render <a href="obsidian://..."> with no target and no click handler (src/drawer/drawer-events.js:591, src/drawer/drawer-render-tabs.js:124 and :716, src/ui/popups.js:317), which matches the blank-tab symptom... modern Chromium dispatches the protocol to the OS but the implicit navigation on the anchor still kicks the SPA. The iframe-based openExternalProtocol in your src/helpers.js is the right pattern, and consolidating four call sites onto one helper is a nice cleanup. Regression tests look solid too.

One small framing nitpick: I don't think this is actually a regression introduced by v2.0.2. That commit (7169a67) only touched cmrsResultToText inside src/helpers.js. It didn't go near any Obsidian-link rendering. The anchor pattern you're replacing has been there since well before the CMRS fix landed. Not a blocker, just want to be accurate in the changelog when this merges. I'll call it a long-standing bug rather than a regression. (Possible the symptom became more visible recently because of browser behavior changes around custom protocols, but that's separate from anything DLE did.)

Two things I want to clear up before merging though, both about the drawer-icon half:

  1. Your description claims the drawer icon "no longer disappears on re-render," but the final diff doesn't actually contain a drawer-icon fix. Looking at the commit history: you added mountDrawerInHolder + 67 lines of MutationObserver mount-guard in 64fd331, then reverted it in 74c331e. Final file list has no src/drawer/drawer.js or src/drawer/drawer-dom.js change, and the corresponding regression tests are reverted too. So as it stands, this PR ships only the Obsidian-link fix.

  2. Why the revert? Was the mount-guard causing a different problem in your testing (MutationObserver firing too aggressively, HMR collision, false positives on theme switches), or did you just want to ship one thing at a time?

If you want both fixes in this PR: re-cherry-pick the mount-guard commit and tell me what made you pull it back, so I can spot the same thing in review. If you'd rather keep this PR focused on Obsidian links: update the description to drop the drawer-icon claim, and open the mount-guard as its own PR. Your diagnosis (async fetch('/icon.svg') resolving against a $drawer that may have detached) is real and worth fixing, just not in this PR.

Either path is fine, your call. Once that's sorted I'll merge.

@DJLegends1011
Copy link
Copy Markdown
Author

Heello @DJLegends1011 :D

I did a static pass on this before reproducing live, and the Obsidian-link half of the PR holds up well. Confirmed four spots in main that render <a href="obsidian://..."> with no target and no click handler (src/drawer/drawer-events.js:591, src/drawer/drawer-render-tabs.js:124 and :716, src/ui/popups.js:317), which matches the blank-tab symptom... modern Chromium dispatches the protocol to the OS but the implicit navigation on the anchor still kicks the SPA. The iframe-based openExternalProtocol in your src/helpers.js is the right pattern, and consolidating four call sites onto one helper is a nice cleanup. Regression tests look solid too.

One small framing nitpick: I don't think this is actually a regression introduced by v2.0.2. That commit (7169a67) only touched cmrsResultToText inside src/helpers.js. It didn't go near any Obsidian-link rendering. The anchor pattern you're replacing has been there since well before the CMRS fix landed. Not a blocker, just want to be accurate in the changelog when this merges. I'll call it a long-standing bug rather than a regression. (Possible the symptom became more visible recently because of browser behavior changes around custom protocols, but that's separate from anything DLE did.)

Two things I want to clear up before merging though, both about the drawer-icon half:

1. Your description claims the drawer icon "no longer disappears on re-render," but the final diff doesn't actually contain a drawer-icon fix. Looking at the commit history: you added `mountDrawerInHolder` + 67 lines of `MutationObserver` mount-guard in `64fd331`, then reverted it in `74c331e`. Final file list has no `src/drawer/drawer.js` or `src/drawer/drawer-dom.js` change, and the corresponding regression tests are reverted too. So as it stands, this PR ships only the Obsidian-link fix.

2. Why the revert? Was the mount-guard causing a different problem in your testing (MutationObserver firing too aggressively, HMR collision, false positives on theme switches), or did you just want to ship one thing at a time?

If you want both fixes in this PR: re-cherry-pick the mount-guard commit and tell me what made you pull it back, so I can spot the same thing in review. If you'd rather keep this PR focused on Obsidian links: update the description to drop the drawer-icon claim, and open the mount-guard as its own PR. Your diagnosis (async fetch('/icon.svg') resolving against a $drawer that may have detached) is real and worth fixing, just not in this PR.

Either path is fine, your call. Once that's sorted I'll merge.

IGHT, keeping the PR focused on Obsidian links is the right call Pixel.

The drawer-icon guard was reverted because after testing, the Obsidian direct-link navigation turned out to be the real cause of the UI disappearing in this repro. I didn’t want to ship the MutationObserver mount guard as part of this PR without a separate confirmed drawer-icon-only repro.

I’ll first update the PR description to remove the drawer-icon claim and frame this as a long-standing direct obsidian:// anchor issue rather than a v2.0.2 regression. The drawer icon issue can be tracked separately if we can reproduce it independent of Obsidian link navigation.

@DJLegends1011 DJLegends1011 force-pushed the claude/wizardly-mclean-5036bd branch from 85b1e45 to 0cd9b27 Compare May 15, 2026 00:03
@DJLegends1011
Copy link
Copy Markdown
Author

commit has been dropped!

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.

3 participants