diff --git a/src/components/editor/FormatterSettingsDialog.tsx b/src/components/editor/FormatterSettingsDialog.tsx index 795db67..1734402 100644 --- a/src/components/editor/FormatterSettingsDialog.tsx +++ b/src/components/editor/FormatterSettingsDialog.tsx @@ -1,5 +1,5 @@ import { Settings2 } from "lucide-react"; -import { useEffect, useState } from "react"; +import { useEffect, useMemo, useState } from "react"; import { type FormatterSettings, useSettingsStore } from "../../stores/settingsStore"; interface FormatterSettingsDialogProps { @@ -24,6 +24,14 @@ const DEFAULTS: FormatterSettings = { type CaseOption = "upper" | "lower" | "preserve"; +function shallowEqual(a: FormatterSettings, b: FormatterSettings): boolean { + const keys = Object.keys(a) as (keyof FormatterSettings)[]; + for (const k of keys) { + if (a[k] !== b[k]) return false; + } + return true; +} + export function FormatterSettingsDialog({ isOpen, onClose }: FormatterSettingsDialogProps) { const { formatterSettings, setFormatterSettings } = useSettingsStore(); // Always merge with defaults so new fields are never undefined @@ -34,9 +42,21 @@ export function FormatterSettingsDialog({ isOpen, onClose }: FormatterSettingsDi if (isOpen) setLocal({ ...DEFAULTS, ...formatterSettings }); }, [isOpen]); // eslint-disable-line react-hooks/exhaustive-deps + // Dirty = any field differs from the persisted settings. Saves a no-op + // localStorage round-trip when the user opens the dialog and clicks Save + // without changing anything. (refs GH-#349) + const isDirty = useMemo( + () => !shallowEqual(local, formatterSettings), + [local, formatterSettings], + ); + if (!isOpen) return null; const handleSave = () => { + if (!isDirty) { + onClose(); + return; + } setFormatterSettings(local); onClose(); }; @@ -88,6 +108,13 @@ export function FormatterSettingsDialog({ isOpen, onClose }: FormatterSettingsDi

SQL Formatter Settings + {isDirty && ( + + )}

@@ -236,7 +263,8 @@ export function FormatterSettingsDialog({ isOpen, onClose }: FormatterSettingsDi diff --git a/src/components/editor/__tests__/FormatterSettingsDialog.test.tsx b/src/components/editor/__tests__/FormatterSettingsDialog.test.tsx index 25cac7b..a164d72 100644 --- a/src/components/editor/__tests__/FormatterSettingsDialog.test.tsx +++ b/src/components/editor/__tests__/FormatterSettingsDialog.test.tsx @@ -100,11 +100,68 @@ describe("FormatterSettingsDialog", () => { const onClose = vi.fn(); render(); - fireEvent.click(screen.getByText("Save")); + // Save starts disabled because local matches persisted defaults; toggle the + // Keywords select (first combobox on the page) to make the dialog dirty. + const keywordsSelect = screen.getAllByRole("combobox")[0] as HTMLSelectElement; + fireEvent.change(keywordsSelect, { target: { value: "lower" } }); + + const saveButton = screen.getByText("Save").closest("button")!; + expect(saveButton).not.toBeDisabled(); + + fireEvent.click(saveButton); expect(onClose).toHaveBeenCalledTimes(1); const state = useSettingsStore.getState(); - expect(state.formatterSettings.keywordCase).toBe("upper"); + expect(state.formatterSettings.keywordCase).toBe("lower"); + }); + + it("Save button is disabled when settings are unchanged from persisted state", () => { + render(); + + const saveButton = screen.getByText("Save").closest("button")!; + expect(saveButton).toBeDisabled(); + // No dirty dot in title + expect(screen.queryByLabelText("unsaved changes")).toBeNull(); + }); + + it("Save button becomes enabled after a setting changes and shows a dirty dot", () => { + render(); + + const keywordsSelect = screen.getAllByRole("combobox")[0] as HTMLSelectElement; + fireEvent.change(keywordsSelect, { target: { value: "lower" } }); + + const saveButton = screen.getByText("Save").closest("button")!; + expect(saveButton).not.toBeDisabled(); + // Dirty indicator appears + expect(screen.getByLabelText("unsaved changes")).toBeInTheDocument(); + }); + + it("clicking Save with no changes does not overwrite persisted state", () => { + // Start with a non-default persisted state so we can detect spurious writes + useSettingsStore.setState({ + formatterSettings: { + ...useSettingsStore.getState().formatterSettings, + keywordCase: "lower", + tabWidth: 4, + }, + }); + + const onClose = vi.fn(); + render(); + + const saveButton = screen.getByText("Save").closest("button")!; + expect(saveButton).toBeDisabled(); + + // Note: fireEvent.click on a disabled button is a no-op in JSDOM. Verify + // by checking the persisted state is untouched (the dialog guarantees no + // write happens when not dirty, so any spurious round-trip would change + // these values back to the dialog's "defaults" view of them). + fireEvent.click(saveButton); + + expect(onClose).not.toHaveBeenCalled(); + const state = useSettingsStore.getState(); + expect(state.formatterSettings.keywordCase).toBe("lower"); + expect(state.formatterSettings.tabWidth).toBe(4); }); it("resets to defaults when Reset to defaults is clicked", () => { @@ -112,12 +169,10 @@ describe("FormatterSettingsDialog", () => { fireEvent.click(screen.getByText("Reset to defaults")); + // After Reset, local state equals defaults (which already match the persisted + // state from beforeEach), so Save is still disabled. To exercise Save, we + // first change a field, then Reset, then change it again. const state = useSettingsStore.getState(); - // After clicking reset, the local state is set to defaults - // But the store isn't updated until Save is clicked - // Let's verify the button exists and we can save - fireEvent.click(screen.getByText("Save")); - // Check the store was updated with defaults expect(state.formatterSettings.tabWidth).toBe(2); });