Skip to content

OneNote: Improve reliability and enterprise account support#528

Open
gurusince88 wants to merge 1 commit into
obsidianmd:masterfrom
gurusince88:fix/onenote-reliability-improvements
Open

OneNote: Improve reliability and enterprise account support#528
gurusince88 wants to merge 1 commit into
obsidianmd:masterfrom
gurusince88:fix/onenote-reliability-improvements

Conversation

@gurusince88
Copy link
Copy Markdown

@gurusince88 gurusince88 commented Apr 8, 2026

Summary

This PR improves the reliability of the OneNote importer, particularly for enterprise M365/SharePoint accounts.

Addresses: #440, #398, #390, #334, #313

Changes

Test plan

  • Fresh import with personal Microsoft account
  • Re-import with existing files (should skip correctly)
  • Re-import after deleting some files (should re-import deleted)
  • Test with M365 enterprise account (if available)
  • Verify mobile portrait shows sign-in button

Breaking changes

Notes imported after this change will have onenote-id in their frontmatter. The old data.json tracking is no longer used, so users who previously imported notes will need to either:

  1. Re-import with "Skip previously imported" disabled, or
  2. Their old notes won't be recognized as previously imported

Addresses issues obsidianmd#440, obsidianmd#398, obsidianmd#390, obsidianmd#334, and obsidianmd#313.

Changes:
- Add notes.read.all OAuth scope for M365/SharePoint enterprise accounts
- Improve error messages to show specific details (401/403/429/504)
- Replace fixed 60s retry with exponential backoff and jitter
- Switch from data.json to frontmatter-based import tracking (onenote-id)
- Fix mobile portrait UI: ensure sign-in button is visible

The frontmatter approach (similar to Notion API importer) fixes the issue
where "Skip previously imported" would incorrectly skip pages that had
been deleted from the vault.

Made-with: Cursor
@Lenivvenil
Copy link
Copy Markdown

Thanks for tackling this — the error message improvements and exponential backoff are solid. A few things worth addressing before merge:


🔴 Performance: O(n×m) file reads in findExistingFileByOnenoteId

const files = this.vault.getMarkdownFiles().filter(f => f.path.startsWith(outputFolder.path));
for (const file of files) {
    const content = await this.vault.read(file); // full disk read per file, per page

This reads every file in the output folder for every page being imported. With 500 OneNote pages and 500 existing files, that's 250,000 sequential disk reads. CONTRIBUTING.md calls this out explicitly: "Be performance minded. Your code will be used in vaults with 10,000 or even 100,000 notes."

The fix is to build an in-memory Map<onenoteId, filePath> once before the import loop by scanning existing files, then do O(1) lookups per page:

// Before the import loop:
const existingIds = new Map<string, string>();
for (const file of this.vault.getMarkdownFiles()) {
    if (!file.path.startsWith(outputFolder.path)) continue;
    const content = await this.vault.read(file);
    const match = content.match(/^onenote-id:\s*["']?([^"'\n]+)["']?$/m);
    if (match) existingIds.set(match[1].trim(), file.path);
}

// Inside the loop:
if (!this.importPreviouslyImported && page.id && existingIds.has(page.id)) {
    progress.reportSkipped(page.title!, 'previously imported');
    continue;
}

🔴 Rate limit retries are now bounded — regression from current behaviour

Original code deliberately did not increment retryCount on 429:

// don't increment the retryCount because we were told to backoff
retryCount  // <-- original

This PR changes it to retryCount + 1, which means after MAX_RETRY_ATTEMPTS (5) rate-limit responses the import hard-fails. Users with large notebooks who hit Microsoft's throttling — the exact users this PR aims to help — will now get failures instead of eventual success.

Suggest keeping 429 retries unbounded (don't increment retryCount) while still applying exponential backoff. The 90-minute stall watchdog already provides an upper bound.


🔴 shouldSkipExistingFile is dead code

The method is defined (~line 1107) but never called — import() calls findExistingFileByOnenoteId instead. Either remove it or wire it in.


🟡 notes.read.all requested for all users

Personal account users don't need notes.read.all and will see a more aggressive permission consent dialog with no benefit. Consider showing a targeted message on 40004 error — "If you're using a work/school account, you may need your admin to grant additional permissions" — rather than requesting the scope upfront for everyone.


🟡 YAML quoting on onenote-id

OneNote page IDs contain ! and other characters that YAML parsers may interpret:

// Current — may break YAML parsers:
const frontmatter = `---\nonenote-id: ${page.id}\n---\n\n`;

// Safer:
const frontmatter = `---\nonenote-id: "${page.id}"\n---\n\n`;

The regex match should also handle quoted values: /^onenote-id:\s*["']?([^"'\n]+)["']?$/m


🟡 Migration: existing users will silently re-import everything

Switching from data.json ID tracking to frontmatter means all users who previously imported will lose their tracking state on upgrade and re-import every page. Worth calling out in the PR description with a clear migration note, and potentially adding a one-time migration step that reads old previouslyImportedIDs from data.json and uses them as a fallback when no frontmatter is found.


The error branching in showContentAreaErrorMessage, the backoff for non-rate-limit errors, and the mobile UI fix all look good. Happy to discuss any of the above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants