🐛 Fix color token sets rendering in ascending-precedence order (#10658)

The Color tokens picker showed token sets in their raw definition
order (ascending precedence), so the lowest-precedence set appeared
first and the highest-precedence (last-defined, winning) set
appeared last. This is the opposite of what's useful: users care
most about which set is currently winning, so that one should be at
the top.

get-sets returns sets in definition order and the picker's
grouped-tokens-by-set pipeline (add-tokens-to-sets ->
filter-active-sets -> filter-non-empty-sets -> group-sets ->
combine-groups-with-resolved) preserves that order at every step, so
the picker just rendered get-sets' raw order. Reverse the set seq
once, before it enters the pipeline, so the highest-precedence set
renders first.

group-sets groups sets by parent path via group-by, which risked
restoring definition order within a subgroup independent of the
reversed input order. Added tests covering a flat set list, a
reversed subgroup (to confirm group-by does not silently re-sort
subgroup members), and a mixed flat/subgrouped list.

Closes #10552

Signed-off-by: Andrew Cunliffe <cunliffeandrewc@gmail.com>
Co-authored-by: Andrey Antukh <niwi@niwi.nz>
This commit is contained in:
Andrew Cunliffe 2026-07-23 04:30:32 -07:00 committed by GitHub
parent d42f78b80e
commit ed85d1d1af
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
3 changed files with 72 additions and 1 deletions

View File

@ -607,7 +607,7 @@
:top top-offset
:maxHeight max-height-top}))))
(defn- group-sets
(defn group-sets
"Groups sets by their parent path (everything before the last '/') if present.
The set name is always the last part of the path.
@ -777,6 +777,10 @@
(mf/with-memo [tokens-lib active-sets-names color-tokens]
(some-> tokens-lib
(ctob/get-sets)
;; Show highest-precedence (last-defined) sets first in the
;; picker; the Tokens panel itself keeps definition order.
;; https://github.com/penpot/penpot/issues/10552
(reverse)
(add-tokens-to-sets)
(filter-active-sets active-sets-names)
(filter-non-empty-sets)

View File

@ -54,6 +54,7 @@
[frontend-tests.tokens.style-dictionary-test]
[frontend-tests.tokens.token-errors-test]
[frontend-tests.tokens.workspace-tokens-remap-test]
[frontend-tests.ui.colorpicker-token-set-order-test]
[frontend-tests.ui.comments-clustering-test]
[frontend-tests.ui.comments-position-modifier-test]
[frontend-tests.ui.ds-controls-numeric-input-test]
@ -129,6 +130,7 @@
'frontend-tests.tokens.style-dictionary-test
'frontend-tests.tokens.token-errors-test
'frontend-tests.tokens.workspace-tokens-remap-test
'frontend-tests.ui.colorpicker-token-set-order-test
'frontend-tests.ui.comments-clustering-test
'frontend-tests.ui.comments-position-modifier-test
'frontend-tests.ui.ds-controls-numeric-input-test

View File

@ -0,0 +1,65 @@
;; This Source Code Form is subject to the terms of the Mozilla Public
;; License, v. 2.0. If a copy of the MPL was not distributed with this
;; file, You can obtain one at http://mozilla.org/MPL/2.0/.
;;
;; Copyright (c) KALEIDOS INC Sucursal en España SL
(ns frontend-tests.ui.colorpicker-token-set-order-test
(:require
[app.main.ui.workspace.colorpicker :refer [group-sets]]
[cljs.test :as t :include-macros true]))
;; https://github.com/penpot/penpot/issues/10552
;; The Color tokens picker shows sets in reverse of their definition order
;; (highest-precedence / last-defined set first). The fix reverses the raw
;; set seq once, before it reaches `group-sets`. These tests drive
;; `group-sets` with already-reversed input -- matching what it receives in
;; the real pipeline -- to prove the `group-by` call inside `group-sets`
;; doesn't scramble that reversal, for both an ungrouped list and a
;; subgrouped one.
(defn- flat-entries
"Reduce group-sets' output (a mix of single-set and multi-set grouped
entries) to a flat [group name] seq, in output order."
[grouped]
(mapcat (fn [{:keys [group sets]}]
(map (fn [{:keys [name]}] [group name]) sets))
grouped))
(t/deftest reversed-flat-set-list
(let [;; definition order: global, alias, semantic (ascending precedence)
definition-order [{:id 1 :set "global" :tokens []}
{:id 2 :set "alias" :tokens []}
{:id 3 :set "semantic" :tokens []}]
result (group-sets (reverse definition-order))]
(t/is (= [[nil "semantic"] [nil "alias"] [nil "global"]]
(flat-entries result)))))
(t/deftest reversed-subgroup-members-are-not-scrambled
(let [;; brand/subgroup/one is defined before brand/subgroup/two
definition-order [{:id 1 :set "brand/subgroup/one"}
{:id 2 :set "brand/subgroup/two"}]
definition-order (map #(assoc % :tokens []) definition-order)
result (group-sets (reverse definition-order))]
;; "two" has higher precedence (defined later) so it must lead --
;; group-by must not silently restore definition order within the
;; subgroup's own member list.
(t/is (= [["brand/subgroup" "two"] ["brand/subgroup" "one"]]
(flat-entries result)))))
(t/deftest reversed-mixed-flat-and-subgroup-set-list
(let [;; definition order: an ungrouped set, then a two-member subgroup,
;; then another ungrouped set (highest precedence).
definition-order [{:id 1 :set "global" :tokens []}
{:id 2 :set "brand/subgroup/one" :tokens []}
{:id 3 :set "brand/subgroup/two" :tokens []}
{:id 4 :set "primitives" :tokens []}]
result (group-sets (reverse definition-order))
entries (flat-entries result)]
;; Every set is present exactly once, and nothing outside the
;; subgroup reordered its two members relative to each other.
(t/is (= #{[nil "global"] [nil "primitives"]
["brand/subgroup" "one"] ["brand/subgroup" "two"]}
(set entries)))
(t/is (= [["brand/subgroup" "two"] ["brand/subgroup" "one"]]
(filter #(= (first %) "brand/subgroup") entries)))))