From 6e7d9e571c1b169eb12afe400bbb104c84b6e876 Mon Sep 17 00:00:00 2001 From: Alex Date: Sun, 3 May 2026 01:08:07 +0200 Subject: [PATCH] Review fixes --- .github/workflows/release.yml | 2 - src/cache.test.ts | 91 ++++++++++++++++++++--------------- src/cache.ts | 2 +- src/client.test.ts | 44 +++++++++-------- src/client.ts | 32 +++++++----- src/main.ts | 13 ++--- src/renderer.test.ts | 52 ++++++++++---------- src/svg.ts | 2 +- 8 files changed, 132 insertions(+), 106 deletions(-) diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 415d51a..e27dd9d 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -60,7 +60,6 @@ jobs: test -s main.js test -s manifest.json test -s styles.css - test -s versions.json - name: Create GitHub release env: @@ -70,6 +69,5 @@ jobs: main.js \ manifest.json \ styles.css \ - versions.json \ --title "$GITHUB_REF_NAME" \ --notes "Release $GITHUB_REF_NAME" diff --git a/src/cache.test.ts b/src/cache.test.ts index 30452de..f6ca8f0 100644 --- a/src/cache.test.ts +++ b/src/cache.test.ts @@ -3,10 +3,12 @@ import type { DataAdapter, ListedFiles, Stat } from 'obsidian' import { PumlerDiskSvgCache } from './cache' import type { RenderDiagramOptions } from './client' +const CACHE_DIR = 'custom-config/plugins/pumler/cache' + describe('PumlerDiskSvgCache', () => { test('stores and reads SVGs by render options', async () => { const adapter = new MemoryAdapter() - const cache = new PumlerDiskSvgCache(adapter, '.obsidian/plugins/pumler/cache', 30) + const cache = new PumlerDiskSvgCache(adapter, CACHE_DIR, 30) const options = renderOptions('Alice -> Bob') await cache.set(options, 'Alice') @@ -17,7 +19,7 @@ describe('PumlerDiskSvgCache', () => { test('keeps only the most recently used entries', async () => { const adapter = new MemoryAdapter() - const cache = new PumlerDiskSvgCache(adapter, '.obsidian/plugins/pumler/cache', 2) + const cache = new PumlerDiskSvgCache(adapter, CACHE_DIR, 2) const first = renderOptions('A -> B') const second = renderOptions('B -> C') const third = renderOptions('C -> D') @@ -35,10 +37,10 @@ describe('PumlerDiskSvgCache', () => { test('ignores corrupt cache indexes', async () => { const adapter = new MemoryAdapter() - await adapter.mkdir('.obsidian/plugins/pumler/cache') - await adapter.write('.obsidian/plugins/pumler/cache/index.json', 'not json') + await adapter.mkdir(CACHE_DIR) + await adapter.write(`${CACHE_DIR}/index.json`, 'not json') - const cache = new PumlerDiskSvgCache(adapter, '.obsidian/plugins/pumler/cache', 30) + const cache = new PumlerDiskSvgCache(adapter, CACHE_DIR, 30) const options = renderOptions('A -> B') await cache.set(options, 'fresh') @@ -64,113 +66,124 @@ class MemoryAdapter implements DataAdapter { return 'memory' } - async exists(normalizedPath: string): Promise { - return this.folders.has(normalizedPath) || this.files.has(normalizedPath) + exists(normalizedPath: string): Promise { + return Promise.resolve(this.hasPath(normalizedPath)) } - async stat(normalizedPath: string): Promise { - if (!await this.exists(normalizedPath)) { - return null + stat(normalizedPath: string): Promise { + if (!this.hasPath(normalizedPath)) { + return Promise.resolve(null) } - return { + return Promise.resolve({ type: this.folders.has(normalizedPath) ? 'folder' : 'file', ctime: 0, mtime: 0, size: this.files.get(normalizedPath)?.length ?? 0 - } + }) } - async list(normalizedPath: string): Promise { + list(normalizedPath: string): Promise { const prefix = `${normalizedPath}/` - return { + return Promise.resolve({ files: Array.from(this.files.keys()).filter(path => path.startsWith(prefix)), folders: Array.from(this.folders).filter(path => path.startsWith(prefix)) - } + }) } - async read(normalizedPath: string): Promise { + read(normalizedPath: string): Promise { const data = this.files.get(normalizedPath) if (data === undefined) { - throw new Error(`Missing file: ${normalizedPath}`) + return Promise.reject(new Error(`Missing file: ${normalizedPath}`)) } - return data + return Promise.resolve(data) } - async readBinary(): Promise { - throw new Error('Not implemented') + readBinary(): Promise { + return Promise.reject(new Error('Not implemented')) } - async write(normalizedPath: string, data: string): Promise { + write(normalizedPath: string, data: string): Promise { this.files.set(normalizedPath, data) + return Promise.resolve() } - async writeBinary(): Promise { - throw new Error('Not implemented') + writeBinary(): Promise { + return Promise.reject(new Error('Not implemented')) } - async append(normalizedPath: string, data: string): Promise { + append(normalizedPath: string, data: string): Promise { this.files.set(normalizedPath, `${this.files.get(normalizedPath) ?? ''}${data}`) + return Promise.resolve() } - async appendBinary(): Promise { - throw new Error('Not implemented') + appendBinary(): Promise { + return Promise.reject(new Error('Not implemented')) } - async process(normalizedPath: string, fn: (data: string) => string): Promise { + process(normalizedPath: string, fn: (data: string) => string): Promise { const data = fn(this.files.get(normalizedPath) ?? '') this.files.set(normalizedPath, data) - return data + return Promise.resolve(data) } getResourcePath(normalizedPath: string): string { return normalizedPath } - async mkdir(normalizedPath: string): Promise { + mkdir(normalizedPath: string): Promise { this.folders.add(normalizedPath) + return Promise.resolve() } - async trashSystem(): Promise { - return false + trashSystem(): Promise { + return Promise.resolve(false) } - async trashLocal(): Promise { - throw new Error('Not implemented') + trashLocal(): Promise { + return Promise.reject(new Error('Not implemented')) } - async rmdir(normalizedPath: string): Promise { + rmdir(normalizedPath: string): Promise { this.folders.delete(normalizedPath) + return Promise.resolve() } - async remove(normalizedPath: string): Promise { + remove(normalizedPath: string): Promise { this.files.delete(normalizedPath) + return Promise.resolve() } - async rename(normalizedPath: string, normalizedNewPath: string): Promise { + rename(normalizedPath: string, normalizedNewPath: string): Promise { const data = this.files.get(normalizedPath) if (data !== undefined) { this.files.delete(normalizedPath) this.files.set(normalizedNewPath, data) - return + return Promise.resolve() } if (this.folders.delete(normalizedPath)) { this.folders.add(normalizedNewPath) } + return Promise.resolve() } - async copy(normalizedPath: string, normalizedNewPath: string): Promise { + copy(normalizedPath: string, normalizedNewPath: string): Promise { const data = this.files.get(normalizedPath) if (data === undefined) { - throw new Error(`Missing file: ${normalizedPath}`) + return Promise.reject(new Error(`Missing file: ${normalizedPath}`)) } this.files.set(normalizedNewPath, data) + return Promise.resolve() } svgFileCount(): number { return Array.from(this.files.keys()).filter(path => path.endsWith('.svg')).length } + + private hasPath(normalizedPath: string): boolean { + return this.folders.has(normalizedPath) || this.files.has(normalizedPath) + } } diff --git a/src/cache.ts b/src/cache.ts index edb763a..5dbc199 100644 --- a/src/cache.ts +++ b/src/cache.ts @@ -37,7 +37,7 @@ export class PumlerDiskSvgCache implements PumlerSvgCache { async get(options: RenderDiagramOptions): Promise { try { return await this.enqueueOperation(() => this.readEntry(options)) - } catch (error) { + } catch { return null } } diff --git a/src/client.test.ts b/src/client.test.ts index 81d7b57..e38e25c 100644 --- a/src/client.test.ts +++ b/src/client.test.ts @@ -1,20 +1,22 @@ import { afterEach, beforeEach, describe, expect, test, vi } from 'vitest' -import { PumlerApiClient, PumlerRenderError } from './client' +import type { RequestUrlResponse } from 'obsidian' +import { PumlerApiClient, type PumlerRequestUrl } from './client' describe('PumlerApiClient', () => { + let requestUrlMock: ReturnType> + beforeEach(() => { - vi.stubGlobal('fetch', vi.fn()) + requestUrlMock = vi.fn() }) afterEach(() => { - vi.unstubAllGlobals() + vi.clearAllMocks() }) test('renders a diagram through the Pumler API', async () => { - const fetchMock = vi.mocked(fetch) - fetchMock.mockResolvedValue(createResponse(200, { diagram: '' })) + requestUrlMock.mockResolvedValue(createResponse(200, { diagram: '' })) - const client = new PumlerApiClient() + const client = new PumlerApiClient(requestUrlMock) const result = await client.renderDiagram({ provider: 'plantuml', type: 'sequence', @@ -23,8 +25,11 @@ describe('PumlerApiClient', () => { }) expect(result).toBe('') - expect(fetchMock).toHaveBeenCalledWith('https://api.pumler.com/api/diagram/render', expect.objectContaining({ + expect(requestUrlMock).toHaveBeenCalledWith(expect.objectContaining({ + url: 'https://api.pumler.com/api/diagram/render', method: 'POST', + contentType: 'application/json', + throw: false, body: JSON.stringify({ source: 'Alice -> Bob', metadata: { @@ -39,7 +44,7 @@ describe('PumlerApiClient', () => { }) test('maps structured API errors', async () => { - vi.mocked(fetch).mockResolvedValue(createResponse(400, { + requestUrlMock.mockResolvedValue(createResponse(400, { error: { message: 'Syntax error', line: 2, @@ -47,7 +52,7 @@ describe('PumlerApiClient', () => { } })) - const client = new PumlerApiClient() + const client = new PumlerApiClient(requestUrlMock) await expect(client.renderDiagram({ provider: 'plantuml', type: 'sequence', @@ -62,9 +67,9 @@ describe('PumlerApiClient', () => { }) test('maps network failures', async () => { - vi.mocked(fetch).mockRejectedValue(new Error('offline')) + requestUrlMock.mockRejectedValue(new Error('offline')) - const client = new PumlerApiClient() + const client = new PumlerApiClient(requestUrlMock) await expect(client.renderDiagram({ provider: 'mermaid', type: 'flowchart', @@ -74,10 +79,9 @@ describe('PumlerApiClient', () => { }) test('does not cache repeated render requests by itself', async () => { - const fetchMock = vi.mocked(fetch) - fetchMock.mockResolvedValue(createResponse(200, { diagram: '' })) + requestUrlMock.mockResolvedValue(createResponse(200, { diagram: '' })) - const client = new PumlerApiClient() + const client = new PumlerApiClient(requestUrlMock) const options = { provider: 'mermaid' as const, type: 'flowchart', @@ -88,14 +92,16 @@ describe('PumlerApiClient', () => { await client.renderDiagram(options) await client.renderDiagram(options) - expect(fetchMock).toHaveBeenCalledTimes(2) + expect(requestUrlMock).toHaveBeenCalledTimes(2) }) }) -function createResponse(status: number, data: unknown): Response { +function createResponse(status: number, data: unknown): RequestUrlResponse { return { - ok: status >= 200 && status < 300, status, - json: async () => data - } as Response + headers: {}, + arrayBuffer: new ArrayBuffer(0), + json: data, + text: JSON.stringify(data) + } } diff --git a/src/client.ts b/src/client.ts index 00086ca..3344a8a 100644 --- a/src/client.ts +++ b/src/client.ts @@ -1,3 +1,4 @@ +import type { RequestUrlParam, RequestUrlResponse } from 'obsidian' import { PUMLER_API_URL, type Provider, type ResolvedTheme } from './constants' export interface RenderDiagramOptions { @@ -19,6 +20,10 @@ interface PumlerErrorPayload { } } +export interface PumlerRequestUrl { + (request: RequestUrlParam): Promise +} + export class PumlerRenderError extends Error { readonly line?: number readonly column?: number @@ -32,15 +37,18 @@ export class PumlerRenderError extends Error { } export class PumlerApiClient { + constructor(private readonly requestUrl: PumlerRequestUrl) {} + async renderDiagram(options: RenderDiagramOptions, requestOptions: RenderDiagramRequestOptions = {}): Promise { - let response: Response + let response: RequestUrlResponse try { - response = await fetch(PUMLER_API_URL, { + response = await this.requestUrl({ + url: PUMLER_API_URL, method: 'POST', headers: { - Accept: 'application/json', - 'Content-Type': 'application/json' + Accept: 'application/json' }, + contentType: 'application/json', body: JSON.stringify({ source: options.source, metadata: { @@ -51,7 +59,7 @@ export class PumlerApiClient { } } }), - signal: requestOptions.signal + throw: false }) } catch (error) { if (requestOptions.signal?.aborted) { @@ -60,8 +68,8 @@ export class PumlerApiClient { throw new PumlerRenderError('Network error: unable to reach the Pumler rendering API') } - const data = await readJson(response) - if (!response.ok) { + const data = readJson(response) + if (response.status < 200 || response.status >= 300) { throw mapApiError(data) } @@ -84,12 +92,10 @@ export function createRenderDiagramCacheSeed(options: RenderDiagramOptions): str ]) } -async function readJson(response: Response): Promise | null> { - try { - return await response.json() - } catch { - return null - } +function readJson(response: RequestUrlResponse): Record | null { + return response.json && typeof response.json === 'object' + ? response.json as Record + : null } function mapApiError(payload: PumlerErrorPayload | Record | null): PumlerRenderError { diff --git a/src/main.ts b/src/main.ts index f7d9308..ea2c502 100644 --- a/src/main.ts +++ b/src/main.ts @@ -1,15 +1,16 @@ -import { MarkdownRenderChild, Plugin, type App, type MarkdownPostProcessorContext } from 'obsidian' +import { MarkdownRenderChild, Plugin, requestUrl, type App, type MarkdownPostProcessorContext } from 'obsidian' import { PumlerDiskSvgCache } from './cache' import { PumlerApiClient } from './client' import { parsePumlerBlock } from './parser' import { PumlerBlockRenderer } from './renderer' export default class PumlerPlugin extends Plugin { - async onload(): Promise { - const cache = this.manifest.dir - ? new PumlerDiskSvgCache(this.app.vault.adapter, `${this.manifest.dir}/cache`) - : undefined - const renderer = new PumlerBlockRenderer(new PumlerApiClient(), cache) + onload(): void { + const cache = new PumlerDiskSvgCache( + this.app.vault.adapter, + `${this.app.vault.configDir}/plugins/${this.manifest.id}/cache` + ) + const renderer = new PumlerBlockRenderer(new PumlerApiClient(requestUrl), cache) this.registerMarkdownCodeBlockProcessor('pumler', (source, element, context) => { const debounceKey = createDebounceKey(source, element, context) diff --git a/src/renderer.test.ts b/src/renderer.test.ts index 8265d20..0e76cec 100644 --- a/src/renderer.test.ts +++ b/src/renderer.test.ts @@ -22,7 +22,7 @@ describe('PumlerBlockRenderer', () => { }) test('renders sanitized SVG on success', async () => { - const renderer = new PumlerBlockRenderer(createClient(async () => '')) + const renderer = new PumlerBlockRenderer(createClient(() => Promise.resolve(''))) const element = document.createElement('div') await renderImmediately(renderer, validBlock(), element) @@ -38,7 +38,7 @@ describe('PumlerBlockRenderer', () => { }) test('open preview action does not toggle the summary panel', async () => { - const renderer = new PumlerBlockRenderer(createClient(async () => '')) + const renderer = new PumlerBlockRenderer(createClient(() => Promise.resolve(''))) const element = document.createElement('div') await renderImmediately(renderer, validBlock(), element) @@ -52,7 +52,7 @@ describe('PumlerBlockRenderer', () => { }) test('collapses and expands the diagram summary with title', async () => { - const renderer = new PumlerBlockRenderer(createClient(async () => '')) + const renderer = new PumlerBlockRenderer(createClient(() => Promise.resolve(''))) const element = document.createElement('div') await renderImmediately(renderer, validBlockWithTitle(), element) @@ -77,7 +77,7 @@ describe('PumlerBlockRenderer', () => { }) test('opens and closes a large preview', async () => { - const renderer = new PumlerBlockRenderer(createClient(async () => '')) + const renderer = new PumlerBlockRenderer(createClient(() => Promise.resolve(''))) const element = document.createElement('div') await renderImmediately(renderer, validBlock(), element) @@ -104,7 +104,7 @@ describe('PumlerBlockRenderer', () => { }) test('registers modal cleanup for render child unload', async () => { - const renderer = new PumlerBlockRenderer(createClient(async () => '')) + const renderer = new PumlerBlockRenderer(createClient(() => Promise.resolve(''))) const element = document.createElement('div') const cleanups: Array<() => void> = [] @@ -122,7 +122,7 @@ describe('PumlerBlockRenderer', () => { }) test('button zoom keeps the diagram center as focal point', async () => { - const renderer = new PumlerBlockRenderer(createClient(async () => '')) + const renderer = new PumlerBlockRenderer(createClient(() => Promise.resolve(''))) const element = document.createElement('div') await renderImmediately(renderer, validBlock(), element) @@ -142,7 +142,7 @@ describe('PumlerBlockRenderer', () => { }) test('zooms the large preview with modified wheel events', async () => { - const renderer = new PumlerBlockRenderer(createClient(async () => '')) + const renderer = new PumlerBlockRenderer(createClient(() => Promise.resolve(''))) const element = document.createElement('div') await renderImmediately(renderer, validBlock(), element) @@ -173,7 +173,7 @@ describe('PumlerBlockRenderer', () => { }) test('wheel zoom keeps the cursor position as focal point', async () => { - const renderer = new PumlerBlockRenderer(createClient(async () => '')) + const renderer = new PumlerBlockRenderer(createClient(() => Promise.resolve(''))) const element = document.createElement('div') await renderImmediately(renderer, validBlock(), element) @@ -204,7 +204,7 @@ describe('PumlerBlockRenderer', () => { }) test('renders validation errors', async () => { - const renderer = new PumlerBlockRenderer(createClient(async () => '')) + const renderer = new PumlerBlockRenderer(createClient(() => Promise.resolve(''))) const element = document.createElement('div') await renderImmediately(renderer, 'Alice -> Bob', element) @@ -214,9 +214,7 @@ describe('PumlerBlockRenderer', () => { }) test('renders API errors with source location', async () => { - const renderer = new PumlerBlockRenderer(createClient(async () => { - throw new PumlerRenderError('Syntax error', 3, 7) - })) + const renderer = new PumlerBlockRenderer(createClient(() => Promise.reject(new PumlerRenderError('Syntax error', 3, 7)))) const element = document.createElement('div') await renderImmediately(renderer, validBlock(), element) @@ -228,10 +226,12 @@ describe('PumlerBlockRenderer', () => { test('renders cached SVGs immediately without waiting for debounce', async () => { vi.useFakeTimers() - const renderDiagram = vi.fn(async () => '') + const renderDiagram = vi.fn(() => Promise.resolve('')) + const cacheGet = vi.fn(() => Promise.resolve('')) + const cacheSet = vi.fn(() => Promise.resolve()) const cache: PumlerSvgCache = { - get: vi.fn(async () => ''), - set: vi.fn(async () => undefined) + get: cacheGet, + set: cacheSet } const renderer = new PumlerBlockRenderer(createClient(renderDiagram), cache) const element = document.createElement('div') @@ -242,13 +242,13 @@ describe('PumlerBlockRenderer', () => { }) expect(renderDiagram).not.toHaveBeenCalled() - expect(cache.get).toHaveBeenCalledWith(expect.objectContaining({ + expect(cacheGet).toHaveBeenCalledWith(expect.objectContaining({ provider: 'plantuml', type: 'sequence', theme: 'light', source: 'Alice -> Bob' })) - expect(cache.set).not.toHaveBeenCalled() + expect(cacheSet).not.toHaveBeenCalled() expect(element.querySelector('rect')).not.toBeNull() }) @@ -261,11 +261,13 @@ describe('PumlerBlockRenderer', () => { ` + const cacheGet = vi.fn(() => Promise.resolve(cachedSvg)) + const cacheSet = vi.fn(() => Promise.resolve()) const cache: PumlerSvgCache = { - get: vi.fn(async () => cachedSvg), - set: vi.fn(async () => undefined) + get: cacheGet, + set: cacheSet } - const renderer = new PumlerBlockRenderer(createClient(async () => ''), cache) + const renderer = new PumlerBlockRenderer(createClient(() => Promise.resolve('')), cache) const firstElement = document.createElement('div') const secondElement = document.createElement('div') @@ -284,10 +286,10 @@ describe('PumlerBlockRenderer', () => { }) test('does not skip cached renders for detached Obsidian processor elements', async () => { - const renderDiagram = vi.fn(async () => '') + const renderDiagram = vi.fn(() => Promise.resolve('')) const cache: PumlerSvgCache = { - get: vi.fn(async () => ''), - set: vi.fn(async () => undefined) + get: vi.fn(() => Promise.resolve('')), + set: vi.fn(() => Promise.resolve()) } const renderer = new PumlerBlockRenderer(createClient(renderDiagram), cache) const element = document.createElement('div') @@ -303,7 +305,7 @@ describe('PumlerBlockRenderer', () => { test('debounces repeated render requests for the same block', async () => { vi.useFakeTimers() - const renderDiagram = vi.fn(async () => '') + const renderDiagram = vi.fn(() => Promise.resolve('')) const renderer = new PumlerBlockRenderer(createClient(renderDiagram)) const element = document.createElement('div') @@ -333,7 +335,7 @@ describe('PumlerBlockRenderer', () => { test('keeps a pending debounced render alive when its view aborts', async () => { vi.useFakeTimers() - const renderDiagram = vi.fn(async () => '') + const renderDiagram = vi.fn(() => Promise.resolve('')) const renderer = new PumlerBlockRenderer(createClient(renderDiagram)) const element = document.createElement('div') const abortController = new AbortController() diff --git a/src/svg.ts b/src/svg.ts index f70568a..3f0b137 100644 --- a/src/svg.ts +++ b/src/svg.ts @@ -1,5 +1,5 @@ const SIZING_STYLE_PROPERTIES = ['width', 'height', 'max-width', 'max-height', 'min-width', 'min-height'] -const CSS_SELECTOR_SEPARATOR_PATTERN = /,(?![^\[]*\])/ +const CSS_SELECTOR_SEPARATOR_PATTERN = /,(?![^[]*])/ const ALLOWED_ELEMENTS = new Set([ 'svg',