improve(pi): add recent color history and preset swatches to ird-color-picker (#262)#328
Merged
Merged
Conversation
…r-picker (#262) Replaces the × clear button with an inline swatch group: Not-Set, optional icon default, Black, White, and up to 4 recent colors. Recents are persisted per slot type in flat `_colorHistory*` global settings, shared across per-action overrides and global defaults, and kept in sync across pickers via the global-settings subscription. Commits dedupe and bump colors to the front, truncate to 6 stored, and skip history for preset/not-set/icon-default values. Redundant clicks short-circuit before saving.
github-actions
Bot
added
the
type: improvement
📝 WalkthroughWalkthroughThe ird-color-picker now infers a slot type from Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ColorPicker
participant SettingsAPI
participant Renderer
User->>ColorPicker: Click swatch / input color / blur hex
ColorPicker->>ColorPicker: normalize color, infer slotType, update currentValue
ColorPicker->>SettingsAPI: write history key (deduped, truncated) [if applicable]
SettingsAPI-->>ColorPicker: settings subscription echo (may mirror write)
ColorPicker->>Renderer: re-render swatches (recentColors, icon-default, Not set)
Renderer-->>User: updated UI
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/stream-deck-plugin/src/pi-components/color-picker.test.ts`:
- Around line 9-33: The SDPI stub's makeHook currently overwrites callbacks[key]
(in installSDPIStub), losing previous listeners and preventing multiple
subscribers from syncing; change callbacks to map keys to arrays of listeners
(e.g., callbacks: Record<string, Array<(v:string)=>void>>) and in makeHook push
the new callback into callbacks[key], invoke it with initialByKey[key] on
registration, and ensure when simulating updates you iterate and call every
listener for that key; update any save/callback invocation sites to use the
array shape while preserving saves and
SDPIComponents.useSettings/useGlobalSettings behavior.
In `@packages/stream-deck-plugin/src/pi-components/color-picker.ts`:
- Around line 211-216: attributeChangedCallback currently only reacts to "value"
but observedAttributes lists "setting","default","global" (and
resolveIconDefault reads data-default-color); update attributeChangedCallback to
also handle changes to "setting", "default", "global", and "data-default-color"
by recomputing this.slotType = inferSlotType(this.getAttribute("setting")),
this.iconDefault = this.resolveIconDefault(), calling
this.updateSwatchDisplay(), and re-running this.hookSettings() (ensure
hookSettings unsubscribes the previous history subscription/key before
re-subscribing so the subscription follows the new key); alternatively remove
immutable attributes from observedAttributes if they truly never change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 24c3cb2f-3949-40f8-a1a0-f560e083f8c6
📒 Files selected for processing (3)
packages/stream-deck-plugin/com.iracedeck.sd.core.sdPlugin/ui/pi-components.jspackages/stream-deck-plugin/src/pi-components/color-picker.test.tspackages/stream-deck-plugin/src/pi-components/color-picker.ts
- Trim observedAttributes to just `value`: the `setting`, `default`, and `global` attributes are set once via EJS templates before the element upgrades and are never mutated at runtime. `data-default-color` wasn't observed anyway. Listing them but only handling `value` in attributeChangedCallback was misleading. - Rework the SDPI test stub to track multiple listeners per settings key via an `emit(key, value)` helper, so a test can create two pickers for the same slot and verify they genuinely stay in sync when history changes — the previous shape silently dropped all but the last subscriber. - Add a 2-picker cross-sync test exercising the new behavior.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/stream-deck-plugin/src/pi-components/color-picker.ts (1)
5-7:⚠️ Potential issue | 🟡 MinorTop-level doc comment is stale about the clear button.
The header still describes a built-in clear button, but behavior is now swatch-based.
📝 Suggested doc fix
- * A custom color picker that supports a "not set" state, inline hex display, - * and a built-in clear button. Replaces `<sdpi-color>` which always requires + * A custom color picker that supports a "not set" state, inline hex display, + * and preset/recent swatches (including a "Not set" swatch). Replaces `<sdpi-color>` which always requires🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/stream-deck-plugin/src/pi-components/color-picker.ts` around lines 5 - 7, Update the top-level doc comment in color-picker.ts to remove the outdated mention of a "built-in clear button" and instead describe the current swatch-based clearing behavior; keep that the component is a custom color picker supporting a "not set" state and inline hex display, and explicitly note that clearing is done via swatches rather than a dedicated clear button so the docs match the implementation of the color picker component.
🧹 Nitpick comments (1)
packages/stream-deck-plugin/src/pi-components/color-picker.ts (1)
466-471: Canonicalize incoming history before storinglastSavedHistoryJson.Using raw subscription JSON weakens the echo-loop guard when payload formatting differs but data is equivalent.
♻️ Suggested hardening
- (value: string) => { - const incoming = typeof value === "string" ? value : ""; - - this.recentColors = parseColorHistory(incoming); - this.lastSavedHistoryJson = incoming; - this.updateSwatchDisplay(); - }, + (value: string) => { + const incoming = typeof value === "string" ? value : ""; + const parsed = parseColorHistory(incoming); + this.recentColors = parsed; + this.lastSavedHistoryJson = JSON.stringify(parsed); + this.updateSwatchDisplay(); + },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/stream-deck-plugin/src/pi-components/color-picker.ts` around lines 466 - 471, The code stores the raw incoming subscription JSON in lastSavedHistoryJson which can differ in formatting even when logically identical; instead canonicalize by parsing via parseColorHistory and then serialize the canonical representation before storing. Update the subscription handler (the arrow function that sets this.recentColors and calls updateSwatchDisplay) to set this.lastSavedHistoryJson to a stable JSON produced from the parsed/normalized recentColors (e.g., JSON.stringify of the canonical structure returned by parseColorHistory) so the echo-loop guard compares normalized payloads.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/stream-deck-plugin/src/pi-components/color-picker.ts`:
- Around line 5-7: Update the top-level doc comment in color-picker.ts to remove
the outdated mention of a "built-in clear button" and instead describe the
current swatch-based clearing behavior; keep that the component is a custom
color picker supporting a "not set" state and inline hex display, and explicitly
note that clearing is done via swatches rather than a dedicated clear button so
the docs match the implementation of the color picker component.
---
Nitpick comments:
In `@packages/stream-deck-plugin/src/pi-components/color-picker.ts`:
- Around line 466-471: The code stores the raw incoming subscription JSON in
lastSavedHistoryJson which can differ in formatting even when logically
identical; instead canonicalize by parsing via parseColorHistory and then
serialize the canonical representation before storing. Update the subscription
handler (the arrow function that sets this.recentColors and calls
updateSwatchDisplay) to set this.lastSavedHistoryJson to a stable JSON produced
from the parsed/normalized recentColors (e.g., JSON.stringify of the canonical
structure returned by parseColorHistory) so the echo-loop guard compares
normalized payloads.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1e29f2f9-6329-4ce1-a1c6-0548aa63a2bf
📒 Files selected for processing (3)
packages/stream-deck-plugin/com.iracedeck.sd.core.sdPlugin/ui/pi-components.jspackages/stream-deck-plugin/src/pi-components/color-picker.test.tspackages/stream-deck-plugin/src/pi-components/color-picker.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related Issue
Fixes #262
What changed?
Redesigned
ird-color-pickerto replace the single×clear button with an inline swatch group to the right of the hex input:defaultordata-default-colorattribute (and it's not Black or White).Recents are persisted per slot type (
backgroundColor,textColor,graphic1Color,graphic2Color,borderColor) under flat_colorHistory*global setting keys, so per-action overrides and global defaults of the same slot share one history. Each picker also subscribes to its slot's history so multiple open pickers (e.g. different actions, or per-action vs global pages) stay in sync without a refresh.The hex input is now fixed at 54px to fit all swatches cleanly in the PI panel width.
Commit semantics:
lastSavedHistoryJsonguard prevents feedback loops between the commit path and the subscription echo.How to test
#aa3300. Confirm it appears as the first recent (slot 4) and persists across PI reloads.#ffaa00. Confirm Background recents are unchanged; Text recents are independent.#000000and#ffffff— neither should appear in recents.Checklist
Summary by CodeRabbit