Skip to content

Make the Obsidian plugin loadable on mobile - #1391

Draft
trangdoan982 wants to merge 3 commits into
mainfrom
eng-mobile-support
Draft

Make the Obsidian plugin loadable on mobile#1391
trangdoan982 wants to merge 3 commits into
mainfrom
eng-mobile-support

Conversation

@trangdoan982

Copy link
Copy Markdown
Member

Reviewer brief

  • Result: the Obsidian plugin bundle no longer contains any Node or Electron code, and ships with isDesktopOnly: false. This is the load-blocker MVP only — it makes the plugin start on mobile so a designer can evaluate it via BRAT. Touch affordances are explicitly not in scope.

  • Review focus: two behavior swaps that also affect desktop users.

    • splitFrontmatter + parseYaml replaces gray-matter. Semantics are matched by unit test, with one deliberate difference: gray-matter threw on malformed YAML, this returns empty frontmatter instead. That is closer to how Obsidian itself tolerates bad YAML, but it does mean a bad import file now yields a node with no frontmatter rather than a hard failure.
    • The markdown-link regex now captures a leading ! instead of using a lookbehind. Worth confirming the image-vs-link split still reads correctly at importNodes.ts.
  • Risk or follow-up: verified statically and by unit test, not on a physical device. The touch gaps below are known and unaddressed, so the plugin is usable-but-incomplete on mobile:

    • The "Create <NodeType>" button lives inside a hover-only tooltip (tagNodeHandler.ts) — unreachable on touch.
    • The canvas context menu (TldrawViewComponent.tsx) is driven by tldraw's right_click; long-press does not produce it, so "Convert to discourse node" is unreachable on canvas.
    • Wikilink drag-to-canvas (wikilinkDragHandler.ts) uses HTML5 drag events, which do not fire on touch.
    • The single-character node-tag hotkey has no non-keyboard trigger.

    The 17 addCommand registrations all surface in the mobile command palette, which is the reliable entry point in the meantime.

The most durable change here is dropping Node builtins and electron from the esbuild external array. Marking them external is what let three require() calls survive into the shipped bundle in the first place; without it esbuild fails the build instead. This is recorded in apps/obsidian/AGENTS.md so it is not quietly reverted.

Verification

  • Production bundle contains require() for only obsidian and @codemirror/view — previously also fs (gray-matter), path (mime-types), and buffer (js-yaml). 2.2MB.
  • Zero regex lookbehind assertions in the bundle. The one remaining (?< is a core-js feature probe using a named capture group.
  • Remaining process.env / Buffer references are all guarded — tldraw wraps its env reads in try/catch, and both Buffer uses are behind typeof Buffer !== "undefined".
  • pnpm install --frozen-lockfile then pnpm ci:validate from the repo root: pass, including the 15 new Obsidian unit tests.

apps/obsidian had no test runner; this adds vitest mirroring the Roam app's setup and exposes it through test:unit so root validation picks it up.

Loom video

Scope check

  • Ran $scope-check against the ENG ticket and final diff.
  • Scope beyond Done When: no ENG ticket — this branch came from a direct request to unblock a mobile BRAT build for design evaluation.

Local delegated full review

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

🤖 Generated with Claude Code

Removes every Node/Electron dependency from the bundle so the plugin can
run on Obsidian mobile, and flips `isDesktopOnly` to false.

The bundle previously emitted three bare Node requires (`fs` from
gray-matter, `path` from mime-types, `buffer` from js-yaml via
gray-matter), each of which throws at load time on mobile:

- Replace gray-matter with a pure `splitFrontmatter` helper plus
  Obsidian's own `parseYaml`.
- Replace mime-types with a local extension-to-MIME map.
- Delete `nativeJsonFileDialogs.ts`, which wrapped Electron's native
  file dialogs. It had no callers and was absent from the bundle, so
  this is not a behavior change.
- Rewrite the markdown-link regex to capture a leading `!` instead of
  using a lookbehind, which older mobile WebViews do not support.

Node builtins and `electron` are also dropped from the esbuild
`external` array. Marking them external is what allowed those requires
to survive into the bundle in the first place; without it, esbuild fails
the build instead of shipping something that breaks on device.

The node search modal stacks its two panes below the `sm` breakpoint
rather than claiming a fixed 900px width.

Touch affordances are deliberately not addressed here: the hover-only
node tag tooltip, wikilink drag-to-canvas, and the canvas right-click
context menu still have no touch equivalent.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@supabase

supabase Bot commented Sep 3, 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 ↗︎.

@vercel

vercel Bot commented Sep 3, 2026

Copy link
Copy Markdown

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

1 Skipped Deployment
Project Deployment Actions Updated
discourse-graph Skipped Skipped Sep 3, 2026 4:38pm UTC

Request Review

@graphite-app

graphite-app Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

PR size/scope check

This PR is over our review-size guideline.

  • Recommended: ~200 lines changed
  • Acceptable limit: up to 400 lines when well-scoped/self-contained
  • Preferred file count: fewer than 5 files

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:

  • What single problem this PR solves
  • Why the files/changes are coupled

Capturing the leading `!` pushed the replace callback to four
parameters, over the max-params limit. ESLint reports this as a warning,
which passes locally but fails the reviewdog lint check on added lines.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Comment on lines +1046 to +1047
} catch {
return { frontmatter: {}, body: content };

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When YAML parsing fails, the function returns body: content (the original full content), but this is inconsistent with the successful parse path. When splitFrontmatter succeeds but parseYaml throws, the frontmatter block markers (---) are already stripped from body, so returning the original content re-introduces them.

Impact: A file with malformed YAML frontmatter will have the raw frontmatter block (including --- delimiters) left in the body text, which will render as markdown content instead of being hidden.

Fix: Return the stripped body instead:

catch {
  return { frontmatter: {}, body };
}

This way, the malformed frontmatter block is removed from the body (matching the successful parse behavior), and users get empty frontmatter instead of a parsing error (the desired tolerance behavior mentioned in the PR description).

Suggested change
} catch {
return { frontmatter: {}, body: content };
} catch {
return { frontmatter: {}, body };

Spotted by Graphite

Fix in Graphite


Is this helpful? React 👍 or 👎 to let us know.

The publish script reads everything from the current working directory,
so running it from a different checkout silently ships that checkout's
manifest. With worktrees in play that is easy to do and there was no
signal until after the release existed.

Log the directory, branch, isDesktopOnly and minAppVersion before
anything is pushed, and fail when manifest.json and dist/manifest.json
disagree. The mirrored repo takes its manifest from dist/ while the
release assets take theirs from source, so a stale build would publish
two different manifests.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@trangdoan982
trangdoan982 marked this pull request as draft September 3, 2026 16:51
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