wiseguru_ReWrite-Voice-Notes/docs/DEVCONFLICTS.md
WiseGuru 42f0a43c62 Resolve the 1.2.1 review-bot no-unsafe-* warnings at the root (type parity)
All ~30 warnings traced to the bot's lint environment resolving types
differently from local, not to unsafe code (DEVCONFLICTS.md finding 10):

- tsconfig lib was ES2016 while the code uses ES2017-ES2019 APIs
  (Object.entries/values/fromEntries, padStart, Promise.finally); the
  test suite's auto-included @types/node masked it locally. lib is now
  ["DOM", "ES2019"] with types: [] so a mismatch fails npm run build.
- moment's typings don't resolve in the bot's environment (obsidian's
  re-export types via the moment package). All calls now go through
  formatMoment in src/time.ts, a narrow structural alias.
- secrets.ts needed as-BufferSource assertions on TS 5.7+ that older TS
  flags as unnecessary; restructured (inferred ArrayBuffer-backed byte
  helpers, BufferSource param, one 32-byte copy) to need none on any
  TS version. Crypto behavior unchanged.
- The two flagged as-Record casts became an isRecord type predicate.

Docs: CLAUDE.md "Review-bot type environment" gotcha, RELEASING.md
parity section (plus the pre-release docs from the workflow change),
DEVCONFLICTS.md finding 10, ROADMAP.md Unreleased entry.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2026-07-09 20:55:52 -07:00

119 lines
13 KiB
Markdown
Raw Blame History

This file contains ambiguous Unicode characters

This file contains Unicode characters that might be confused with other characters. If you think that this is intentional, you can safely ignore this warning. Use the Escape button to reveal them.

# Developer-guideline conflicts and potential problems
A review of the ReWrite plugin against Obsidian's official developer documentation:
- Plugin guidelines: https://docs.obsidian.md/Plugins/Releasing/Plugin+guidelines
- Submission requirements: https://docs.obsidian.md/Plugins/Releasing/Submission+requirements+for+plugins
- Developer policies: https://docs.obsidian.md/Developer+policies
Scope: this document identifies conflicts and potential problems. Severity labels are this reviewer's estimate of how likely each item is to matter during community-plugin review. Each finding now carries a **Resolution** line recording how it was handled (fixed, or accepted as correct/intentional with rationale).
---
## High: likely to be flagged in submission review
### 1. LICENSE file still attributes copyright to the sample plugin's author
[LICENSE](../LICENSE) reads `Copyright (C) 2020-2025 by Dynalist Inc.` — this is the verbatim license shipped with `obsidian-sample-plugin`, never updated to the actual author.
- Developer policy: plugins must "Include a LICENSE file and clearly indicate the license" and respect copyright / credit incorporated code.
- The license body (0BSD-style permissive grant) is fine, but the copyright holder is wrong, which is a copyright-attribution problem, not just cosmetic.
> **Resolution: Fixed.** [LICENSE](../LICENSE) now reads `Copyright (C) 2026 WiseGuru`; the 0BSD grant body is kept verbatim (license choice confirmed: 0BSD's only trade-off vs MIT is no mandatory attribution, which is acceptable here).
### 2. Node.js APIs used while `isDesktopOnly: false`
[manifest.json](../manifest.json) sets `isDesktopOnly: false`, but the plugin imports Node built-ins (`child_process`, `net`, `fs`, `process`) in [src/whisper-host.ts](../src/whisper-host.ts).
- Submission requirement: "If using Node.js packages like `fs`, `crypto`, or `os`, the plugin must be marked as desktop-only."
- The plugin does this deliberately and defensibly: the Node modules are lazy-`require`d only inside `Platform.isDesktop` guards (the local whisper.cpp host is a desktop-only feature), so mobile never touches them. This is the accepted pattern for a mixed desktop/mobile plugin.
- Still flag-worthy: automated review and human reviewers frequently catch Node imports in non-desktop-only manifests. Expect to have to justify it. The casts in `getNodeApi()` ([src/whisper-host.ts](../src/whisper-host.ts), `window.require` / `window.process`) are part of the same pattern.
> **Resolution: Accepted as-is.** `isDesktopOnly` stays `false` so mobile keeps the cloud-provider features; Node modules are lazy-`require`d only inside `Platform.isDesktop` guards (the local whisper.cpp host), so mobile never loads them. The README already discloses the desktop-only host. This is the documented pattern for a mixed desktop/mobile plugin; be ready to explain it in review.
### 3. `package.json` license identifier is not a valid SPDX id
[package.json](../package.json) line 14 declares `"license": "0-BSD"`. The SPDX identifier is `0BSD` (no hyphen). Tooling that validates SPDX will not recognize `0-BSD`.
> **Resolution: Fixed.** [package.json](../package.json) now declares `"license": "0BSD"`.
---
## Medium: code-guideline deviations
### 4. `Vault.modify()` used instead of `Vault.process()` for read-modify-write
Guideline: "Use `Vault.process` instead of `Vault.modify`" for background edits (atomic), and "Use the Editor interface instead of `Vault.modify()` to the active file."
- [src/insert.ts:59-62](../src/insert.ts#L59-L62) — `insertAppend` reads the file then `vault.modify`s it. This is a non-atomic read-modify-write, and the target may be the **active** file (it falls back to the active view's file), where the guideline prefers the Editor interface to preserve cursor/selection.
- [src/templates-folder.ts:255](../src/templates-folder.ts#L255) — `vault.modify` during the template-update reconcile.
- [src/template-guide.ts:249](../src/template-guide.ts#L249) — `vault.modify` when rewriting the guide.
The latter two are background, non-active-file writes; `Vault.process` would be the recommended atomic form.
> **Resolution: Fixed.** All three sites now use `Vault.process(file, (data) => ...)`: [insert.ts](../src/insert.ts) `insertAppend` computes the separator from the callback's `data` (and no longer does a separate `vault.read`), [templates-folder.ts](../src/templates-folder.ts) uses `process(child, () => rendered)`, and [template-guide.ts](../src/template-guide.ts) uses `process(existing, () => content)`. Behavior is unchanged.
### 5. `app.vault.adapter` used instead of `app.vault`
Guideline: prefer `app.vault` over `app.vault.adapter` (caching + serialized, safer operations).
- [src/secrets.ts:296-314](../src/secrets.ts#L296-L314) — `adapter.exists/read/write` for `secrets.json.nosync`.
- [src/whisper-host.ts:397-427](../src/whisper-host.ts#L397-L427) — `adapter.exists/read/write/remove` for the PID sidecar.
Both touch config/sidecar files that are intentionally not regular vault notes (e.g. the `.nosync` secrets envelope), so direct adapter access is arguably justified — but it is still a documented deviation a reviewer may question.
> **Resolution: Accepted as correct (reclassified).** Both paths resolve under `plugin.manifest.dir` (`.obsidian/plugins/rewrite-voice-notes/`), which is the plugin config directory, not vault note content. The `app.vault` TFile API does not address files there; `app.vault.adapter` is the appropriate API for plugin-config files. No change.
### 6. Use of undocumented / private Obsidian internals
The guidelines steer away from relying on internals not in the public API (they can change without notice). The plugin reaches several via `as unknown as` casts:
- [src/ui/quick-record.ts:189](../src/ui/quick-record.ts#L189) — `app.hotkeyManager` (not in public typings).
- [src/ui/modal.ts:507](../src/ui/modal.ts#L507) — cast on `this.app` to reach an internal.
- [src/secrets.ts:111](../src/secrets.ts#L111) — `app.secretStorage`. This one is now a GA public API (1.11.4) but predates the installed typings, so it is reached via a cast; lower risk than the others but worth listing.
These are localized behind narrow interfaces (the documented pattern for when a cast is unavoidable), but they remain private-API dependencies that can break on an Obsidian update.
> **Resolution: Accepted as-is.** Each access is already isolated behind a narrow local interface + cast, which is the recommended mitigation when a needed API is absent from the public typings. `secretStorage` is a GA public API the bundled typings simply predate. Left unchanged; revisit if a future typings update makes any of these first-class.
### 7. Editor-dependent commands use `callback` instead of `editorCheckCallback`
Guideline: "Use `editorCallback` or `editorCheckCallback` for commands requiring an active Markdown editor."
- [src/main.ts:80-86](../src/main.ts#L80-L86) — `process-text` requires an active Markdown editor/selection but is registered with a plain `callback` and checks for an editor internally (showing a Notice when absent). Using `editorCheckCallback` would let Obsidian hide the command when no editor is active, which is the recommended behavior. (The whisper start/stop commands already use `checkCallback` correctly.)
> **Resolution: Accepted as intentional (reclassified).** `processTextWithTemplate` does not strictly require an editor: when none is active it shows a guiding Notice ("Open a Markdown note or select text"). Switching to `editorCheckCallback` would hide the command entirely in that state and remove that fallback. Kept as `callback` by design.
---
## Low: minor / conventional
### 8. `manifest.json` missing `author` / `authorUrl`
[manifest.json](../manifest.json) has no `author` or `authorUrl` fields. Not strictly required, but conventional and present in the sample manifest; pairs with finding #1 (the stale LICENSE copyright).
> **Resolution: Fixed.** [manifest.json](../manifest.json) now sets `author` (`WiseGuru`), `authorUrl` (`https://github.com/WiseGuru`), and `fundingUrl` (`https://wyz.guru/buy-someone-else-coffee/`).
### 9. `app.vault.getFiles()` full-vault scan
[src/ui/audio-source.ts:18](../src/ui/audio-source.ts#L18) iterates all vault files to filter audio by extension. The guideline's "don't iterate all files" advice is specifically about *finding a file by path* (use `getFileByPath`), which this is not — there is no extension index in the API, so a scan is reasonable. Listed only for completeness; not a real conflict.
### 10. 1.2.1 automated-review `no-unsafe-*` warnings (type-environment mismatch)
The 1.2.1 submission drew ~30 warnings (`no-unsafe-assignment/call/member-access/argument/return`, `no-unnecessary-type-assertion`) that local `npm run lint` did not reproduce even after the rule mirror ([eslint.config.mts](../eslint.config.mts)) was in place. Investigation showed none of the flagged code was genuinely unsafe; every warning came from the bot's **type environment** differing from local. The bot's typed lint uses the repo's own `tsconfig.json` but not the repo's full `node_modules`, so a value that types cleanly locally can be error-typed there (and an error-typed value trips `no-unsafe-*` at every use). Three sub-causes:
1. **Declared `lib` was lower than the APIs the code uses.** `tsconfig.json` declared `lib` ES2016 while the source uses `Object.entries`/`values`/`fromEntries`, `String.padStart`, and `Promise.finally` (ES2017ES2019). Locally, tsc auto-included the test suite's `@types/node`, which silently supplied those types; in the bot's environment they were error-typed (~20 of the warnings).
2. **`moment`'s types don't resolve in the bot's environment.** `obsidian` itself types fine there, but its `moment` re-export's type comes from the `moment` package (a transitive dependency), which the bot doesn't have — every direct `moment(...)` call was error-typed (5 warnings across [src/audio-persist.ts](../src/audio-persist.ts), [src/insert.ts](../src/insert.ts), [src/template-guide.ts](../src/template-guide.ts)).
3. **TS-version-dependent assertions.** On TS 5.7+ (local, 5.8) a value declared `Uint8Array` is `ArrayBufferLike`-backed and needs `as BufferSource` to satisfy WebCrypto; on the bot's older TS the same assertion is "unnecessary" ([src/secrets.ts](../src/secrets.ts)).
> **Resolution: Fixed (all three).** `tsconfig.json` now declares `lib: ["DOM", "ES2019"]` with `types: []` (so the `@types/node` masking can never recur — a lib/API mismatch now fails `npm run build` locally) and `skipLibCheck`. All `moment` calls go through `formatMoment` in [src/time.ts](../src/time.ts), a narrow structural alias over Obsidian's bundled moment (same pattern as `ScriptProcessorNodeLike`). The secrets byte helpers were restructured to need no `BufferSource` assertion under any TS version (inferred `ArrayBuffer`-backed return types, a `BufferSource` param, one 32-byte defensive copy of the hash-wasm output), and the two `as Record<string, unknown>` narrows the bot flagged were replaced with an `isRecord` type predicate.
---
## Checked and clean (no conflict found)
For the record, these commonly-flagged items were checked and are compliant:
- **DOM safety**: no `innerHTML` / `outerHTML` / `insertAdjacentHTML` anywhere; DOM is built with `createEl`/`createDiv`/`createSpan`.
- **No hardcoded styling in JS**: no `el.style` / inline-style assignments; all styling lives in [styles.css](../styles.css) and uses Obsidian CSS variables (`--text-muted`, `--background-modifier-border`, etc.).
- **App instance**: uses `this.app` throughout; no `window.app` / global `app`.
- **No `var`**; `const`/`let` only. async/await used over raw Promises.
- **No default hotkeys** set on any command.
- **`normalizePath`** is used consistently for user-supplied paths.
- **Resource cleanup**: `registerEvent` / `registerInterval` used; the one manual DOM element (Quick Record floater) is torn down in `onunload`.
- **Settings headings**: routed through a `setHeading()` helper, no manual `<h2>`, no top-level "Settings"/plugin-name heading.
- **Sentence case** in UI text and command names; the codebase carries no `eslint-disable` directives (the former sentence-case exemption for a random-string example in [src/ui/passphrase-modal.ts](../src/ui/passphrase-modal.ts) was replaced by passing the string through a variable, which the rule does not inspect).
- **Manifest description**: action-focused, ends with a period, under 250 characters, no emoji.
- **Network-use disclosure** (developer policy): the README states keys are user-supplied, "Nothing is sent to a ReWrite server," and lists every provider endpoint.
- **No telemetry, no ads, no self-update mechanism, no code obfuscation** (developer policy prohibitions).
- **Frontmatter** modified via `FileManager.processFrontMatter` ([src/insert.ts:94](../src/insert.ts#L94)), not manual YAML editing of the note.