add placeholder to keep custom palette ID's consistent#5537
Merged
add placeholder to keep custom palette ID's consistent#5537
Conversation
Contributor
WalkthroughThe PR modifies custom palette loading to insert gray placeholder entries for missing palette files, ensuring proper palette index alignment. The UI filters out these placeholders during rendering, while JSON documentation clarifies that the palette count includes them. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 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. 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}
Contributor
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 (2)
wled00/colors.cpp (1)
271-307:⚠️ Potential issue | 🟡 MinorID-preservation gap when a palette file exists but fails to parse.
Placeholders are only inserted in the file-exists branch before attempting to load. If
WLED_FS.exists(fileName)returns true butreadObjectFromFile()fails or the JSON has an invalid format (pal.isNull() || pal.size()<=3),emptyPaletteGaphas already been reset and no entry is pushed for this slot — so any later palettes loaded in the sameloadCustomPalettes()call will shift down by one and lose the ID stability this PR is meant to provide.If preserving IDs across all file-state failure modes is desired, treat parse/validation failures the same as a missing file by inserting a gray placeholder when the load doesn't succeed.
🔧 One possible fix
DEBUGFX_PRINTF_P(PSTR("Reading palette from %s\n"), fileName); + bool palLoaded = false; if (readObjectFromFile(fileName, nullptr, &pDoc)) { JsonArray pal = pDoc[F("palette")]; if (!pal.isNull() && pal.size()>3) { // not an empty palette (at least 2 entries) ... customPalettes.push_back(targetPalette.loadDynamicGradientPalette(tcp)); + palLoaded = true; } else { DEBUGFX_PRINTLN(F("Wrong palette format.")); } } + if (!palLoaded) customPalettes.push_back(CRGBPalette16(CRGB(128, 128, 128))); // keep ID stable } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/colors.cpp` around lines 271 - 307, The issue: placeholders are added immediately when WLED_FS.exists(fileName) is true but emptyPaletteGap is cleared before parsing, so parse/read failures cause missing placeholders and ID shift. Fix by treating parse/validation failures like a missing file: if readObjectFromFile(...) returns false or the palette JSON is invalid (pal.isNull() || pal.size()<=3) then push the same gray-placeholder(s) into customPalettes (using the same CRGBPalette16(CRGB(128,128,128)) logic) and do not reset emptyPaletteGap unless a palette was successfully loaded; adjust logic around WLED_FS.exists(fileName), readObjectFromFile, emptyPaletteGap, customPalettes, and the targetPalette.loadDynamicGradientPalette/tcp path to ensure placeholders are added when load fails.wled00/data/index.js (1)
995-1007:⚠️ Potential issue | 🟡 MinorBrittle placeholder detection via a hard-coded sentinel color.
Two concerns with detecting placeholders by "16 entries with RGB all equal 128":
- Cross-file magic constant. The sentinel
(128,128,128)is also produced inwled00/colors.cpp(CRGBPalette16(CRGB(128, 128, 128))). If anyone tweaks the placeholder color there, this filter silently breaks. Consider centralizing the constant (e.g. a shared comment + named constant in C++) or using a more explicit sentinel that is unlikely to collide with user palettes.- False positive on user palettes. A user-authored custom palette that happens to be 16 mid-gray stops would be silently hidden from the palette list. Niche, but valid.
Optional mitigation: also require the
cpalcount-vs-file-existence relationship (i.e. only filter slots whosepalette<n>.jsonwas reported missing), or pick a sentinel pattern that varies per entry to make accidental collisions effectively impossible.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/data/index.js` around lines 995 - 1007, The current placeholder detection in the loop using palettesData[255-j] and checking for 16 entries of RGB(128,128,128) is brittle and can hide legitimate user palettes; update the logic in the code that builds the list (the loop referencing li.cpalcount, palettesData, generateListItemHtml and genPalPrevCss) to use a stronger sentinel: either (A) detect placeholders by consulting the actual palette file existence/manifest reported by the server (e.g., only treat an entry as a placeholder if the corresponding palette<n>.json was reported missing) or (B) require an additional explicit marker/flag in palettesData (e.g., a boolean like isPlaceholder) instead of relying on the RGB magic value; implement one of these approaches and remove the hard-coded RGB(128,128,128) check so user-created 16-step mid-gray palettes are not accidentally filtered out.
🧹 Nitpick comments (1)
wled00/colors.cpp (1)
272-275: Optional: usevector::insertfor the bulk placeholder push.Minor readability/efficiency: a single
insertwith a count avoids per-element capacity checks.♻️ Suggested change
- // add gray placeholders to preserve palette IDs for subsequent slots (is omitted in UI but shown in cpal.htm) - for (unsigned g = 0; g < emptyPaletteGap; g++) - customPalettes.push_back(CRGBPalette16(CRGB(128, 128, 128))); - emptyPaletteGap = 0; // reset gap counter if file exists + // add gray placeholders to preserve palette IDs for subsequent slots (omitted in UI, shown in cpal.htm) + customPalettes.insert(customPalettes.end(), emptyPaletteGap, CRGBPalette16(CRGB(128, 128, 128))); + emptyPaletteGap = 0; // reset gap counter if file exists🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/colors.cpp` around lines 272 - 275, Replace the per-element loop that calls customPalettes.push_back(...) for emptyPaletteGap with a single bulk insert: construct one CRGBPalette16(CRGB(128,128,128)) and call customPalettes.insert to append emptyPaletteGap copies, then reset emptyPaletteGap; update references to the loop around customPalettes, emptyPaletteGap, CRGBPalette16 and CRGB to locate the change. This reduces repeated capacity checks and improves readability while preserving the exact placeholder value and behavior.
🤖 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 `@wled00/colors.cpp`:
- Around line 271-307: The issue: placeholders are added immediately when
WLED_FS.exists(fileName) is true but emptyPaletteGap is cleared before parsing,
so parse/read failures cause missing placeholders and ID shift. Fix by treating
parse/validation failures like a missing file: if readObjectFromFile(...)
returns false or the palette JSON is invalid (pal.isNull() || pal.size()<=3)
then push the same gray-placeholder(s) into customPalettes (using the same
CRGBPalette16(CRGB(128,128,128)) logic) and do not reset emptyPaletteGap unless
a palette was successfully loaded; adjust logic around WLED_FS.exists(fileName),
readObjectFromFile, emptyPaletteGap, customPalettes, and the
targetPalette.loadDynamicGradientPalette/tcp path to ensure placeholders are
added when load fails.
In `@wled00/data/index.js`:
- Around line 995-1007: The current placeholder detection in the loop using
palettesData[255-j] and checking for 16 entries of RGB(128,128,128) is brittle
and can hide legitimate user palettes; update the logic in the code that builds
the list (the loop referencing li.cpalcount, palettesData, generateListItemHtml
and genPalPrevCss) to use a stronger sentinel: either (A) detect placeholders by
consulting the actual palette file existence/manifest reported by the server
(e.g., only treat an entry as a placeholder if the corresponding palette<n>.json
was reported missing) or (B) require an additional explicit marker/flag in
palettesData (e.g., a boolean like isPlaceholder) instead of relying on the RGB
magic value; implement one of these approaches and remove the hard-coded
RGB(128,128,128) check so user-created 16-step mid-gray palettes are not
accidentally filtered out.
---
Nitpick comments:
In `@wled00/colors.cpp`:
- Around line 272-275: Replace the per-element loop that calls
customPalettes.push_back(...) for emptyPaletteGap with a single bulk insert:
construct one CRGBPalette16(CRGB(128,128,128)) and call customPalettes.insert to
append emptyPaletteGap copies, then reset emptyPaletteGap; update references to
the loop around customPalettes, emptyPaletteGap, CRGBPalette16 and CRGB to
locate the change. This reduces repeated capacity checks and improves
readability while preserving the exact placeholder value and behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c39eb8bb-bd77-43da-8379-b80fa3b683aa
📒 Files selected for processing (3)
wled00/colors.cppwled00/data/index.jswled00/json.cpp
DedeHai
added a commit
that referenced
this pull request
Apr 25, 2026
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.
fix for #5533
when deleting custom palettes, they now keep their ID, leaving previously created presets unchanged.
The "dummy" palettes inserted are gray, so if a custom palette that was used in a preset is deleted, it will end up as gray to indicate an empty palette. Also the "dummy" palettes are omitted in the custom palette list.
Summary by CodeRabbit
Release Notes