fix: resolve Obsidian plugin-review findings (policy, guidelines, lockfile)#219
Merged
Conversation
The "Reload PodNotes" command disabled and re-enabled the plugin via app.plugins.disablePlugin/enablePlugin. Programmatically disabling and re-enabling a plugin is disallowed by Obsidian developer policy (it can run newly downloaded code without user awareness), so the command and its supporting global.d.ts type declarations are removed. Also rename the "Show PodNotes" command to "Show player" (Obsidian already shows the plugin name beside the command) and stop leaving the getUniversalPodcastLink promise unhandled.
The vendored lucide list was duplicated, producing ~580 "union type constituent is duplicated" warnings. Derive both ICON_LIST (a deduplicated `as const` array) and the IconType union from a single source of truth so no name repeats; the full set of 602 icon names is preserved.
Replace the deprecated implicit global `app` with the plugin instance's own app reference. Feature modules that already depend on the plugin store (createPodcastNote, createFeedNote, URIHandler) read `get(plugin).app`; the leaf utilities take the dependency explicitly — createMediaUrlObjectFromFilePath now receives a Vault, and ensureFolderExists defaults its vault to the plugin app's. getContextMenuHandler passes its own app through. Tests inject the app via the plugin store / parameters instead of a global.
Replace manually created HTML heading elements with Setting().setName().setHeading(), direct element.style writes with setCssStyles(), and `document` with `activeDocument` for popout-window compatibility. Settle the fire-and-forget saveSettings()/exportOPML/render calls with `void`, and widen pickFile's callback to accept an async handler (no misused promise). Adds jsdom polyfills for setCssStyles and activeWindow/activeDocument so component tests exercise the same paths.
Switch the extension-probe HEAD request from `fetch` to Obsidian's `requestUrl` (the sanctioned network primitive), reading the content-type header case-insensitively and keeping the mp3 fallback. Resolve the vault from the plugin app reference instead of the global `app`, use window.setTimeout, and set the download-notice styles via setCssStyles.
onunload no longer detaches the view's leaves: detaching there reset a leaf the user had moved back to its default location on the next load. To avoid the duplicate that a hot reload would otherwise leave (the restored leaf plus a freshly auto-opened one), collapse the single-instance player back to one leaf on layout-change (a no-op unless more than one exists, so it converges and preserves the restored leaf on a cold start). Also use window.* timers and window.navigator instead of globalThis, and void the modal's unmount().
Prefix setTimeout/clearTimeout/setInterval/clearInterval with window. so timers resolve against the correct (possibly popped-out) window.
Prefer App#loadLocalStorage/App#saveLocalStorage (namespaced per vault by Obsidian) over the global localStorage, so one vault's feed cache can't leak into another, with a window.localStorage fallback for non-plugin contexts. Legacy un-prefixed v1-v4 blobs are purged from the raw localStorage on load.
Add an actions/attest-build-provenance step (with id-token/attestations write permissions) so main.js and manifest.json carry verifiable build provenance.
…delines Swap the unmaintained eslint-plugin-import for eslint-plugin-import-x (pinned to 4.16.1 for @typescript-eslint/utils peer compatibility), per the Obsidian review's module-replacement recommendation. Wire eslint-plugin-obsidianmd's guideline rules (no global app/fetch/localStorage/globalThis, window timers, setCssStyles, setHeading, no onunload leaf detach, active document, no plugin name in command name) into the lint config as a regression guard for the fixes in this PR. Remove the stale bun.lock so the repo (and the plugin-review build verification) installs via npm against the in-sync package-lock.json.
Deploying podnotes with
|
| Latest commit: |
534ed93
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://ef337203.podnotes.pages.dev |
| Branch Preview URL: | https://chhoumann-fix-plugin-review.podnotes.pages.dev |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 890f147530
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+42
to
+43
| "eslint-plugin-import-x": "4.16.1", | ||
| "eslint-plugin-obsidianmd": "^0.3.0", |
There was a problem hiding this comment.
These new dev dependencies are required by eslint.config.mjs, but package-lock.json was not updated and still has neither the root devDependency entries nor node_modules/eslint-plugin-import-x / node_modules/eslint-plugin-obsidianmd. Any clean lockfile-based install, such as npm ci, will not be able to reproduce this dependency graph and linting will fail because the config imports packages that are absent from the committed lockfile. Please regenerate and commit package-lock.json with these dependency changes.
Useful? React with 👍 / 👎.
The removed "Reload PodNotes" (hrpn) command was the readiness sentinel for plugin.reload(); without it every E2E reload would poll a command that never appears and time out (30s). Point it at the always-registered "podnotes-show-leaf" command instead.
Add coverage for the production paths the review flagged as untested: the vault-scoped App#load/saveLocalStorage adapter (previously every test fell back to window.localStorage), and dedupePlayerLeaves' convergence invariant (keep the first leaf, detach the rest, no-op at <= 1 so a cold-restart leaf is preserved).
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
|
🎉 This PR is included in version 2.17.1 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Resolves the actionable findings from the Obsidian community plugin-review dashboard. Each finding was validated against the current code (the review was captured at
76e830f, before the recent download-streaming and architecture refactors landed), then fixed with the implementation that best fits the codebase — not a mechanical rule-silencing pass. Behavior is preserved; the changes are validated by the full gate suite (lint, typecheck, format, build, 754 unit tests) and by live runtime checks in an isolated Obsidian vault.To keep these fixes from regressing, the relevant
eslint-plugin-obsidianmdguideline rules are wired into the lint config (the project lint previously didn't enforce them — the dashboard ran its own ruleset).Per-finding breakdown
Policy errors (fixed)
app.plugins.disablePlugin/enablePlugin) and itsglobal.d.tstype declarations.bun.lock; the repo is npm-only (AGENTS.md), so the review's build verification now installs via npm against an in-syncpackage-lock.json.package-lock.json).Guideline warnings (fixed)
appobject → use the plugin's app reference: feature modules readget(plugin).app; leaf utilities take an explicitVault/App(e.g.createMediaUrlObjectFromFilePath(vault, …),ensureFolderExistsdefault).void-ed fire-and-forget calls;pickFile's callback now acceptsvoid | Promise<void>.el.setCssStyles({…})in the settings tab and download notice.setTimeout/clearTimeout/setInterval/clearInterval→window.*for popout-window compatibility.fetch→ ObsidianrequestUrl(the extension-probe HEAD request).globalThis→window(media-session lookup).onunload→ removed; the single-instance player view is instead de-duplicated onlayout-change, which preserves a leaf the user moved (verified: a cold-restart leaf is kept, a hot-reload duplicate collapses back to one).localStorage→ vault-scopedApp#loadLocalStorage/saveLocalStorage(with awindow.localStoragefallback and legacy-key purge).new Setting(el).setName(…).setHeading().document→activeDocument.eslint-plugin-import→eslint-plugin-import-x(pinned 4.16.1 for peer compatibility).IconNameunion (×580) →IconTypeandICON_LISTnow derive from one deduplicated source array (all 602 names preserved).Recommendations
actions/attest-build-provenancestep (withid-token/attestationswrite permissions) formain.js+manifest.jsonin the release workflow.Rejected / deferred (with rationale)
'error' type that acts as 'any'(rejected — not reproducible) — does not fire on the current code; the landed refactor already narrowed those catch clauses tounknown.ws, is tree-shaken out ofmain.js). Resolving them requires major build-toolchain upgrades that are out of scope for a review-remediation PR and risk destabilizing the toolchain; the shipped artifact is unaffected.Testing
npm run lint,npm run typecheck,npm run format:check,npm run build,npm run test(754 passing) — all green.npm run docs:buildwas not run locally (MkDocs isn't installed in this environment); the docs change is a heading rename + section removal incommands.md.Do not merge — opening for maintainer review.