fix: address Copilot review feedback on watcher and key capture modal
- commit
- 8dba5bf77465abc2dfefcae5a29d0f4b1b550dd2
- author
- brice · · 2026-01-24 14:33 UTC
- parent
- 1b74686
- signature
- GPG Verified
CD88F9E564E0102646FB6077A1A07C7C7C3E349D· arch-gpg Verified for the committer email
2 files · +23 -3
M internal/infrastructure/config/watcher.go +4 -1
| @@ -30,7 +30,10 @@ func (m *Manager) Watch() error { | |||
| 30 | 30 | log.Debug().Msg("skipping reload (triggered by own Save)") | |
| 31 | 31 | m.skipNextReload = false | |
| 32 | 32 | // Still need to sync viper's internal state with the file we just wrote | |
| 33 | - | _ = m.viper.ReadInConfig() | |
| 33 | + | if err := m.viper.ReadInConfig(); err != nil { | |
| 34 | + | log.Warn().Err(err).Msg("failed to sync viper config after Save") | |
| 35 | + | // Continue anyway - in-memory config is correct, just viper internal state is off | |
| 36 | + | } | |
| 34 | 37 | m.notifyCallbacksLocked() | |
| 35 | 38 | return | |
| 36 | 39 | } | |
| @@ -30,7 +30,10 @@ func (m *Manager) Watch() error { | |||||
| 30 | log.Debug().Msg("skipping reload (triggered by own Save)") | 30 | log.Debug().Msg("skipping reload (triggered by own Save)") | ||
| 31 | m.skipNextReload = false | 31 | m.skipNextReload = false | ||
| 32 | // Still need to sync viper's internal state with the file we just wrote | 32 | // Still need to sync viper's internal state with the file we just wrote | ||
| 33 | - | _ = m.viper.ReadInConfig() | 33 | + | if err := m.viper.ReadInConfig(); err != nil { |
| 34 | + | log.Warn().Err(err).Msg("failed to sync viper config after Save") | |||
| 35 | + | // Continue anyway - in-memory config is correct, just viper internal state is off | |||
| 36 | + | } | |||
| 34 | m.notifyCallbacksLocked() | 37 | m.notifyCallbacksLocked() | ||
| 35 | return | 38 | return | ||
| 36 | } | 39 | } | ||
M webui/src/pages/config/KeyCaptureModal.svelte +19 -2
| @@ -30,6 +30,13 @@ | |||
| 30 | 30 | let capturedKey = $state<string | null>(null); | |
| 31 | 31 | let isSaving = $state(false); | |
| 32 | 32 | ||
| 33 | + | // Symbol keys that are already shifted (pressing shift produces these characters) | |
| 34 | + | // Don't add 'shift' modifier for these since they're self-describing | |
| 35 | + | const SHIFTED_SYMBOLS = new Set([ | |
| 36 | + | "{", "}", "(", ")", "<", ">", "!", "@", "#", "$", "%", "^", "&", "*", | |
| 37 | + | "+", "_", ":", '"', "?", "|", "~", | |
| 38 | + | ]); | |
| 39 | + | ||
| 33 | 40 | function handleKeyDown(e: KeyboardEvent) { | |
| 34 | 41 | if (!isCapturing) return; | |
| 35 | 42 | e.preventDefault(); | |
| @@ -38,7 +45,6 @@ | |||
| 38 | 45 | const parts: string[] = []; | |
| 39 | 46 | if (e.ctrlKey) parts.push("ctrl"); | |
| 40 | 47 | if (e.altKey) parts.push("alt"); | |
| 41 | - | if (e.shiftKey) parts.push("shift"); | |
| 42 | 48 | ||
| 43 | 49 | let key = e.key.toLowerCase(); | |
| 44 | 50 | switch (key) { | |
| @@ -77,6 +83,12 @@ | |||
| 77 | 83 | break; | |
| 78 | 84 | } | |
| 79 | 85 | ||
| 86 | + | // Only add shift modifier for letter keys and non-symbol keys | |
| 87 | + | // Symbol keys like { } + etc. are already the shifted result | |
| 88 | + | if (e.shiftKey && !SHIFTED_SYMBOLS.has(e.key)) { | |
| 89 | + | parts.push("shift"); | |
| 90 | + | } | |
| 91 | + | ||
| 80 | 92 | parts.push(key); | |
| 81 | 93 | capturedKey = parts.join("+"); | |
| 82 | 94 | isCapturing = false; | |
| @@ -101,7 +113,12 @@ | |||
| 101 | 113 | keysToSave.push(capturedKey); | |
| 102 | 114 | } | |
| 103 | 115 | isSaving = true; | |
| 104 | - | onSave(keysToSave); | |
| 116 | + | try { | |
| 117 | + | onSave(keysToSave); | |
| 118 | + | } finally { | |
| 119 | + | // Reset isSaving so modal remains dismissible if save fails | |
| 120 | + | isSaving = false; | |
| 121 | + | } | |
| 105 | 122 | } | |
| 106 | 123 | ||
| 107 | 124 | onMount(() => { | |
| @@ -30,6 +30,13 @@ | |||||
| 30 | let capturedKey = $state<string | null>(null); | 30 | let capturedKey = $state<string | null>(null); | ||
| 31 | let isSaving = $state(false); | 31 | let isSaving = $state(false); | ||
| 32 | 32 | ||||
| 33 | + | // Symbol keys that are already shifted (pressing shift produces these characters) | |||
| 34 | + | // Don't add 'shift' modifier for these since they're self-describing | |||
| 35 | + | const SHIFTED_SYMBOLS = new Set([ | |||
| 36 | + | "{", "}", "(", ")", "<", ">", "!", "@", "#", "$", "%", "^", "&", "*", | |||
| 37 | + | "+", "_", ":", '"', "?", "|", "~", | |||
| 38 | + | ]); | |||
| 39 | + | ||||
| 33 | function handleKeyDown(e: KeyboardEvent) { | 40 | function handleKeyDown(e: KeyboardEvent) { | ||
| 34 | if (!isCapturing) return; | 41 | if (!isCapturing) return; | ||
| 35 | e.preventDefault(); | 42 | e.preventDefault(); | ||
| @@ -38,7 +45,6 @@ | |||||
| 38 | const parts: string[] = []; | 45 | const parts: string[] = []; | ||
| 39 | if (e.ctrlKey) parts.push("ctrl"); | 46 | if (e.ctrlKey) parts.push("ctrl"); | ||
| 40 | if (e.altKey) parts.push("alt"); | 47 | if (e.altKey) parts.push("alt"); | ||
| 41 | - | if (e.shiftKey) parts.push("shift"); | |||
| 42 | 48 | ||||
| 43 | let key = e.key.toLowerCase(); | 49 | let key = e.key.toLowerCase(); | ||
| 44 | switch (key) { | 50 | switch (key) { | ||
| @@ -77,6 +83,12 @@ | |||||
| 77 | break; | 83 | break; | ||
| 78 | } | 84 | } | ||
| 79 | 85 | ||||
| 86 | + | // Only add shift modifier for letter keys and non-symbol keys | |||
| 87 | + | // Symbol keys like { } + etc. are already the shifted result | |||
| 88 | + | if (e.shiftKey && !SHIFTED_SYMBOLS.has(e.key)) { | |||
| 89 | + | parts.push("shift"); | |||
| 90 | + | } | |||
| 91 | + | ||||
| 80 | parts.push(key); | 92 | parts.push(key); | ||
| 81 | capturedKey = parts.join("+"); | 93 | capturedKey = parts.join("+"); | ||
| 82 | isCapturing = false; | 94 | isCapturing = false; | ||
| @@ -101,7 +113,12 @@ | |||||
| 101 | keysToSave.push(capturedKey); | 113 | keysToSave.push(capturedKey); | ||
| 102 | } | 114 | } | ||
| 103 | isSaving = true; | 115 | isSaving = true; | ||
| 104 | - | onSave(keysToSave); | 116 | + | try { |
| 117 | + | onSave(keysToSave); | |||
| 118 | + | } finally { | |||
| 119 | + | // Reset isSaving so modal remains dismissible if save fails | |||
| 120 | + | isSaving = false; | |||
| 121 | + | } | |||
| 105 | } | 122 | } | ||
| 106 | 123 | ||||
| 107 | onMount(() => { | 124 | onMount(() => { | ||