fix: address review findings for keybinding presets

- Delay setting currentPresetName until after successful preset load
- Prompt save-as when switching from modified default preset
- Trim and validate preset name before duplicate check in saveAsNewPreset
- Reject reserved name "default" in presetFilePath
- Convert onPresetsChanged callback prop to emit in KeybindingPresetToolbar
- Add tests for savePreset failure and "default" name rejection
This commit is contained in:
Johnpaul
2026-03-10 00:55:09 +01:00
parent 4b0a2668d6
commit b0171dd6c5
4 changed files with 56 additions and 11 deletions

View File

@@ -20,7 +20,7 @@
<KeybindingPresetToolbar
:preset-names="presetNames"
:on-presets-changed="refreshPresetList"
@presets-changed="refreshPresetList"
/>
<DataTable
@@ -201,11 +201,14 @@ async function refreshPresetList() {
async function initPresets() {
await refreshPresetList()
const currentName = settingStore.get('Comfy.Keybinding.CurrentPreset')
keybindingStore.currentPresetName = currentName
if (currentName !== 'default') {
const preset = await presetService.loadPreset(currentName)
if (preset) {
keybindingStore.savedPresetData = preset
keybindingStore.currentPresetName = currentName
} else {
keybindingStore.currentPresetName = 'default'
keybindingStore.savedPresetData = null
}
}
}
@@ -220,15 +223,19 @@ async function saveAsNewPreset() {
defaultValue: ''
})
if (!name) return
if (presetNames.value.includes(name)) {
const trimmedName = name.trim()
if (!trimmedName) return
if (presetNames.value.includes(trimmedName)) {
const overwrite = await dialogService.confirm({
title: t('g.keybindingPresets.overwritePresetTitle'),
message: t('g.keybindingPresets.overwritePresetMessage', { name }),
message: t('g.keybindingPresets.overwritePresetMessage', {
name: trimmedName
}),
type: 'overwrite'
})
if (!overwrite) return
}
await presetService.savePreset(name)
await presetService.savePreset(trimmedName)
refreshPresetList()
}

View File

@@ -61,9 +61,12 @@ import SelectValue from '@/components/ui/select/SelectValue.vue'
import { useKeybindingPresetService } from '@/platform/keybindings/presetService'
import { useKeybindingStore } from '@/platform/keybindings/keybindingStore'
const { presetNames, onPresetsChanged } = defineProps<{
const { presetNames } = defineProps<{
presetNames: string[]
onPresetsChanged: () => void
}>()
const emit = defineEmits<{
'presets-changed': []
}>()
const { t } = useI18n()
@@ -84,7 +87,7 @@ watch(selectedPreset, async (newValue) => {
if (newValue !== keybindingStore.currentPresetName) {
await presetService.switchPreset(newValue)
selectedPreset.value = keybindingStore.currentPresetName
onPresetsChanged()
emit('presets-changed')
}
})
@@ -107,6 +110,6 @@ async function handleSavePreset() {
async function handleImportFromDropdown() {
await presetService.importPreset()
onPresetsChanged()
emit('presets-changed')
}
</script>

View File

@@ -141,6 +141,25 @@ describe('useKeybindingPresetService', () => {
)
expect(store.currentPresetName).toBe('my-preset')
})
it('does not update store when storeUserData rejects', async () => {
mockApi.storeUserData.mockRejectedValue(new Error('Server error'))
const keybinding = new KeybindingImpl({
commandId: 'test.cmd',
combo: { key: 'A', ctrl: true }
})
store.addUserKeybinding(keybinding)
store.currentPresetName = 'old-preset'
const service = await getPresetService()
await expect(service.savePreset('my-preset')).rejects.toThrow(
'Server error'
)
expect(store.currentPresetName).toBe('old-preset')
expect(store.savedPresetData).toBeNull()
})
})
describe('deletePreset', () => {
@@ -242,6 +261,11 @@ describe('useKeybindingPresetService', () => {
await expect(service.savePreset('.hidden')).rejects.toThrow()
})
it('rejects the reserved name "default"', async () => {
const service = await getPresetService()
await expect(service.savePreset('default')).rejects.toThrow()
})
it('rejects empty names', async () => {
const service = await getPresetService()
await expect(service.savePreset('')).rejects.toThrow()

View File

@@ -22,6 +22,7 @@ function presetFilePath(name: string): string {
const trimmed = name.trim()
if (
!trimmed ||
trimmed === 'default' ||
trimmed.includes('/') ||
trimmed.includes('\\') ||
trimmed.includes('..') ||
@@ -179,8 +180,18 @@ export function useKeybindingPresetService() {
})
if (result === null) return
if (result && keybindingStore.currentPresetName !== 'default') {
await savePreset(keybindingStore.currentPresetName)
if (result) {
if (keybindingStore.currentPresetName !== 'default') {
await savePreset(keybindingStore.currentPresetName)
} else {
const name = await dialogService.prompt({
title: t('g.keybindingPresets.saveAsNewPreset'),
message: t('g.keybindingPresets.presetNamePrompt'),
defaultValue: ''
})
if (!name) return
await savePreset(name.trim())
}
}
}