fix: code review round 4 — robustness and settings wiring

- Serialize generateKnowledgeNote to prevent TOCTOU and duplicates (Fix #33/34)
- Destroy preview on load failure to prevent Engine/Scene leak (Fix #38)
- Clean up stale error messages on model retry
- Pass getSettings to DirectModelView for snapshot save settings
This commit is contained in:
flash555588 2026-05-07 21:31:36 +08:00
parent 59b4e9e2f8
commit a5b0229204
3 changed files with 51 additions and 28 deletions

View file

@ -48,7 +48,7 @@ export default class AI3DModelWorkbench extends Plugin {
this.addSettingTab(new AI3DSettingTab(this.app, this));
// Register direct file view for .glb/.gltf/.stl
this.registerView(DIRECT_VIEW_TYPE, (leaf) => new DirectModelView(leaf));
this.registerView(DIRECT_VIEW_TYPE, (leaf) => new DirectModelView(leaf, () => this.getSettings()));
this.registerExtensions([...SUPPORTED_MODEL_EXTENSIONS], DIRECT_VIEW_TYPE);
// Register ```3d and ```3dgrid code block processors

View file

@ -1,4 +1,5 @@
import { FileView, TFile, type WorkspaceLeaf } from "obsidian";
import type { PluginSettings } from "../domain/models";
import { BabylonModelPreview } from "../render/babylon/scene";
import { createHelperButtons } from "./inline/helper-buttons";
@ -6,9 +7,11 @@ export const DIRECT_VIEW_TYPE = "ai3d-direct-view";
export class DirectModelView extends FileView {
private preview: BabylonModelPreview | null = null;
private getSettings: () => PluginSettings;
constructor(leaf: WorkspaceLeaf) {
constructor(leaf: WorkspaceLeaf, getSettings: () => PluginSettings) {
super(leaf);
this.getSettings = getSettings;
}
getViewType(): string {
@ -64,6 +67,7 @@ export class DirectModelView extends FileView {
// Remove just closes the tab
this.leaf.detach();
},
this.getSettings,
);
try {

View file

@ -252,9 +252,10 @@ export function mountWorkbench(
loading = true;
// Destroy previous preview
// Destroy previous preview and clean up old error messages
preview?.destroy();
preview = null;
previewHost.querySelectorAll(".ai3d-inline-empty:not(.ai3d-empty-state)").forEach(el => el.remove());
// Clear empty state, show loading
emptyState.style.display = "none";
@ -277,6 +278,8 @@ export function mountWorkbench(
ps.store.setState({ modelPreview: summary });
} catch (err) {
console.error("[AI3D] Failed to load model:", err);
preview?.destroy();
preview = null;
canvas.remove();
emptyState.style.display = "";
const errDiv = previewHost.createDiv({ cls: "ai3d-inline-empty" });
@ -303,37 +306,53 @@ function createDefaultProfile(): ModelAssetProfile {
return { tags: [], notes: "", createdAt: new Date().toISOString(), updatedAt: new Date().toISOString() };
}
/** Guard against concurrent or duplicate note generation calls. */
let noteGenerationLock: Promise<void> | null = null;
async function generateKnowledgeNote(app: App, state: PluginState) {
const path = state.currentModelPath;
if (!path) return;
// Serialize concurrent calls to prevent duplicate note creation
if (noteGenerationLock) await noteGenerationLock;
let resolveLock!: () => void;
noteGenerationLock = new Promise<void>(r => { resolveLock = r; });
const profile = state.modelAssetProfiles[path];
const preview = state.modelPreview;
const fileName = path.split("/").pop() ?? "model";
const baseName = fileName.replace(/\.[^.]+$/, "");
const reportFolder = state.settings.reportFolder;
const notePath = `${reportFolder}/${baseName} Report.md`;
try {
const path = state.currentModelPath;
if (!path) return;
// Check if note already exists
const exists = await app.vault.adapter.exists(notePath);
if (exists) {
const file = app.vault.getAbstractFileByPath(notePath);
if (file instanceof TFile) {
// Update existing file
const content = buildNoteContent(baseName, path, profile, preview);
await app.vault.modify(file, content);
const profile = state.modelAssetProfiles[path];
const preview = state.modelPreview;
const fileName = path.split("/").pop() ?? "model";
const baseName = fileName.replace(/\.[^.]+$/, "");
const reportFolder = state.settings.reportFolder;
const notePath = `${reportFolder}/${baseName} Report.md`;
const content = buildNoteContent(baseName, path, profile, preview);
// If file exists, update it; otherwise create (with fallback if concurrent creation won)
const existingFile = app.vault.getAbstractFileByPath(notePath);
if (existingFile instanceof TFile) {
await app.vault.modify(existingFile, content);
return;
}
return;
}
// Ensure folder exists
const folder = app.vault.getAbstractFileByPath(reportFolder);
if (!folder) {
await app.vault.createFolder(reportFolder).catch(() => {});
}
// Ensure folder exists
const folder = app.vault.getAbstractFileByPath(reportFolder);
if (!folder) {
await app.vault.createFolder(reportFolder).catch(() => {});
}
const content = buildNoteContent(baseName, path, profile, preview);
await app.vault.create(notePath, content);
try {
await app.vault.create(notePath, content);
} catch {
// File was created concurrently — fall back to modify
const file = app.vault.getAbstractFileByPath(notePath);
if (file instanceof TFile) {
await app.vault.modify(file, content);
}
}
} finally {
resolveLock();
if (noteGenerationLock) noteGenerationLock = null;
}
}
function buildNoteContent(