From 8048c30bca4d809f7832b41bbfd3895c09f9a62d Mon Sep 17 00:00:00 2001 From: scheinriese Date: Tue, 10 Mar 2026 11:13:45 +0100 Subject: [PATCH] Address pr-review findings: fix listener race condition, lint warnings, and keyboard accessibility MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Replace boolean *global-listener-setup? with refcount *global-listener-refcount to prevent concurrent popovers from breaking global shortcut suppression - Fix unused binding custom? → _custom? in shortcut list - Fix _state → state in cmdk/core.cljs (was actually used, underscore was wrong) - Merge redundant let expression in ui.cljs autocomplete - Remove stale :tiled false prop from cmdk hint - Add tabIndex, role="button", and Enter/Space keyboard activation to shortcut rows Co-Authored-By: Claude Opus 4.6 --- src/main/frontend/components/cmdk/core.cljs | 8 +- src/main/frontend/components/shortcut.cljs | 102 ++++++++++---------- src/main/frontend/ui.cljs | 19 ++-- 3 files changed, 65 insertions(+), 64 deletions(-) diff --git a/src/main/frontend/components/cmdk/core.cljs b/src/main/frontend/components/cmdk/core.cljs index 6e8a44dfcf..5c038cba99 100644 --- a/src/main/frontend/components/cmdk/core.cljs +++ b/src/main/frontend/components/cmdk/core.cljs @@ -378,9 +378,9 @@ files)] (swap! !results update group merge {:status :success :items items})))) -(defmethod load-results :themes [group _state] - (let [!input (::input _state) - !results (::results _state) +(defmethod load-results :themes [group state] + (let [!input (::input state) + !results (::results state) themes (state/sub :plugin/installed-themes) themes (if (string/blank? @!input) themes @@ -1110,7 +1110,7 @@ filter' [:div.flex.flex-row.gap-1.items-center.opacity-50.hover:opacity-100 [:div "Type"] - (shui/shortcut "esc" {:tiled false}) + (shui/shortcut "esc") [:div "to clear search filter"]] :else diff --git a/src/main/frontend/components/shortcut.cljs b/src/main/frontend/components/shortcut.cljs index a761dfc099..f9f30777b6 100644 --- a/src/main/frontend/components/shortcut.cljs +++ b/src/main/frontend/components/shortcut.cljs @@ -30,7 +30,7 @@ (defonce *refresh-sentry (atom 0)) (defn refresh-shortcuts-list! [] (reset! *refresh-sentry (inc @*refresh-sentry))) -(defonce *global-listener-setup? (atom false)) +(defonce *global-listener-refcount (atom 0)) (defn- to-vector [v] (when-not (nil? v) @@ -87,13 +87,9 @@ (let [^js el (rum/deref *ref-el) key-handler (KeyHandler. el) - teardown-global! - (when-not @*global-listener-setup? - (shortcut/unlisten-all! true) - (reset! *global-listener-setup? true) - (fn [] - (shortcut/listen-all!) - (reset! *global-listener-setup? false)))] + _ (when (zero? @*global-listener-refcount) + (shortcut/unlisten-all! true)) + _ (swap! *global-listener-refcount inc)] (events/listen key-handler "key" (fn [^js e] @@ -155,7 +151,8 @@ #(do (when-let [timer (rum/deref *commit-timer)] (js/clearTimeout timer)) - (some-> teardown-global! (apply nil)) + (when (zero? (swap! *global-listener-refcount dec)) + (shortcut/listen-all!)) (.dispose key-handler)))) []) @@ -547,13 +544,9 @@ (let [^js el (rum/deref *ref-el) key-handler (KeyHandler. el) - teardown-global! - (when-not @*global-listener-setup? - (shortcut/unlisten-all! true) - (reset! *global-listener-setup? true) - (fn [] - (shortcut/listen-all!) - (reset! *global-listener-setup? false)))] + _ (when (zero? @*global-listener-refcount) + (shortcut/unlisten-all! true)) + _ (swap! *global-listener-refcount inc)] (events/listen key-handler "key" (fn [^js e] @@ -632,7 +625,8 @@ (js/clearTimeout timer)) (when-let [timer (rum/deref *fade-timer)] (js/clearTimeout timer)) - (some-> teardown-global! (apply nil)) + (when (zero? (swap! *global-listener-refcount dec)) + (shortcut/listen-all!)) (.dispose key-handler)))) []) @@ -930,7 +924,7 @@ user-binding (and user-binding (to-vector user-binding)) label (shortcut-desc-label id m) cats (classify-shortcut m) - custom? (contains? cats :Custom) + _custom? (contains? cats :Custom) disabled? (contains? cats :Disabled) unset? (contains? cats :Unset)]] @@ -949,39 +943,47 @@ (and (sequential? s) (sequential? keystroke') (apply = (map first [s keystroke']))))) binding'))))) - [:li.shortcut-row.flex.items-start.justify-between.text-sm - {:key (str id) - :class (when (= active-id id) "active") - :on-click (when (and id (not disabled?)) - (fn [^js e] - (if (= active-id id) - (let [popup-id (keyword (str "customize-shortcut-" (name id)))] - (reset! *active-shortcut-id nil) - (shui/popup-hide! popup-id)) - (let [anchor-el (-> (.-currentTarget e) (.querySelector ".action-wrap"))] - (open-customize-shortcut-dialog! anchor-el id)))))} - [:span.label-wrap label] + (let [row-action (when (and id (not disabled?)) + (fn [^js e] + (if (= active-id id) + (let [popup-id (keyword (str "customize-shortcut-" (name id)))] + (reset! *active-shortcut-id nil) + (shui/popup-hide! popup-id)) + (let [anchor-el (-> (.-currentTarget e) (.querySelector ".action-wrap"))] + (open-customize-shortcut-dialog! anchor-el id)))))] + [:li.shortcut-row.flex.items-start.justify-between.text-sm + {:key (str id) + :class (when (= active-id id) "active") + :tab-index (when (and id (not disabled?)) 0) + :role (when (and id (not disabled?)) "button") + :on-click row-action + :on-key-down (when row-action + (fn [^js e] + (when (contains? #{13 32} (.-keyCode e)) + (.preventDefault e) + (row-action e))))} + [:span.label-wrap label] - [:span.action-wrap - {:class (util/classnames [{:disabled disabled?}])} + [:span.action-wrap + {:class (util/classnames [{:disabled disabled?}])} - (cond - unset? - [:span.shortcut-status-label (t :keymap/unset)] + (cond + unset? + [:span.shortcut-status-label (t :keymap/unset)] - (or user-binding (false? user-binding)) - [:<> - [:span.shortcut-status-label (str (t :keymap/custom) ":")] - (if disabled? - [:span.shortcut-status-label (t :keymap/disabled)] - (for [b user-binding - :when (string? b)] - [:span {:key b} - (shui/shortcut b)]))] + (or user-binding (false? user-binding)) + [:<> + [:span.shortcut-status-label (str (t :keymap/custom) ":")] + (if disabled? + [:span.shortcut-status-label (t :keymap/disabled)] + (for [b user-binding + :when (string? b)] + [:span {:key b} + (shui/shortcut b)]))] - :else - (for [b binding - :when (string? b)] - [:span {:key b} - (shui/shortcut (dh/binding-for-display id b) - {:raw-binding [b]})]))]]))))])])]])) + :else + (for [b binding + :when (string? b)] + [:span {:key b} + (shui/shortcut (dh/binding-for-display id b) + {:raw-binding [b]})]))]])))))])])]])) diff --git a/src/main/frontend/ui.cljs b/src/main/frontend/ui.cljs index fb00c6b003..7e09115c48 100644 --- a/src/main/frontend/ui.cljs +++ b/src/main/frontend/ui.cljs @@ -538,16 +538,15 @@ (if (and (gobj/get e "shiftKey") on-shift-chosen) (on-shift-chosen item) (on-chosen item e))))} - (if item-render (item-render item chosen?) item)))]] - - (let [group-name (and (fn? get-group-name) (get-group-name item))] - (if (and group-name (not (contains? @*groups group-name))) - (do - (swap! *groups conj group-name) - [:div - [:div.ui__ac-group-name group-name] - item-cp]) - item-cp)))))] + (if item-render (item-render item chosen?) item)))] + group-name (and (fn? get-group-name) (get-group-name item))] + (if (and group-name (not (contains? @*groups group-name))) + (do + (swap! *groups conj group-name) + [:div + [:div.ui__ac-group-name group-name] + item-cp]) + item-cp))))] [:div#ui__ac {:class class} (if (seq matched) [:div#ui__ac-inner.hide-scrollbar