From 8e4644cfe55feca5a3be859656a7ffbc8250966c Mon Sep 17 00:00:00 2001 From: Netajam Date: Tue, 24 Mar 2026 20:09:18 +0100 Subject: [PATCH] Review improvements on NanoID PR: validation, caching, and duplicate detection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Cache customAlphabet generator to avoid recreating on every call - Add input validation: NanoID length (4-128), alphabet (min 2 unique chars), single-char separators - Add UID duplicate detection via in-memory Set + filePath→uid Map (O(1) lookups) - Build cache once on plugin load, keep in sync on create/delete/external edits - Listen to metadataCache.on('changed') to detect external frontmatter modifications - Rebuild cache when uidKey setting changes - Use path map for reliable cache cleanup on file delete - Fix escaped apostrophes in JSDoc comments - Revert unrelated quote-style changes Co-Authored-By: Claude Opus 4.6 --- src/main.ts | 54 ++++++++++++++++++++++++++++++++ src/settings.ts | 30 +++++++++++------- src/uidUtils.ts | 83 ++++++++++++++++++++++++++++++++----------------- 3 files changed, 127 insertions(+), 40 deletions(-) diff --git a/src/main.ts b/src/main.ts index 27b60d3..10589a7 100644 --- a/src/main.ts +++ b/src/main.ts @@ -9,6 +9,8 @@ import * as uidUtils from './uidUtils'; export default class UIDGenerator extends Plugin { settings: UIDGeneratorSettings; + uidCache: Set = new Set(); + uidPathMap: Map = new Map(); // filePath → uid async onload() { await this.loadSettings(); @@ -26,11 +28,46 @@ export default class UIDGenerator extends Plugin { // could be implemented but adds complexity (managing timers per file path). const debouncedFileHandler = debounce(commands.handleAutoGenerateUid.bind(null, this), 500, true); this.app.workspace.onLayoutReady(() => { + this.buildUidCache(); this.registerEvent(this.app.vault.on('create', (file) => { if (file instanceof TFile && file.extension === 'md') { + // Sync cache for files dropped/synced into vault that already have a UID + const existingUid = uidUtils.getUIDFromFile(this, file); + if (existingUid) { + this.uidCache.add(existingUid); + this.uidPathMap.set(file.path, existingUid); + } debouncedFileHandler(file); } })); + this.registerEvent(this.app.vault.on('delete', (file) => { + if (file instanceof TFile && file.extension === 'md') { + // Use path map since metadata cache may already be cleared + const uid = this.uidPathMap.get(file.path); + if (uid) { + this.uidCache.delete(uid); + this.uidPathMap.delete(file.path); + } + } + })); + // Sync cache when frontmatter is edited externally or by another plugin + this.registerEvent(this.app.metadataCache.on('changed', (file) => { + if (file instanceof TFile && file.extension === 'md') { + const oldUid = this.uidPathMap.get(file.path); + const newUid = uidUtils.getUIDFromFile(this, file); + if (oldUid !== newUid) { + if (oldUid) { + this.uidCache.delete(oldUid); + } + if (newUid) { + this.uidCache.add(newUid); + this.uidPathMap.set(file.path, newUid); + } else { + this.uidPathMap.delete(file.path); + } + } + } + })); this.registerEvent(this.app.workspace.on('file-open', (file) => { if (file instanceof TFile && file.extension === 'md') { const activeLeaf = this.app.workspace.getActiveViewOfType(MarkdownView); @@ -187,6 +224,9 @@ export default class UIDGenerator extends Plugin { if (!Array.isArray(this.settings.autoGenerationExclusions)) { this.settings.autoGenerationExclusions = []; } + if (!Array.isArray(this.settings.nanoidSeparators)) { + this.settings.nanoidSeparators = []; + } this.settings.copyFormatString = this.settings.copyFormatString || DEFAULT_SETTINGS.copyFormatString; this.settings.copyFormatStringMissingUid = this.settings.copyFormatStringMissingUid || DEFAULT_SETTINGS.copyFormatStringMissingUid; } @@ -196,6 +236,20 @@ export default class UIDGenerator extends Plugin { } + // --- UID Cache --- + buildUidCache(): void { + this.uidCache.clear(); + this.uidPathMap.clear(); + const files = this.app.vault.getMarkdownFiles(); + for (const file of files) { + const uid = uidUtils.getUIDFromFile(this, file); + if (uid) { + this.uidCache.add(uid); + this.uidPathMap.set(file.path, uid); + } + } + } + // --- Public method needed by Settings Tab --- async clearUIDsInFolder(folderPath: string): Promise { await commands.handleClearUIDsInFolder(this, folderPath); diff --git a/src/settings.ts b/src/settings.ts index 72db8d3..57fd64f 100644 --- a/src/settings.ts +++ b/src/settings.ts @@ -59,13 +59,17 @@ export class UIDSettingTab extends PluginSettingTab { .setValue(this.plugin.settings.uidKey) .onChange(async (value) => { const cleanedValue = value.trim().replace(/\s+/g, ''); - this.plugin.settings.uidKey = cleanedValue || DEFAULT_SETTINGS.uidKey; + const newKey = cleanedValue || DEFAULT_SETTINGS.uidKey; + const keyChanged = newKey !== this.plugin.settings.uidKey; + this.plugin.settings.uidKey = newKey; if (value !== this.plugin.settings.uidKey) { - // Update input value if it was cleaned (e.g., spaces removed) text.setValue(this.plugin.settings.uidKey); } await this.plugin.saveSettings(); - this.display(); // Re-render to potentially update descriptions elsewhere + if (keyChanged) { + this.plugin.buildUidCache(); + } + this.display(); })); // --- UID Generator Type --- @@ -87,12 +91,12 @@ export class UIDSettingTab extends PluginSettingTab { if (this.plugin.settings.uidGenerator === 'nanoid') { new Setting(containerEl) .setName('NanoID length') - .setDesc('Length of the generated ID (excluding injected characters).') + .setDesc('Length of the generated ID (excluding separators). Min 4, max 128.') .addText(text => text .setValue(String(this.plugin.settings.nanoidLength)) .onChange(async (value) => { const num = parseInt(value); - if (!isNaN(num) && num > 0) { + if (!isNaN(num) && num >= 4 && num <= 128) { this.plugin.settings.nanoidLength = num; await this.plugin.saveSettings(); } @@ -100,11 +104,12 @@ export class UIDSettingTab extends PluginSettingTab { new Setting(containerEl) .setName('NanoID alphabet') - .setDesc('Custom characters to use for ID generation.') + .setDesc('Characters used for ID generation. Must have at least 2 unique characters.') .addTextArea(text => text .setValue(this.plugin.settings.nanoidAlphabet) .onChange(async (value) => { - if (value.length > 0) { + const unique = new Set(value); + if (unique.size >= 2) { this.plugin.settings.nanoidAlphabet = value; await this.plugin.saveSettings(); } @@ -131,12 +136,13 @@ export class UIDSettingTab extends PluginSettingTab { })); new Setting(groupContainer) - .setName(`Inject character`) - .setDesc('Character to inject (e.g., "+").') + .setName('Inject character') + .setDesc('Single character to inject (e.g., "-").') .addText(text => text .setValue(separator.char) .onChange(async (value) => { - this.plugin.settings.nanoidSeparators[index].char = value; + const char = value.slice(0, 1); + this.plugin.settings.nanoidSeparators[index].char = char; await this.plugin.saveSettings(); })); @@ -161,7 +167,7 @@ export class UIDSettingTab extends PluginSettingTab { .setButtonText('Add Group') .setCta() .onClick(async () => { - this.plugin.settings.nanoidSeparators.push({ char: '', position: -2 }); + this.plugin.settings.nanoidSeparators.push({ char: '-', position: -2 }); await this.plugin.saveSettings(); this.display(); })); @@ -309,7 +315,7 @@ export class UIDSettingTab extends PluginSettingTab { const uidKey = this.plugin.settings.uidKey; // Basic validation for the folder path input if (!folderPath || folderPath.trim() === '') { - new Notice('Please specify a folder path in the \'Folder to clear uids from\' setting above first.'); + new Notice("Please specify a folder path in the 'Folder to clear uids from' setting above first."); return; // Prevent proceeding without a path } diff --git a/src/uidUtils.ts b/src/uidUtils.ts index 896ac99..0260fb1 100644 --- a/src/uidUtils.ts +++ b/src/uidUtils.ts @@ -3,38 +3,61 @@ import { v4 as uuidv4 } from 'uuid'; import { customAlphabet } from 'nanoid'; import UIDGenerator from './main'; +// Cache the nanoid generator to avoid recreating on every call +let cachedNanoid: (() => string) | null = null; +let cachedAlphabet = ''; +let cachedLength = 0; + +function getNanoidGenerator(alphabet: string, length: number): () => string { + if (cachedNanoid && cachedAlphabet === alphabet && cachedLength === length) { + return cachedNanoid; + } + cachedAlphabet = alphabet; + cachedLength = length; + cachedNanoid = customAlphabet(alphabet, length); + return cachedNanoid; +} + /** - * Generates a unique ID using the UUID v4 standard. + * Generates a unique ID using UUID v4 or NanoID based on settings. * @param plugin The UIDGenerator plugin instance (for settings). */ +const MAX_COLLISION_RETRIES = 10; + export function generateUID(plugin: UIDGenerator): string { + for (let attempt = 0; attempt < MAX_COLLISION_RETRIES; attempt++) { + const id = generateRawUID(plugin); + if (!plugin.uidCache.has(id)) { + return id; + } + console.warn(`[UIDGenerator] Collision detected for "${id}", retrying (${attempt + 1}/${MAX_COLLISION_RETRIES})`); + } + // Fallback: return the last generated ID anyway (extremely unlikely to reach here) + const fallback = generateRawUID(plugin); + console.error(`[UIDGenerator] Failed to generate unique ID after ${MAX_COLLISION_RETRIES} attempts. Using potentially duplicate ID.`); + return fallback; +} + +function generateRawUID(plugin: UIDGenerator): string { if (plugin.settings.uidGenerator === 'nanoid') { - const alphabet = plugin.settings.nanoidAlphabet; - const length = plugin.settings.nanoidLength; - const nanoid = customAlphabet(alphabet, length); + const { nanoidAlphabet, nanoidLength, nanoidSeparators } = plugin.settings; + const nanoid = getNanoidGenerator(nanoidAlphabet, nanoidLength); let id = nanoid(); - const separators = plugin.settings.nanoidSeparators; - - if (separators && separators.length > 0) { - // Create a mutable array of insertions, normalizing positions - const insertions = separators - .filter(s => s.char && s.char.length > 0) // Only process non-empty chars + if (nanoidSeparators && nanoidSeparators.length > 0) { + const insertions = nanoidSeparators + .filter(s => s.char && s.char.length === 1) .map(s => { - let actualPosition = s.position; - // Normalize negative positions relative to the *initial* nanoid length - if (actualPosition < 0) { - actualPosition = length + actualPosition; // e.g., length=21, pos=-2 -> 19 + let pos = s.position; + if (pos < 0) { + pos = nanoidLength + pos; } - // Ensure position is within bounds [0, length] - actualPosition = Math.max(0, Math.min(length, actualPosition)); - return { char: s.char, position: actualPosition }; + pos = Math.max(0, Math.min(nanoidLength, pos)); + return { char: s.char, position: pos }; }) - .sort((a, b) => b.position - a.position); // Sort descending by position + .sort((a, b) => b.position - a.position); - // Apply insertions from right to left to avoid index shifting issues - for (const insertion of insertions) { - const { char, position } = insertion; + for (const { char, position } of insertions) { id = id.slice(0, position) + char + id.slice(position); } } @@ -59,7 +82,7 @@ export function getUIDFromFile(plugin: UIDGenerator, file: TFile | null): string return null; } const uidValue = frontmatter[plugin.settings.uidKey]; - // Ensure it\'s treated as a string, even if stored as number in YAML + // Ensure it's treated as a string, even if stored as number in YAML return typeof uidValue === 'string' || typeof uidValue === 'number' ? String(uidValue) : null; } catch (error) { console.error(`[UIDGenerator] Error reading metadata for ${file.path}:`, error); @@ -68,7 +91,7 @@ export function getUIDFromFile(plugin: UIDGenerator, file: TFile | null): string } /** - * Sets or updates the UID property in a file\'s frontmatter. + * Sets or updates the UID property in a file's frontmatter. * @param plugin The UIDGenerator plugin instance. * @param file The TFile to modify. * @param uid The UID string to set. @@ -103,7 +126,7 @@ export async function setUID(plugin: UIDGenerator, file: TFile, uid: string, ove frontmatter[key] = uid; uidWasSetOrOverwritten = true; } else { - // If overwrite is true but value is the same, we didn\'t *really* change it + // If overwrite is true but value is the same, we didn't *really* change it uidWasSetOrOverwritten = false; } @@ -115,8 +138,8 @@ export async function setUID(plugin: UIDGenerator, file: TFile, uid: string, ove }); if (uidWasSetOrOverwritten) { - // Determine action based on initial state and overwrite flag - const action = initialUidExists && overwrite ? 'Overwrote' : 'Set'; + plugin.uidCache.add(uid); + plugin.uidPathMap.set(file.path, uid); } return uidWasSetOrOverwritten; @@ -128,7 +151,7 @@ export async function setUID(plugin: UIDGenerator, file: TFile, uid: string, ove } /** - * Removes the UID property from a file\'s frontmatter, respecting the custom key. + * Removes the UID property from a file's frontmatter, respecting the custom key. * @param plugin The UIDGenerator plugin instance. * @param file The TFile to modify. * @returns Promise - True if a UID was found and removed, false otherwise. @@ -136,15 +159,19 @@ export async function setUID(plugin: UIDGenerator, file: TFile, uid: string, ove export async function removeUID(plugin: UIDGenerator, file: TFile): Promise { if (!file || !(file instanceof TFile)) return false; let uidWasPresent = false; + let removedUid: string | null = null; const key = plugin.settings.uidKey; try { await plugin.app.fileManager.processFrontMatter(file, (frontmatter) => { if (frontmatter.hasOwnProperty(key)) { + removedUid = String(frontmatter[key]); uidWasPresent = true; delete frontmatter[key]; } }); - if (uidWasPresent) { + if (uidWasPresent && removedUid) { + plugin.uidCache.delete(removedUid); + plugin.uidPathMap.delete(file.path); } return uidWasPresent; } catch (error) {