Skip to content

Commit ae0f7a1

Browse files
chhoumannchhoumann
authored
fix(gui): reduce ai settings modal reload churn (#1134)
1 parent 9373144 commit ae0f7a1

3 files changed

Lines changed: 66 additions & 27 deletions

File tree

‎plans/issue-1130-fsm-modal-reload.md‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ The user-visible proof is simple: in a dev modal that currently jumps (the exist
1818
- [x] (2026-03-05 06:15Z) Integrated reload controller into scoped modals (`ChoiceBuilder`, Macro settings modals, AI settings modals) and replaced direct full-refresh `reload()` implementations.
1919
- [x] (2026-03-05 06:16Z) Verified with `bun run test`, `bun run build`, `bun run build-with-lint`, and Obsidian CLI runtime probes in `vault=dev`.
2020
- [x] (2026-03-05 06:44Z) Hardened reload queue handling by replacing recursive pending-reload replay with iterative draining and added two regression tests for re-entrant/coalesced reload requests.
21+
- [x] (2026-03-05 07:56Z) Implemented phase-2 in-place UI updates for AI command settings modals so `Show advanced settings` no longer triggers full modal reload, and removed non-infinite model-change/name-edit reloads by refreshing UI elements directly.
2122

2223
## Surprises & Discoveries
2324

@@ -39,6 +40,9 @@ The user-visible proof is simple: in a dev modal that currently jumps (the exist
3940
- Observation: `requestReload()` can be called re-entrantly during `render()`, so pending replay should avoid recursion.
4041
Evidence: added tests where render triggers queued reload(s), asserting two render passes and latest-reason coalescing.
4142

43+
- Observation: `Show advanced settings` can be updated as a local section re-render without changing surrounding settings rows.
44+
Evidence: CLI probe in `quickadd:testQuickAdd` showed `scrollTop` remained `220` and active element stayed `TEXTAREA` while modal height increased after toggle.
45+
4246
## Decision Log
4347

4448
- Decision: implement an explicit finite state machine (FSM) for modal reload lifecycle instead of direct `contentEl.empty(); display();` calls.
@@ -61,6 +65,10 @@ The user-visible proof is simple: in a dev modal that currently jumps (the exist
6165
Rationale: iterative draining preserves behavior while removing recursive call chains during re-entrant reload scenarios.
6266
Date/Author: 2026-03-05 / Codex
6367

68+
- Decision: convert advanced-settings visibility in AI command settings modals from full reload to in-place section rendering.
69+
Rationale: this is a high-frequency interaction and does not require rebuilding unrelated controls; local re-render removes unnecessary modal churn.
70+
Date/Author: 2026-03-05 / Codex
71+
6472
## Outcomes & Retrospective
6573

6674
Implemented outcome matches purpose: reload-driven modal jumps are now handled through an FSM-based controller that captures and restores UI position. Scoped modal classes no longer call direct full-refresh reload logic without restoration.
@@ -72,8 +80,9 @@ Validation results:
7280
- `bun run build-with-lint`: passed.
7381
- Obsidian CLI runtime probe (`vault=dev`) confirms preserved scroll/focus on model-change reload in the test modal.
7482
- Post-merge hardening: `modalReloadMachine` now has 6 passing tests, including queued and coalesced reload behavior during render re-entrancy.
83+
- Phase-2 follow-up: `Show advanced settings` in both AI command settings modals now updates in place (no full reload); CLI probe confirms stable scroll/focus with non-zero scroll range.
7584

76-
Remaining gap: this change stabilizes reload behavior, but does not yet eliminate reloads entirely. Follow-up work can convert high-churn sections to fine-grained updates to reduce rerenders further.
85+
Remaining gap: this change reduces reload frequency in AI command settings flows but does not eliminate reloads across all modal classes. Further follow-up can convert additional conditional UI paths (for example `CaptureChoiceBuilder`, `TemplateChoiceBuilder`, and provider/model editors) to fine-grained section updates.
7786

7887
## Context and Orientation
7988

@@ -274,3 +283,4 @@ Integration rule for every migrated modal:
274283
Revision Note (2026-03-05): Initial ExecPlan created in response to issue #1130 and user request to pursue an FSM-based solution rather than ad-hoc `reload()` calls.
275284
Revision Note (2026-03-05): Updated after implementation to reflect completed milestones, final validation evidence, and runtime probe adjustments needed to distinguish true preservation from expected scroll clamping.
276285
Revision Note (2026-03-05): Updated after merge with queue-drain hardening and additional controller regression tests for re-entrant reload requests.
286+
Revision Note (2026-03-05): Updated for phase-2 partial migration that removes full reloads from advanced-settings toggles in AI command settings modals and records new CLI evidence.

‎src/gui/MacroGUIs/AIAssistantCommandSettingsModal.ts‎

Lines changed: 30 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,8 @@ export class AIAssistantCommandSettingsModal extends Modal {
2727
private settings: IAIAssistantCommand;
2828
private showAdvancedSettings = false;
2929
private readonly reloadController: ModalReloadController;
30+
private advancedSettingsContainer: HTMLElement | null = null;
31+
private refreshTokenCountText: (() => void) | null = null;
3032

3133
private get systemPromptTokenLength(): number {
3234
if (this.settings.model === "Ask me") return Number.POSITIVE_INFINITY;
@@ -52,7 +54,6 @@ export class AIAssistantCommandSettingsModal extends Modal {
5254
modalEl: this.modalEl,
5355
contentEl: this.contentEl,
5456
render: () => {
55-
this.contentEl.empty();
5657
this.display();
5758
},
5859
});
@@ -62,6 +63,10 @@ export class AIAssistantCommandSettingsModal extends Modal {
6263
}
6364

6465
private display(): void {
66+
this.contentEl.empty();
67+
this.advancedSettingsContainer = null;
68+
this.refreshTokenCountText = null;
69+
6570
const header = this.contentEl.createEl("h2", {
6671
text: `${this.settings.name} Settings`,
6772
});
@@ -81,7 +86,7 @@ export class AIAssistantCommandSettingsModal extends Modal {
8186

8287
if (newName && newName !== this.settings.name) {
8388
this.settings.name = newName;
84-
this.reload();
89+
header.setText(`${this.settings.name} Settings`);
8590
}
8691
} catch {} // no new name, don't need exceptional state for that
8792
});
@@ -92,15 +97,10 @@ export class AIAssistantCommandSettingsModal extends Modal {
9297
this.addOutputVariableNameSetting(this.contentEl);
9398

9499
this.addShowAdvancedSettingsToggle(this.contentEl);
95-
96-
if (this.showAdvancedSettings) {
97-
if (!this.settings.modelParameters)
98-
this.settings.modelParameters = {};
99-
this.addTemperatureSetting(this.contentEl);
100-
this.addTopPSetting(this.contentEl);
101-
this.addFrequencyPenaltySetting(this.contentEl);
102-
this.addPresencePenaltySetting(this.contentEl);
103-
}
100+
this.advancedSettingsContainer = this.contentEl.createDiv(
101+
"qa-ai-advanced-settings"
102+
);
103+
this.renderAdvancedSettingsSection();
104104

105105
this.addSystemPromptSetting(this.contentEl);
106106
}
@@ -158,8 +158,7 @@ export class AIAssistantCommandSettingsModal extends Modal {
158158
dropdown.setValue(this.settings.model);
159159
dropdown.onChange((value) => {
160160
this.settings.model = value;
161-
162-
this.reload();
161+
this.refreshTokenCountText?.();
163162
});
164163
});
165164
}
@@ -229,6 +228,7 @@ export class AIAssistantCommandSettingsModal extends Modal {
229228
: "select a model to calculate"
230229
}`;
231230
}, 50);
231+
this.refreshTokenCountText = () => updateTokenCount();
232232

233233
updateTokenCount();
234234

@@ -247,12 +247,28 @@ export class AIAssistantCommandSettingsModal extends Modal {
247247
.addToggle((toggle) => {
248248
toggle.setValue(this.showAdvancedSettings);
249249
toggle.onChange((value) => {
250+
if (value === this.showAdvancedSettings) return;
250251
this.showAdvancedSettings = value;
251-
this.reload();
252+
this.renderAdvancedSettingsSection();
252253
});
253254
});
254255
}
255256

257+
private renderAdvancedSettingsSection(): void {
258+
if (!this.advancedSettingsContainer) return;
259+
260+
this.advancedSettingsContainer.empty();
261+
if (!this.showAdvancedSettings) return;
262+
if (!this.settings.modelParameters) {
263+
this.settings.modelParameters = {};
264+
}
265+
266+
this.addTemperatureSetting(this.advancedSettingsContainer);
267+
this.addTopPSetting(this.advancedSettingsContainer);
268+
this.addFrequencyPenaltySetting(this.advancedSettingsContainer);
269+
this.addPresencePenaltySetting(this.advancedSettingsContainer);
270+
}
271+
256272
addTemperatureSetting(container: HTMLElement) {
257273
new Setting(container)
258274
.setName("Temperature")

‎src/gui/MacroGUIs/AIAssistantInfiniteCommandSettingsModal.ts‎

Lines changed: 25 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ export class InfiniteAIAssistantCommandSettingsModal extends Modal {
2424
private settings: IInfiniteAIAssistantCommand;
2525
private showAdvancedSettings = false;
2626
private readonly reloadController: ModalReloadController;
27+
private advancedSettingsContainer: HTMLElement | null = null;
2728

2829
private get systemPromptTokenLength(): number {
2930
const model = getModelByName(this.settings.model);
@@ -46,7 +47,6 @@ export class InfiniteAIAssistantCommandSettingsModal extends Modal {
4647
modalEl: this.modalEl,
4748
contentEl: this.contentEl,
4849
render: () => {
49-
this.contentEl.empty();
5050
this.display();
5151
},
5252
});
@@ -57,6 +57,8 @@ export class InfiniteAIAssistantCommandSettingsModal extends Modal {
5757

5858
private display(): void {
5959
this.contentEl.empty();
60+
this.advancedSettingsContainer = null;
61+
6062
const header = this.contentEl.createEl("h2", {
6163
text: `${this.settings.name} Settings`,
6264
});
@@ -76,7 +78,7 @@ export class InfiniteAIAssistantCommandSettingsModal extends Modal {
7678

7779
if (newName && newName !== this.settings.name) {
7880
this.settings.name = newName;
79-
this.reload();
81+
header.setText(`${this.settings.name} Settings`);
8082
}
8183
} catch {} // no new name, don't need exceptional state for that
8284
});
@@ -90,15 +92,10 @@ export class InfiniteAIAssistantCommandSettingsModal extends Modal {
9092
this.addOutputVariableNameSetting(this.contentEl);
9193

9294
this.addShowAdvancedSettingsToggle(this.contentEl);
93-
94-
if (this.showAdvancedSettings) {
95-
if (!this.settings.modelParameters)
96-
this.settings.modelParameters = {};
97-
this.addTemperatureSetting(this.contentEl);
98-
this.addTopPSetting(this.contentEl);
99-
this.addFrequencyPenaltySetting(this.contentEl);
100-
this.addPresencePenaltySetting(this.contentEl);
101-
}
95+
this.advancedSettingsContainer = this.contentEl.createDiv(
96+
"qa-ai-advanced-settings"
97+
);
98+
this.renderAdvancedSettingsSection();
10299

103100
this.addSystemPromptSetting(this.contentEl);
104101
}
@@ -207,12 +204,28 @@ export class InfiniteAIAssistantCommandSettingsModal extends Modal {
207204
.addToggle((toggle) => {
208205
toggle.setValue(this.showAdvancedSettings);
209206
toggle.onChange((value) => {
207+
if (value === this.showAdvancedSettings) return;
210208
this.showAdvancedSettings = value;
211-
this.reload();
209+
this.renderAdvancedSettingsSection();
212210
});
213211
});
214212
}
215213

214+
private renderAdvancedSettingsSection(): void {
215+
if (!this.advancedSettingsContainer) return;
216+
217+
this.advancedSettingsContainer.empty();
218+
if (!this.showAdvancedSettings) return;
219+
if (!this.settings.modelParameters) {
220+
this.settings.modelParameters = {};
221+
}
222+
223+
this.addTemperatureSetting(this.advancedSettingsContainer);
224+
this.addTopPSetting(this.advancedSettingsContainer);
225+
this.addFrequencyPenaltySetting(this.advancedSettingsContainer);
226+
this.addPresencePenaltySetting(this.advancedSettingsContainer);
227+
}
228+
216229
addTemperatureSetting(container: HTMLElement) {
217230
new Setting(container)
218231
.setName("Temperature")

0 commit comments

Comments
 (0)