mirror of
https://github.com/Ark0N/Codeman.git
synced 2026-10-11 09:49:41 +02:00
fix(settings): Apply cannot keep a later Save open, refreshes only after a saved PUT (review)
- Split the one-shot keep-open intent from the in-flight guard and consume it before the first await, so a Save clicked while an Apply is in flight closes the modal. - Refresh the dependent groups only when the settings PUT returned ok (_apiPut answers null or a non-ok response instead of throwing), and also when only the webhook save failed. - Find the 'Apply or Save' hint by a data marker, not its English text; add zh-CN strings for the new toast and title. - Tests run the real saveAppSettings() (Apply then Save mid-flight, a failed PUT, a webhook-only failure, a plain Save). - Narrow the changeset and JSDoc to MCP sync and CLI management; touch the tray comment, the architecture note and the wiki.
This commit is contained in:
@@ -32,15 +32,16 @@ describe('Settings Apply button', () => {
|
||||
expect(modal.match(/onclick="app\.saveAppSettings\(\)"/g)).toHaveLength(2);
|
||||
});
|
||||
|
||||
it('flags the save as keep-open while it runs and clears the flag afterwards', async () => {
|
||||
it('raises the keep-open intent for the save it wraps and clears it afterwards', async () => {
|
||||
const app = loadApp();
|
||||
const seen: boolean[] = [];
|
||||
app.saveAppSettings = vi.fn(async () => {
|
||||
seen.push(app._applyingSettings);
|
||||
seen.push(app._keepSettingsOpenOnce);
|
||||
});
|
||||
await app.applyAppSettings();
|
||||
expect(seen).toEqual([true]);
|
||||
expect(app._applyingSettings).toBe(false);
|
||||
expect(app._keepSettingsOpenOnce).toBe(false);
|
||||
expect(app._applyInFlight).toBe(false);
|
||||
});
|
||||
|
||||
it('clears the flag even when the save throws, and ignores a second click mid-save', async () => {
|
||||
@@ -57,13 +58,15 @@ describe('Settings Apply button', () => {
|
||||
throw new Error('boom');
|
||||
});
|
||||
await expect(app.applyAppSettings()).rejects.toThrow('boom');
|
||||
expect(app._applyingSettings).toBe(false);
|
||||
expect(app._applyInFlight).toBe(false);
|
||||
expect(app._keepSettingsOpenOnce).toBe(false);
|
||||
});
|
||||
|
||||
it('refreshes the dependent groups from the saved values', () => {
|
||||
const app = loadApp();
|
||||
const out = {
|
||||
textContent: 'Apply or Save settings to turn MCP sync on first',
|
||||
textContent: 'Anything: the hint is found by its marker, not its (translatable) text',
|
||||
dataset: { hint: 'save-first' },
|
||||
style: { display: 'block' },
|
||||
innerHTML: 'x',
|
||||
};
|
||||
@@ -83,11 +86,119 @@ describe('Settings Apply button', () => {
|
||||
expect(app.applyCliManagementVisibility).toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('only closes the modal on a plain Save', () => {
|
||||
const body = settingsUi.slice(settingsUi.indexOf('\n async saveAppSettings() {'));
|
||||
expect(body).toContain('const keepOpen = this._applyingSettings === true;');
|
||||
expect(body).toMatch(
|
||||
/else if \(keepOpen\) \{\s*this\._refreshSettingsAfterApply\(settings\);\s*\} else \{\s*this\.closeAppSettings\(\);/
|
||||
);
|
||||
describe('the real saveAppSettings()', () => {
|
||||
/** An element that answers any property read, call or write: enough for a DOM-heavy function to run. */
|
||||
function genericElement(): any {
|
||||
const el: any = new Proxy(function () {}, {
|
||||
get: (_t, prop) => {
|
||||
if (prop === 'value') return '';
|
||||
if (prop === 'checked') return false;
|
||||
if (prop === Symbol.toPrimitive) return () => '';
|
||||
if (prop === 'then') return undefined;
|
||||
return el;
|
||||
},
|
||||
set: () => true,
|
||||
apply: () => el,
|
||||
});
|
||||
return el;
|
||||
}
|
||||
|
||||
function loadRealApp(put: () => Promise<unknown>) {
|
||||
const CodemanApp = function CodemanApp() {} as unknown as { prototype: Record<string, unknown> };
|
||||
const el = genericElement();
|
||||
const context = vm.createContext({
|
||||
CodemanApp,
|
||||
localStorage: { getItem: () => null, setItem: () => {} },
|
||||
document: new Proxy(
|
||||
{
|
||||
getElementById: () => el,
|
||||
querySelectorAll: () => [],
|
||||
querySelector: () => el,
|
||||
addEventListener: () => {},
|
||||
body: el,
|
||||
documentElement: el,
|
||||
},
|
||||
{ get: (t: any, prop: string) => (prop in t ? t[prop] : el) }
|
||||
),
|
||||
window: {},
|
||||
location: { reload: vi.fn() },
|
||||
VoiceInput: new Proxy({}, { get: () => vi.fn(() => ({})) }),
|
||||
KeyboardAccessoryBar: new Proxy({}, { get: () => vi.fn(() => ({})) }),
|
||||
MobileDetection: new Proxy({}, { get: () => vi.fn(() => false) }),
|
||||
setTimeout,
|
||||
console,
|
||||
});
|
||||
vm.runInContext(settingsUi, context, { filename: 'settings-ui.js' });
|
||||
const target = new (CodemanApp as unknown as new () => Record<string, any>)();
|
||||
// saveAppSettings() leans on helpers from the other settings mixins; anything it asks for
|
||||
// that is not defined is a harmless stub, while the state the Apply logic owns stays real.
|
||||
const state = new Set(['_keepSettingsOpenOnce', '_applyInFlight', '_mcpSyncSavedOn']);
|
||||
const app: Record<string, any> = new Proxy(target, {
|
||||
get: (t, prop) =>
|
||||
prop in t || typeof prop !== 'string' || state.has(prop) || prop === 'then'
|
||||
? t[prop as string]
|
||||
: vi.fn(() => 14),
|
||||
});
|
||||
app.loadAppSettingsFromStorage = () => ({});
|
||||
app.saveAppSettingsToStorage = vi.fn();
|
||||
app.notificationManager = new Proxy(
|
||||
{ preferences: {}, normalizePreferences: (p: unknown) => p, getToastDurationMs: () => 3000 },
|
||||
{ get: (t: any, prop: string) => (prop in t ? t[prop] : vi.fn()) }
|
||||
);
|
||||
app._apiPut = vi.fn(put);
|
||||
app._handleTunnelEnableRefusal = vi.fn(async () => false);
|
||||
app.saveModelConfigFromSettings = vi.fn(async () => {});
|
||||
app._webhookPending = () => false;
|
||||
app.showToast = vi.fn();
|
||||
app.closeAppSettings = vi.fn();
|
||||
app._refreshSettingsAfterApply = vi.fn();
|
||||
// The apply*() helpers re-skin and re-lay-out the page from real DOM geometry; that is not what
|
||||
// these tests are about, so they are stubbed. applyAppSettings() itself stays real.
|
||||
for (const name of Object.keys(CodemanApp.prototype)) {
|
||||
if (/^apply[A-Z]/.test(name) && name !== 'applyAppSettings') app[name] = vi.fn();
|
||||
}
|
||||
return app;
|
||||
}
|
||||
|
||||
it('a Save clicked while an Apply is in flight closes the modal, and the Apply keeps it open', async () => {
|
||||
let resolvePut!: (v: unknown) => void;
|
||||
const first = new Promise((r) => (resolvePut = r));
|
||||
const app = loadRealApp(() => first);
|
||||
const apply = app.applyAppSettings();
|
||||
// The Apply's PUT is still pending; the user clicks Save now.
|
||||
const save = app.saveAppSettings();
|
||||
resolvePut({ ok: true });
|
||||
await Promise.all([apply, save]);
|
||||
expect(app.closeAppSettings).toHaveBeenCalledTimes(1);
|
||||
expect(app._refreshSettingsAfterApply).toHaveBeenCalledTimes(1);
|
||||
const toasts = app.showToast.mock.calls.map((c: unknown[]) => c[0]);
|
||||
expect(toasts).toContain('Settings applied');
|
||||
expect(toasts).toContain('Settings saved');
|
||||
});
|
||||
|
||||
it('does not refresh the dependent groups when the settings PUT failed', async () => {
|
||||
for (const failure of [{ ok: false }, null]) {
|
||||
const app = loadRealApp(async () => failure);
|
||||
await app.applyAppSettings();
|
||||
expect(app._refreshSettingsAfterApply, JSON.stringify(failure)).not.toHaveBeenCalled();
|
||||
expect(app.closeAppSettings).not.toHaveBeenCalled();
|
||||
}
|
||||
});
|
||||
|
||||
it('still refreshes when only the webhook failed (the rest was saved)', async () => {
|
||||
const app = loadRealApp(async () => ({ ok: true }));
|
||||
app._webhookPending = () => true;
|
||||
app.saveWebhook = vi.fn(async () => 'bad URL');
|
||||
await app.applyAppSettings();
|
||||
expect(app._refreshSettingsAfterApply).toHaveBeenCalledTimes(1);
|
||||
expect(app.closeAppSettings).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('a plain Save closes the modal and never refreshes in place', async () => {
|
||||
const app = loadRealApp(async () => ({ ok: true }));
|
||||
await app.saveAppSettings();
|
||||
expect(app.closeAppSettings).toHaveBeenCalledTimes(1);
|
||||
expect(app._refreshSettingsAfterApply).not.toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user