From 3e7e09c83c309271e18f8f2f32cac7281467e495 Mon Sep 17 00:00:00 2001 From: murashit Date: Wed, 13 May 2026 08:39:00 +0900 Subject: [PATCH] Address Obsidian review warnings --- src/app-server/client.ts | 8 +++---- src/app-server/types.ts | 2 +- src/electron.d.ts | 5 +++++ src/main.ts | 37 +++++++++++++++++++++----------- src/panel/app-server-logs.ts | 21 ++++++++++++++---- src/panel/thread-naming.ts | 6 +++--- src/view/composer.ts | 8 ++++--- src/view/dom.ts | 7 +++--- src/view/toolbar.ts | 2 +- tests/app-server-client.test.ts | 9 +++++++- tests/app-server-logs.test.ts | 15 +++++++++++++ tests/connection-manager.test.ts | 9 +++++++- tests/mocks/obsidian.ts | 9 ++++++++ tests/view-renderers.test.ts | 6 ++++++ 14 files changed, 110 insertions(+), 34 deletions(-) create mode 100644 src/electron.d.ts diff --git a/src/app-server/client.ts b/src/app-server/client.ts index dfbf77b4..0e801c8b 100644 --- a/src/app-server/client.ts +++ b/src/app-server/client.ts @@ -337,7 +337,7 @@ export class AppServerClient { private request(method: M, params: ClientRequestParams): Promise { const id = this.nextId++; const promise = new Promise((resolve, reject) => { - const timeout = globalThis.setTimeout(() => { + const timeout = window.setTimeout(() => { this.pending.delete(id); reject(new Error(`Codex app-server request timed out: ${method}`)); }, this.requestTimeoutMs); @@ -354,7 +354,7 @@ export class AppServerClient { } catch (error) { const pending = this.pending.get(id); if (pending) { - globalThis.clearTimeout(pending.timeout); + window.clearTimeout(pending.timeout); this.pending.delete(id); } throw error; @@ -396,7 +396,7 @@ export class AppServerClient { this.handlers.onLog(`Orphan app-server response: ${JSON.stringify(message)}`); return; } - globalThis.clearTimeout(pending.timeout); + window.clearTimeout(pending.timeout); this.pending.delete(message.id); if ("error" in message && message.error) { pending.reject(new Error(message.error.message || "Codex app-server request failed.")); @@ -413,7 +413,7 @@ export class AppServerClient { private rejectAll(error: Error): void { for (const pending of this.pending.values()) { - globalThis.clearTimeout(pending.timeout); + window.clearTimeout(pending.timeout); pending.reject(error); } this.pending.clear(); diff --git a/src/app-server/types.ts b/src/app-server/types.ts index 2e840b7a..91df1588 100644 --- a/src/app-server/types.ts +++ b/src/app-server/types.ts @@ -26,5 +26,5 @@ export interface PendingRequest { method: ClientRequestMethod; reject: (reason: Error) => void; resolve: (value: unknown) => void; - timeout: ReturnType; + timeout: ReturnType; } diff --git a/src/electron.d.ts b/src/electron.d.ts new file mode 100644 index 00000000..e0c2f04a --- /dev/null +++ b/src/electron.d.ts @@ -0,0 +1,5 @@ +declare module "electron" { + export const shell: { + openPath(path: string): Promise; + }; +} diff --git a/src/main.ts b/src/main.ts index a559b918..1cd0869f 100644 --- a/src/main.ts +++ b/src/main.ts @@ -71,10 +71,6 @@ import { renderPendingRequestMessage } from "./view/pending-request-message"; import { bottomScrollTop, captureScrollAnchor, isNearScrollBottom, restoreScrollAnchor } from "./view/scroll"; import { renderToolbar, toolbarSignature, type ToolbarChoice, type ToolbarViewModel } from "./view/toolbar"; -interface ElectronShell { - openPath(path: string): Promise; -} - export default class CodexPanelPlugin extends Plugin { settings: CodexPanelSettings = DEFAULT_SETTINGS; vaultPath = ""; @@ -90,13 +86,13 @@ export default class CodexPanelPlugin extends Plugin { }); this.addCommand({ - id: "open-codex-panel", + id: "open-panel", name: "Open panel", callback: () => void this.activateView(), }); this.addCommand({ - id: "new-codex-chat", + id: "new-chat", name: "New chat", callback: async () => { const view = await this.activateView(); @@ -134,7 +130,7 @@ export default class CodexPanelPlugin extends Plugin { } async loadSettings(): Promise { - const data = await this.loadData(); + const data: unknown = await this.loadData(); this.settings = normalizeSettings(data); if (!settingsMatchNormalizedData(data, this.settings)) { await this.saveSettings(); @@ -255,7 +251,7 @@ class CodexPanelView extends ItemView { async onOpen(): Promise { this.registerNoteIndexInvalidation(); - this.registerDomEvent(document, "pointerdown", (event) => this.closeToolbarPanelOnOutsidePointer(event)); + this.registerDomEvent(activeDocument, "pointerdown", (event) => this.closeToolbarPanelOnOutsidePointer(event)); this.render(); await this.ensureConnected(); } @@ -691,7 +687,7 @@ class CodexPanelView extends ItemView { } try { - const shell = (require("electron") as { shell?: ElectronShell }).shell; + const { shell } = await import("electron"); if (!shell) throw new Error("Electron shell is not available."); const vaultCodexPath = join(this.plugin.vaultPath, ".codex"); @@ -910,7 +906,7 @@ class CodexPanelView extends ItemView { return [ `Model: ${currentModel(snapshot, config) ?? "(from default)"}`, `Override: ${runtimeOverrideLabel(this.state.requestedModel)}`, - `Provider: ${config.model_provider ?? "(from default)"}`, + `Provider: ${statusValue(config.model_provider, "(from default)")}`, `Effort: ${currentReasoningEffort(snapshot, config) ?? "(from default)"}`, `Mode: ${this.collaborationModeLabel()}`, `Service tier: ${serviceTierLabel(snapshot, config)}`, @@ -1035,7 +1031,7 @@ class CodexPanelView extends ItemView { workspaceRoot: this.state.activeThreadCwd ?? this.plugin.vaultPath, openDetails: this.state.openDetails, onDetailsToggle: () => { - requestAnimationFrame(() => { + window.requestAnimationFrame(() => { this.state.messagesPinnedToBottom = isNearScrollBottom(messagesEl); }); }, @@ -1075,7 +1071,7 @@ class CodexPanelView extends ItemView { } } - requestAnimationFrame(() => { + window.requestAnimationFrame(() => { if (shouldScrollToBottom) { messagesEl.scrollTop = bottomScrollTop(messagesEl); } else { @@ -1121,7 +1117,7 @@ class CodexPanelView extends ItemView { if (!this.state.messagesPinnedToBottom) return; const messagesEl = parent.closest(".codex-panel__messages"); if (!messagesEl) return; - requestAnimationFrame(() => { + window.requestAnimationFrame(() => { if (!this.state.messagesPinnedToBottom) return; messagesEl.scrollTop = bottomScrollTop(messagesEl); this.state.messagesPinnedToBottom = isNearScrollBottom(messagesEl); @@ -1323,3 +1319,18 @@ class CodexPanelView extends ItemView { this.registerEvent(this.app.vault.on("modify", invalidate)); } } + +function statusValue(value: unknown, fallback: string): string { + if (typeof value === "string") return value; + if (typeof value === "number" || typeof value === "boolean" || typeof value === "bigint") return String(value); + if (value === null || value === undefined) return fallback; + return jsonPreview(value, fallback); +} + +function jsonPreview(value: unknown, fallback: string): string { + try { + return JSON.stringify(value) ?? fallback; + } catch { + return fallback; + } +} diff --git a/src/panel/app-server-logs.ts b/src/panel/app-server-logs.ts index 415d9872..e19c96a3 100644 --- a/src/panel/app-server-logs.ts +++ b/src/panel/app-server-logs.ts @@ -1,15 +1,17 @@ export type ClassifiedAppServerLog = { kind: "plain"; text: string } | { kind: "error"; text: string } | null; +const ANSI_PATTERN = new RegExp("\\u001B\\[[0-?]*[ -/]*[@-~]", "g"); + export function classifyAppServerLog(message: string): ClassifiedAppServerLog { const normalized = stripAnsi(message).trimEnd(); if (!normalized || isMcpTokenRefreshLog(normalized)) return null; const parsed = parseAppServerLog(normalized); if (!parsed) return null; - const level = String(parsed.level ?? "").toUpperCase(); + const level = logString(parsed.level).toUpperCase(); const fields = parsed.fields && typeof parsed.fields === "object" ? (parsed.fields as Record) : {}; - const text = stripAnsi(String(fields.message ?? normalized)); - const target = String(parsed.target ?? ""); + const text = stripAnsi(logString(fields.message, normalized)); + const target = logString(parsed.target); if (target.includes("rmcp::transport::worker") && isMcpTokenRefreshLog(text)) return null; if (target.includes("codex_core::tools::router") && text.includes("apply_patch verification failed")) return null; if (level === "ERROR") return { kind: "error", text }; @@ -30,5 +32,16 @@ function isMcpTokenRefreshLog(message: string): boolean { } function stripAnsi(message: string): string { - return message.replace(/\u001b\[[0-?]*[ -/]*[@-~]/g, ""); + return message.replace(ANSI_PATTERN, ""); +} + +function logString(value: unknown, fallback = ""): string { + if (typeof value === "string") return value; + if (typeof value === "number" || typeof value === "boolean" || typeof value === "bigint") return String(value); + if (value === null || value === undefined) return fallback; + try { + return JSON.stringify(value) ?? fallback; + } catch { + return fallback; + } } diff --git a/src/panel/thread-naming.ts b/src/panel/thread-naming.ts index 0ab9649f..b7e55aaf 100644 --- a/src/panel/thread-naming.ts +++ b/src/panel/thread-naming.ts @@ -70,14 +70,14 @@ export async function generateThreadTitleWithCodex( let namingThreadId: string | null = null; let expectedTurnId: string | null = null; let completed = false; - let timeout: ReturnType | null = null; + let timeout: ReturnType | null = null; let rejectCompletedTurn: ((error: Error) => void) | null = null; let handleNamingNotification: (notification: ServerNotification) => void = () => undefined; const completedItems: ThreadItem[] = []; const completedTurn = new Promise((resolve, reject) => { rejectCompletedTurn = reject; - timeout = globalThis.setTimeout(() => { + timeout = window.setTimeout(() => { if (completed) return; completed = true; reject(new Error("Timed out while generating a Codex thread title.")); @@ -133,7 +133,7 @@ export async function generateThreadTitleWithCodex( return titleFromNamingTurn(turn); } finally { completed = true; - if (timeout) globalThis.clearTimeout(timeout); + if (timeout) window.clearTimeout(timeout); client.disconnect(); } } diff --git a/src/view/composer.ts b/src/view/composer.ts index 46d2df3a..2886382d 100644 --- a/src/view/composer.ts +++ b/src/view/composer.ts @@ -105,10 +105,12 @@ export function syncComposerHeight(composer: HTMLTextAreaElement | null): void { const style = getComputedStyle(composer); const minHeight = parseCssPixels(style.minHeight, 76); const maxHeight = composerMaxHeight(style.maxHeight); - composer.style.height = "auto"; + composer.setCssProps({ height: "auto" }); const nextHeight = Math.min(Math.max(composer.scrollHeight, minHeight), maxHeight); - composer.style.height = `${nextHeight}px`; - composer.style.overflowY = composer.scrollHeight > maxHeight ? "auto" : "hidden"; + composer.setCssProps({ + height: `${nextHeight}px`, + "overflow-y": composer.scrollHeight > maxHeight ? "auto" : "hidden", + }); } function parseCssPixels(value: string, fallback: number): number { diff --git a/src/view/dom.ts b/src/view/dom.ts index 539960a0..4fb2d174 100644 --- a/src/view/dom.ts +++ b/src/view/dom.ts @@ -8,11 +8,12 @@ export function shortSignature(value: string): string { export function renderTextWithWikiLinks(parent: HTMLElement, text: string, openLink: (target: string) => void): void { const wikilinkPattern = /\[\[([^\]\n]+?)\]\]/g; + const doc = parent.ownerDocument; let lastIndex = 0; for (const match of text.matchAll(wikilinkPattern)) { const index = match.index ?? 0; if (index > lastIndex) { - parent.appendChild(document.createTextNode(text.slice(lastIndex, index))); + parent.appendChild(doc.createTextNode(text.slice(lastIndex, index))); } const rawLink = match[1] ?? ""; @@ -21,7 +22,7 @@ export function renderTextWithWikiLinks(parent: HTMLElement, text: string, openL const label = (separator === -1 ? rawLink : rawLink.slice(separator + 1)).trim() || target; if (target.length === 0) { - parent.appendChild(document.createTextNode(match[0])); + parent.appendChild(doc.createTextNode(match[0])); } else { const link = parent.createEl("a", { cls: "internal-link codex-panel__wikilink", @@ -40,6 +41,6 @@ export function renderTextWithWikiLinks(parent: HTMLElement, text: string, openL } if (lastIndex < text.length) { - parent.appendChild(document.createTextNode(text.slice(lastIndex))); + parent.appendChild(doc.createTextNode(text.slice(lastIndex))); } } diff --git a/src/view/toolbar.ts b/src/view/toolbar.ts index 3ecac3f9..496be350 100644 --- a/src/view/toolbar.ts +++ b/src/view/toolbar.ts @@ -345,7 +345,7 @@ function renderThreadRenameRow(parent: HTMLElement, thread: ToolbarThreadRow, ac } }; window.setTimeout(() => { - if (document.activeElement !== input) { + if (input.ownerDocument.activeElement !== input) { input.focus(); input.select(); } diff --git a/tests/app-server-client.test.ts b/tests/app-server-client.test.ts index a4c14b2b..1f4f37c7 100644 --- a/tests/app-server-client.test.ts +++ b/tests/app-server-client.test.ts @@ -1,4 +1,4 @@ -import { describe, expect, it } from "vitest"; +import { beforeEach, describe, expect, it, vi } from "vitest"; import { AppServerClient } from "../src/app-server/client"; import type { AppServerTransport, AppServerTransportHandlers } from "../src/app-server/transport"; @@ -57,6 +57,13 @@ async function connectedClient(): Promise<{ client: AppServerClient; transport: } describe("AppServerClient", () => { + beforeEach(() => { + vi.stubGlobal("window", { + clearTimeout, + setTimeout, + }); + }); + it("routes responses, notifications, and server requests", async () => { let transport: FakeTransport; const getTransport = () => transport; diff --git a/tests/app-server-logs.test.ts b/tests/app-server-logs.test.ts index 9c1e267b..b90d55f1 100644 --- a/tests/app-server-logs.test.ts +++ b/tests/app-server-logs.test.ts @@ -27,6 +27,21 @@ describe("app-server log classification", () => { }); }); + it("renders structured JSON log fields without object stringification", () => { + expect( + classifyAppServerLog( + JSON.stringify({ + level: "ERROR", + fields: { message: { error: "boom" } }, + target: { crate: "codex" }, + }), + ), + ).toEqual({ + kind: "error", + text: '{"error":"boom"}', + }); + }); + it("suppresses structured apply_patch router verification logs", () => { expect( classifyAppServerLog( diff --git a/tests/connection-manager.test.ts b/tests/connection-manager.test.ts index 4a947127..3f126c80 100644 --- a/tests/connection-manager.test.ts +++ b/tests/connection-manager.test.ts @@ -1,4 +1,4 @@ -import { describe, expect, it, vi } from "vitest"; +import { beforeEach, describe, expect, it, vi } from "vitest"; import { AppServerClient } from "../src/app-server/client"; import { ConnectionManager, StaleConnectionError } from "../src/app-server/connection-manager"; @@ -38,6 +38,13 @@ class SilentTransport implements AppServerTransport { } describe("ConnectionManager", () => { + beforeEach(() => { + vi.stubGlobal("window", { + clearTimeout, + setTimeout, + }); + }); + it("disconnects clients whose initialization fails", async () => { let transport!: SilentTransport; const manager = new ConnectionManager( diff --git a/tests/mocks/obsidian.ts b/tests/mocks/obsidian.ts index 1b541053..eff2ff7b 100644 --- a/tests/mocks/obsidian.ts +++ b/tests/mocks/obsidian.ts @@ -11,6 +11,7 @@ declare global { createEl(tag: K, options?: ElementOptions): HTMLElementTagNameMap[K]; createSpan(options?: ElementOptions): HTMLSpanElement; empty(): void; + setCssProps(props: Record): void; setAttr(name: string, value: string): void; } } @@ -236,6 +237,14 @@ function ensureElementHelpers(): void { }; } + if (!HTMLElement.prototype.setCssProps) { + HTMLElement.prototype.setCssProps = function setCssProps(props: Record): void { + for (const [key, value] of Object.entries(props)) { + this.style.setProperty(key, value); + } + }; + } + if (!HTMLElement.prototype.createEl) { HTMLElement.prototype.createEl = function createEl( tag: K, diff --git a/tests/view-renderers.test.ts b/tests/view-renderers.test.ts index ba03b2d3..068e339e 100644 --- a/tests/view-renderers.test.ts +++ b/tests/view-renderers.test.ts @@ -28,6 +28,7 @@ declare global { createSpan(options?: { cls?: string; text?: string; attr?: Record }): HTMLSpanElement; empty(): void; hide(): void; + setCssProps(props: Record): void; setAttr(name: string, value: string): void; show(): void; } @@ -75,6 +76,11 @@ beforeEach(() => { HTMLElement.prototype.setAttr = function setAttr(this: HTMLElement, name: string, value: string): void { this.setAttribute(name, value); }; + HTMLElement.prototype.setCssProps = function setCssProps(this: HTMLElement, props: Record): void { + for (const [key, value] of Object.entries(props)) { + this.style.setProperty(key, value); + } + }; HTMLElement.prototype.scrollIntoView = vi.fn(); globalThis.createDiv = ((options: { cls?: string; text?: string; attr?: Record } = {}) => document.body.createDiv(options)) as typeof globalThis.createDiv;