refactor: simplify modal state handling and section rendering

Author: chhoumann

EXECPLAN-ui-state-persistence.md

@@ -25,6 +25,8 @@ After this change, a QuickAdd user will be able to edit complex settings (especi
 - [x] (2026-03-02 17:45Z) Implemented Follow-up 5: extracted shared behavior-section helpers for one-page override and file opening controls.
 - [x] (2026-03-02 17:48Z) Implemented Follow-up 6: added integration tests for queue + `settingsStore` subscription/dispose lifecycle.
 - [x] (2026-03-02 17:53Z) Implemented Follow-up 7: added stable CLI debug command surface (`quickadd:debug`) while retaining namespace compatibility.
+- [x] (2026-03-02 21:38Z) Added full simplification feedback backlog from Claude pass (`claude -p "/simplify but don't fix, just give the feedback"`).
+- [x] (2026-03-02 21:52Z) Implemented simplification backlog items across builder deduplication, CLI utility reuse, draft-session consistency, queue/preserve-ui refinements, and targeted efficiency cleanups; reran tests/build + Obsidian CLI verification.
 
 ## Surprises & Discoveries
 
@@ -258,6 +260,86 @@ Validation:
 - CLI transcript using only stable seam.
 - Backward-compatible behavior during deprecation window for global namespace access.
 
+## Simplification Feedback Backlog (Claude Pass, 2026-03-02)
+
+This section captures all feedback items from the Claude simplification pass.
+Items are tracked individually even where they overlap previously completed work.
+
+### Code Reuse
+
+- [x] [High] Remove dead base-class methods from
+      `src/gui/ChoiceBuilder/choiceBuilder.ts`:
+      `addOnePageOverrideSetting`,
+      `addOpenFileSetting`,
+      `addFileOpeningSetting`.
+- [x] [High] Deduplicate identical `renderSection` implementations in
+      `src/gui/ChoiceBuilder/captureChoiceBuilder.ts` and
+      `src/gui/ChoiceBuilder/templateChoiceBuilder.ts` by moving shared logic
+      to base class.
+- [x] [High] Avoid reimplementing flattening logic in
+      `src/cli/registerQuickAddCliHandlers.ts`; extend/reuse
+      `src/utils/choiceUtils.ts:flattenChoices` with optional path segments.
+- [x] [High] Replace repeated `instanceof Error ? error.message : String(error)`
+      handling in CLI with shared `src/utils/errorUtils.ts:toError`.
+- [x] [Low] Replace `checkObjectDiff` usage in
+      `src/gui/AIAssistant/AIAssistantProvidersModal.ts` with
+      `createDraftSession().isDirty()` where appropriate.
+- [x] [Low] Extract a shared save/cancel modal action bar helper used by:
+      `src/gui/MacroGUIs/OpenFileCommandSettingsModal.ts`,
+      `src/gui/MacroGUIs/ConditionalCommandSettingsModal.ts`,
+      `src/gui/MacroGUIs/ConditionalBranchEditorModal.ts`.
+
+### Code Quality
+
+- [x] [Medium] Simplify `flushNow` in
+      `src/state/settingsPersistenceQueue.ts` by separating timeout, retry, and
+      loop-wake responsibilities.
+- [x] [Medium] Document and/or refactor double-restore behavior in
+      `src/gui/utils/preserveUiContext.ts:scheduleRestore` to make intent
+      explicit and controllable.
+- [x] [Medium] Consolidate debug API access in `src/main.ts`:
+      prefer `getDebugApi(): QuickAddDebugApi | null` over multiple pass-through
+      debug methods and avoid leaking queue internals in public return types.
+- [x] [Medium] Migrate `src/gui/AIAssistant/AIAssistantProvidersModal.ts` to
+      `DraftSession` pattern for consistency with Open File modal editing.
+- [x] [Low] Remove dual source of truth for `filterQuery` in
+      `src/gui/ChoiceView.svelte` (local state vs `uiStore` field).
+- [x] [Low] Revisit `scheduledRevisions` stat in
+      `src/state/settingsPersistenceQueue.ts`; rename or remove to avoid
+      misleading metrics.
+- [x] [Low] Simplify no-op conditional branch in
+      `src/gui/MacroGUIs/ConditionalCommandSettingsModal.ts:cloneCondition`.
+- [x] [Low] Replace stringly-typed debug actions (`"get"`, `"reset"`, `"flush"`)
+      in `src/cli/registerQuickAddCliHandlers.ts` with typed constants/union.
+
+### Efficiency
+
+- [x] [Medium] Reduce repeated `querySelectorAll` work in
+      `src/gui/utils/preserveUiContext.ts` by reusing scroll target discovery
+      across capture + restore passes.
+- [x] [Medium] Add caching or memoization for vault-wide scans triggered by
+      section rerenders in:
+      `src/gui/ChoiceBuilder/captureChoiceBuilder.ts` and
+      `src/gui/ChoiceBuilder/templateChoiceBuilder.ts`.
+- [x] [Medium] Optimize `createDraftSession().isDirty()` in
+      `src/state/createDraftSession.ts` for repeated checks (dirty flag and/or
+      incremental tracking).
+- [x] [Medium] Cancel losing timeout in
+      `src/state/settingsPersistenceQueue.ts:waitForPromiseOrTimeout`.
+- [x] [Medium] Reconsider timeout window reset across retries in
+      `src/state/settingsPersistenceQueue.ts` so max wait does not scale by
+      retry count under persistent failure.
+- [x] [Low] Avoid scheduling persistence writes on semantically unchanged
+      settings in `src/main.ts` subscriber path.
+- [x] [Low] Memoize `prepareFuzzySearch` usage in
+      `src/gui/ChoiceView.svelte` when inputs are unchanged.
+- [x] [Low] Add cleanup/eviction strategy for stale `collapsedChoiceIds` in
+      `src/gui/stores/uiStore.ts`.
+- [x] [Low] Avoid full-vault template scans on every request in
+      `src/main.ts:getTemplateFiles` (cache/index by folder path).
+- [ ] [Low] Revisit `api` getter allocation pattern in `src/main.ts` to avoid
+      constructing a new `ChoiceExecutor` per access.
+
 ## Context and Orientation
 
 This repository is an Obsidian community plugin named QuickAdd. Obsidian plugins persist settings by calling the plugin method `saveData(...)`, which writes JSON into the plugin’s data file (`<vault>/.obsidian/plugins/quickadd/data.json`). In this codebase, settings are held in a Zustand store at `src/settingsStore.ts` and mirrored onto the plugin instance in `src/main.ts`.

src/cli/registerQuickAddCliHandlers.test.ts

@@ -97,19 +97,23 @@ function createPlugin(choices: IChoice[]) {
 				handlers.push({ command, description, flags, handler });
 			},
 		),
-		getDebugStats: vi.fn(() => debugStats),
-		resetDebugStats: vi.fn(() => true),
-		flushDebugPersistence: vi.fn(async () => debugStats.persistence),
+		getDebugApi: vi.fn(() => ({
+			getStats: () => debugStats,
+			resetStats: () => undefined,
+			flushNow: async () => debugStats.persistence,
+		})),
 	} as unknown as QuickAdd & {
 		registerCliHandler: (
 			command: string,
 			description: string,
 			flags: CliFlags | null,
 			handler: (params: CliData) => string | Promise<string>,
 		) => void;
-		getDebugStats: () => unknown;
-		resetDebugStats: () => boolean;
-		flushDebugPersistence: () => Promise<unknown>;
+		getDebugApi: () => {
+			getStats: () => unknown;
+			resetStats: () => void;
+			flushNow: () => Promise<unknown>;
+		};
 	};
 
 	return { plugin, handlers };

src/cli/registerQuickAddCliHandlers.ts

@@ -7,10 +7,12 @@ import {
 	collectChoiceRequirements,
 	getUnresolvedRequirements,
 } from "../preflight/collectChoiceRequirements";
+import { flattenChoicesWithPath } from "../utils/choiceUtils";
+import { toError } from "../utils/errorUtils";
 import type IChoice from "../types/choices/IChoice";
-import type IMultiChoice from "../types/choices/IMultiChoice";
 
 type ChoiceType = IChoice["type"];
+type DebugAction = "get" | "reset" | "flush";
 
 interface CliChoiceSummary {
 	id: string;
@@ -37,12 +39,14 @@ interface RegisterCliHandlerTarget {
 }
 
 interface DebugCliTarget {
-	getDebugStats?: () => {
-		persistence: unknown;
-		ui: unknown;
+	getDebugApi?: () => {
+		getStats: () => {
+			persistence: unknown;
+			ui: unknown;
+		};
+		resetStats: () => void;
+		flushNow: () => Promise<unknown>;
 	} | null;
-	resetDebugStats?: () => boolean;
-	flushDebugPersistence?: () => Promise<unknown>;
 }
 
 const RUN_FLAGS: CliFlags = {
@@ -95,6 +99,12 @@ const DEBUG_FLAGS: CliFlags = {
 	},
 };
 
+const DEBUG_ACTIONS = {
+	get: "get",
+	reset: "reset",
+	flush: "flush",
+} as const satisfies Record<DebugAction, DebugAction>;
+
 const RESERVED_RUN_PARAMS = new Set<string>(["choice", "id", "vars", "ui"]);
 const RESERVED_CHECK_PARAMS = new Set<string>(["choice", "id", "vars"]);
 
@@ -128,11 +138,7 @@ function parseVarsJson(value: string): Record<string, unknown> {
 	try {
 		parsed = JSON.parse(value);
 	} catch (error) {
-		throw new Error(
-			`Invalid vars JSON: ${
-				error instanceof Error ? error.message : String(error)
-			}`,
-		);
+		throw toError(error, "Invalid vars JSON");
 	}
 
 	if (!parsed || typeof parsed !== "object" || Array.isArray(parsed)) {
@@ -166,34 +172,6 @@ function extractVariables(
 	return variables;
 }
 
-function flattenChoices(
-	choices: IChoice[],
-	segments: string[] = [],
-): CliChoiceSummary[] {
-	const flattened: CliChoiceSummary[] = [];
-
-	for (const choice of choices) {
-		const pathSegments = [...segments, choice.name];
-		const path = pathSegments.join(" / ");
-		const isMulti = choice.type === "Multi";
-		flattened.push({
-			id: choice.id,
-			name: choice.name,
-			type: choice.type,
-			command: choice.command,
-			path,
-			runnable: !isMulti,
-		});
-
-		if (isMulti) {
-			const multiChoice = choice as IMultiChoice;
-			flattened.push(...flattenChoices(multiChoice.choices, pathSegments));
-		}
-	}
-
-	return flattened;
-}
-
 function resolveChoiceFromParams(plugin: QuickAdd, params: CliData): IChoice {
 	if (typeof params.id === "string" && params.id.trim().length > 0) {
 		return plugin.getChoiceById(params.id);
@@ -321,7 +299,7 @@ async function runChoiceHandler(
 		return serialize({
 			ok: false,
 			command,
-			error: error instanceof Error ? error.message : String(error),
+			error: toError(error).message,
 		});
 	}
 }
@@ -340,7 +318,20 @@ function listChoicesHandler(plugin: QuickAdd, params: CliData): string {
 			});
 		}
 
-		const allChoices = flattenChoices(plugin.settings.choices);
+		const allChoices = flattenChoicesWithPath(plugin.settings.choices).map(
+			({ choice, pathSegments }) => {
+				const isMulti = choice.type === "Multi";
+				const summary: CliChoiceSummary = {
+					id: choice.id,
+					name: choice.name,
+					type: choice.type,
+					command: choice.command,
+					path: pathSegments.join(" / "),
+					runnable: !isMulti,
+				};
+				return summary;
+			},
+		);
 		const choices = allChoices.filter((choice) => {
 			if (
 				requestedType &&
@@ -362,7 +353,7 @@ function listChoicesHandler(plugin: QuickAdd, params: CliData): string {
 		return serialize({
 			ok: false,
 			command: CLI_COMMANDS.list,
-			error: error instanceof Error ? error.message : String(error),
+			error: toError(error).message,
 		});
 	}
 }
@@ -415,15 +406,24 @@ async function checkChoiceHandler(
 		return serialize({
 			ok: false,
 			command: CLI_COMMANDS.check,
-			error: error instanceof Error ? error.message : String(error),
+			error: toError(error).message,
 		});
 	}
 }
 
+function parseDebugAction(actionRaw: string | undefined): DebugAction | null {
+	const normalized = (actionRaw ?? DEBUG_ACTIONS.get).trim().toLowerCase();
+	if (normalized === DEBUG_ACTIONS.get) return DEBUG_ACTIONS.get;
+	if (normalized === DEBUG_ACTIONS.reset) return DEBUG_ACTIONS.reset;
+	if (normalized === DEBUG_ACTIONS.flush) return DEBUG_ACTIONS.flush;
+	return null;
+}
+
 async function debugHandler(plugin: QuickAdd, params: CliData): Promise<string> {
-	const actionRaw = typeof params.action === "string" ? params.action : "get";
-	const action = actionRaw.trim().toLowerCase();
+	const actionRaw = typeof params.action === "string" ? params.action : undefined;
+	const action = parseDebugAction(actionRaw);
 	const debugTarget = plugin as unknown as DebugCliTarget;
+	const debugApi = debugTarget.getDebugApi?.() ?? null;
 
 	if (!plugin.settings.devMode) {
 		return serialize({
@@ -433,48 +433,56 @@ async function debugHandler(plugin: QuickAdd, params: CliData): Promise<string>
 		});
 	}
 
-	if (action === "get") {
+	if (!debugApi) {
+		return serialize({
+			ok: false,
+			command: CLI_COMMANDS.debug,
+			error: "Debug API is unavailable.",
+		});
+	}
+
+	if (action === DEBUG_ACTIONS.get) {
 		return serialize({
 			ok: true,
 			command: CLI_COMMANDS.debug,
-			action: "get",
-			stats: debugTarget.getDebugStats?.() ?? null,
+			action: DEBUG_ACTIONS.get,
+			stats: debugApi.getStats(),
 		});
 	}
 
-	if (action === "reset") {
-		const reset = debugTarget.resetDebugStats?.() ?? false;
+	if (action === DEBUG_ACTIONS.reset) {
+		debugApi.resetStats();
 		return serialize({
-			ok: reset,
+			ok: true,
 			command: CLI_COMMANDS.debug,
-			action: "reset",
-			stats: debugTarget.getDebugStats?.() ?? null,
+			action: DEBUG_ACTIONS.reset,
+			stats: debugApi.getStats(),
 		});
 	}
 
-	if (action === "flush") {
+	if (action === DEBUG_ACTIONS.flush) {
 		try {
-			const persistence = await debugTarget.flushDebugPersistence?.();
+			const persistence = await debugApi.flushNow();
 			return serialize({
 				ok: true,
 				command: CLI_COMMANDS.debug,
-				action: "flush",
+				action: DEBUG_ACTIONS.flush,
 				persistence: persistence ?? null,
 			});
 		} catch (error) {
 			return serialize({
 				ok: false,
 				command: CLI_COMMANDS.debug,
-				action: "flush",
-				error: error instanceof Error ? error.message : String(error),
+				action: DEBUG_ACTIONS.flush,
+				error: toError(error).message,
 			});
 		}
 	}
 
 	return serialize({
 		ok: false,
 		command: CLI_COMMANDS.debug,
-		error: `Invalid action '${actionRaw}'. Use get, reset, or flush.`,
+		error: `Invalid action '${actionRaw ?? ""}'. Use get, reset, or flush.`,
 	});
 }