New image & scrolling text interface: "WLED Matrix tool" #4982
Conversation
WalkthroughAdds a new Matrix Tool web UI and GIF encoder/decoder, updates build script to emit matrixtool HTML (non-inlined CSS), wires a new server route, updates the homepage button to point to the Matrix Tool, and extracts/adjusts some styling. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wled00/data/style.css (1)
39-51: Indent with tabs per project guideline.New declarations must use tabs for indentation in
wled00/dataassets; the added lines currently use spaces.Apply this diff:
- transition: all 0.3s ease; + transition: all 0.3s ease; } -button:hover, .btn:hover{ - background:#555; - border-color:#555; -} +button:hover, .btn:hover{ + background:#555; + border-color:#555; +}As per coding guidelines
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
tools/cdata.js(3 hunks)wled00/data/index.htm(1 hunks)wled00/data/matrixtool/matrixtool.htm(1 hunks)wled00/data/matrixtool/omggif.js(1 hunks)wled00/data/pxmagic/pxmagic.htm(1 hunks)wled00/data/settings.htm(1 hunks)wled00/data/style.css(2 hunks)wled00/wled_server.cpp(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
wled00/data/**/*.{htm,html,css,js}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use tabs for indentation in web files (.htm/.html/.css/.js) under wled00/data
Files:
wled00/data/style.csswled00/data/matrixtool/omggif.jswled00/data/matrixtool/matrixtool.htmwled00/data/settings.htmwled00/data/pxmagic/pxmagic.htmwled00/data/index.htm
wled00/**/*.{cpp,h}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use spaces (2 per level) for all C++ source and header files
Files:
wled00/wled_server.cpp
🧠 Learnings (2)
📚 Learning: 2025-09-20T09:00:08.942Z
Learnt from: CR
PR: wled/WLED#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-20T09:00:08.942Z
Learning: Applies to wled00/html_*.h : DO NOT edit generated headers wled00/html_*.h
Applied to files:
wled00/wled_server.cpptools/cdata.js
📚 Learning: 2025-09-14T18:43:59.338Z
Learnt from: blazoncek
PR: wled/WLED#4932
File: wled00/data/cpal/cpal.htm:14-14
Timestamp: 2025-09-14T18:43:59.338Z
Learning: In WLED, static web pages like cpal.htm are served directly at the root URL level (e.g., "/cpal.htm") via handleStaticContent() from embedded page data, regardless of their physical source folder location in the repository. Relative script references like "common.js" resolve correctly from the served URL path, not the source file path.
Applied to files:
tools/cdata.js
🪛 Biome (2.1.2)
wled00/data/matrixtool/omggif.js
[error] 438-438: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 464-464: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 465-465: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 466-466: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 467-467: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 468-468: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 469-469: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 470-470: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 471-471: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 472-472: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 473-473: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 474-474: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 475-475: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 483-483: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 512-512: This code is unreachable
(lint/correctness/noUnreachable)
[error] 32-32: Shouldn't redeclare 'gopts'. Consider to delete it or rename it.
'gopts' is defined here:
(lint/suspicious/noRedeclare)
[error] 32-32: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 32-32: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 448-448: Shouldn't redeclare 'block_size'. Consider to delete it or rename it.
'block_size' is defined here:
(lint/suspicious/noRedeclare)
[error] 477-477: Shouldn't redeclare 'has_local_palette'. Consider to delete it or rename it.
'has_local_palette' is defined here:
(lint/suspicious/noRedeclare)
[error] 487-487: Shouldn't redeclare 'block_size'. Consider to delete it or rename it.
'block_size' is defined here:
(lint/suspicious/noRedeclare)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (esp32_wrover)
- GitHub Check: wled_build / Build Environments (esp32s3dev_8MB_opi)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (esp32dev)
- GitHub Check: wled_build / Build Environments (esp32_eth)
- GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi)
- GitHub Check: wled_build / Build Environments (lolin_s2_mini)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
wled00/data/matrixtool/matrixtool.htm (7)
254-440: Consider accessibility enhancements.The UI structure is well-organized. For better accessibility, consider adding ARIA labels to interactive elements and ensuring full keyboard navigation support for the cropping canvas and context menus.
467-491: Segment filtering correctly prevents NaN dimensions.The implementation at lines 477-481 correctly filters out 1D segments (where
h > 1 && w > 1is false), which resolves the NaN dimension issue from previous discussions. The fallback values at line 489 (||16) provide safe defaults when segment data is missing.Consider showing errors to users.
Line 490 catches errors with
console.error, which logs to the console but doesn't notify the user. Consider using thetoast()function to display fetch errors:- }).catch(console.error); + }).catch(e => { console.error(e); toast('Failed to load segments', 'error'); });
537-563: Consider parallelizing image loading.The sequential image loading at lines 540-562 uses
awaitinside afor...ofloop, which loads images one at a time. For better performance, consider loading images in parallel:- for(const f of imgs){ + await Promise.all(imgs.map(async (f) => { const name=f.name.replace('/',''),url=`${wURL}/${name}`; // ... rest of loading logic - } + }));This will significantly improve load times when multiple images are present on the device.
786-799: Prefertoast()overalert()for consistency.Line 788 uses
alert()for the unsupported format message, which is inconsistent with thetoast()pattern used elsewhere in the codebase. Consider refactoring:function showUnsupported(url,name){ - const msg=`Image format not supported by WLED.\nPlease convert to GIF below or use the Pixel Magic Tool.`; - alert(msg); + toast('Image format not supported by WLED. Please convert to GIF below or use the Pixel Magic Tool.', 'error'); fetch(url)This improves UI consistency and user experience.
959-976: Simplify shadow disabling.Line 974 sets
shadowColorto"#000F"(transparent) to disable the shadow effect. A clearer approach is to setshadowBlurto 0:ctx.roundRect(crop.x, crop.y, crop.w, crop.h, 6); ctx.stroke(); - ctx.shadowColor = "#000F"; // disable shadow again by simply making it transparent + ctx.shadowBlur = 0; + ctx.shadowColor = "transparent"; updPrev(); }
1139-1156: Consider usingaddEventListenerfor context menu.Lines 1146-1148 use inline
onclickattributes in the context menu HTML, which can cause CSP violations and reduces maintainability. Consider refactoring to event listeners:menu.innerHTML=` - <button onclick="downloadImg()">Download</button> - <button class="danger" onclick="deleteImg()">Delete</button> - <button onclick="closeMenu()">Cancel</button>`; + <button data-action="download">Download</button> + <button class="danger" data-action="delete">Delete</button> + <button data-action="cancel">Cancel</button>`; + menu.querySelector('[data-action="download"]').onclick = downloadImg; + menu.querySelector('[data-action="delete"]').onclick = deleteImg; + menu.querySelector('[data-action="cancel"]').onclick = closeMenu;
1249-1255: Consider using named constants for effect IDs.The code uses magic numbers for effect IDs (e.g., 53 for image, 122 for scrolling text) throughout the file. Consider defining named constants at the top of the script for better maintainability:
const FX_IMAGE = 53; const FX_SCROLLING_TEXT = 122; // Then use throughout: function upTxt(){ const id=+gId('segTxt').value,txt=gId('txtIn').value.trim().slice(0,64); const j={on:true,seg:{id,fx:FX_SCROLLING_TEXT,n:txt,...}};This makes the code more self-documenting and easier to maintain if effect IDs change.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wled00/data/matrixtool/matrixtool.htm(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
wled00/data/**/*.{htm,html,css,js}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use tabs for indentation in web files (.htm/.html/.css/.js) under wled00/data
Files:
wled00/data/matrixtool/matrixtool.htm
wled00/data/**
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
wled00/data/**: When modifying web UI files, runnpm run buildto regenerate embedded headers before any firmware build
For web UI changes, edit files only under wled00/data (not firmware or generated files)
Files:
wled00/data/matrixtool/matrixtool.htm
🧠 Learnings (2)
📚 Learning: 2025-10-05T15:41:15.790Z
Learnt from: DedeHai
PR: wled/WLED#4982
File: wled00/data/matrixtool/omggif.js:32-35
Timestamp: 2025-10-05T15:41:15.790Z
Learning: The file `wled00/data/matrixtool/omggif.js` is an external third-party library from https://github.com/deanm/omggif by Dean McNamee and should not be reviewed for code issues as it's maintained externally.
Applied to files:
wled00/data/matrixtool/matrixtool.htm
📚 Learning: 2025-10-05T15:24:05.535Z
Learnt from: CR
PR: wled/WLED#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-05T15:24:05.535Z
Learning: Applies to wled00/data/**/*.{htm,html,css,js} : Use tabs for indentation in web files (.htm/.html/.css/.js) under wled00/data
Applied to files:
wled00/data/matrixtool/matrixtool.htm
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (18)
- GitHub Check: wled_build / Build Environments (esp32c3dev)
- GitHub Check: wled_build / Build Environments (lolin_s2_mini)
- GitHub Check: wled_build / Build Environments (esp32s3dev_8MB_opi)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi)
- GitHub Check: wled_build / Build Environments (esp32_wrover)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (esp32_eth)
- GitHub Check: wled_build / Build Environments (esp32dev)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_compat)
- GitHub Check: wled_build / Build Environments (esp8266_2m_compat)
- GitHub Check: wled_build / Build Environments (esp8266_2m_160)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_160)
- GitHub Check: wled_build / Build Environments (nodemcuv2_compat)
- GitHub Check: wled_build / Build Environments (esp01_1m_full)
- GitHub Check: wled_build / Build Environments (nodemcuv2_160)
- GitHub Check: wled_build / Build Environments (nodemcuv2)
- GitHub Check: wled_build / Build Environments (esp8266_2m)
🔇 Additional comments (1)
wled00/data/matrixtool/matrixtool.htm (1)
1-253: LGTM!The HTML structure and CSS styling are well-organized. The tab indentation follows the project guidelines for web assets under
wled00/data. The styling provides a comprehensive and polished UI for the Matrix Tool.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
wled00/data/matrixtool/matrixtool.htm (3)
712-712: Consider adding clarifying comment for delay conversion.The
10multiplier converts GIF centiseconds to milliseconds, and50sets a reasonable minimum. A brief comment would help future maintainers.Apply this diff:
const avg=gF.reduce((s,f)=>s+f.delay,0)/gF.length; -aT=setInterval(step,Math.max(avg*10,50)); +// GIF delays are in centiseconds; convert to ms with 50ms minimum +aT=setInterval(step,Math.max(avg*10,50));
956-975: Consider using a more sophisticated color quantization algorithm.The current bit-reduction approach works but may produce poor results for images with many colors. A median-cut or octree quantization would preserve colors better.
The current implementation is functional for a simple tool, but if image quality is important, consider implementing median-cut quantization or using a library. The current approach progressively drops LSBs which can cause significant banding in gradients.
Example libraries:
- RgbQuant.js
- quantize.js
However, this would add dependency size, so only consider if image quality issues are reported.
878-878: Use more precise regex for hex color parsing.The pattern
/\w\w/gmatches any two word characters, but/[0-9a-fA-F]{2}/gmore precisely matches hex digits. While color inputs are browser-controlled, the more specific pattern is technically more correct.Apply this diff:
let t=+gId('bt').value, dt=c.getImageData(0,0,c.canvas.width,c.canvas.height), - b=gId('bg').value.match(/\w\w/g).map(x=>parseInt(x,16)); + b=gId('bg').value.match(/[0-9a-fA-F]{2}/g).map(x=>parseInt(x,16));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wled00/data/matrixtool/matrixtool.htm(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
wled00/data/**/*.{htm,html,css,js}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use tabs for indentation in web files (.htm/.html/.css/.js) under wled00/data
Files:
wled00/data/matrixtool/matrixtool.htm
wled00/data/**
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
wled00/data/**: When modifying web UI files, runnpm run buildto regenerate embedded headers before any firmware build
For web UI changes, edit files only under wled00/data (not firmware or generated files)
Files:
wled00/data/matrixtool/matrixtool.htm
🧠 Learnings (2)
📚 Learning: 2025-10-05T15:41:15.790Z
Learnt from: DedeHai
PR: wled/WLED#4982
File: wled00/data/matrixtool/omggif.js:32-35
Timestamp: 2025-10-05T15:41:15.790Z
Learning: The file `wled00/data/matrixtool/omggif.js` is an external third-party library from https://github.com/deanm/omggif by Dean McNamee and should not be reviewed for code issues as it's maintained externally.
Applied to files:
wled00/data/matrixtool/matrixtool.htm
📚 Learning: 2025-10-05T15:24:05.535Z
Learnt from: CR
PR: wled/WLED#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-05T15:24:05.535Z
Learning: Applies to wled00/data/**/*.{htm,html,css,js} : Use tabs for indentation in web files (.htm/.html/.css/.js) under wled00/data
Applied to files:
wled00/data/matrixtool/matrixtool.htm
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (17)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (lolin_s2_mini)
- GitHub Check: wled_build / Build Environments (esp32dev)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (esp32_eth)
- GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi)
- GitHub Check: wled_build / Build Environments (esp32_wrover)
- GitHub Check: wled_build / Build Environments (esp32c3dev)
- GitHub Check: wled_build / Build Environments (esp32s3dev_8MB_opi)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_compat)
- GitHub Check: wled_build / Build Environments (esp8266_2m_160)
- GitHub Check: wled_build / Build Environments (esp8266_2m_compat)
- GitHub Check: wled_build / Build Environments (nodemcuv2_compat)
- GitHub Check: wled_build / Build Environments (nodemcuv2_160)
- GitHub Check: wled_build / Build Environments (esp8266_2m)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_160)
- GitHub Check: wled_build / Build Environments (esp01_1m_full)
🔇 Additional comments (5)
wled00/data/matrixtool/matrixtool.htm (5)
469-477: Segment filtering correctly prevents NaN dimensions.The check
if(h>1&&w>1)properly filters out 1D segments and prevents NaN values from being stored in the dataset, as the dataset assignments are inside the conditional block.
697-713: GIF animation logic is well-structured.The function properly handles edge cases (no frames, single frame), clears existing intervals before starting new ones, and uses a sensible minimum frame delay to prevent excessive CPU usage.
640-668: GIF disposal handling correctly implements the GIF89a spec.The frame-by-frame decoding properly handles disposal modes:
- Mode 2: Restore to background (line 660)
- Mode 3: Restore to previous frame (line 661)
This ensures animated GIFs display correctly even with complex disposal requirements.
984-1005: Image playback correctly handles segment conflicts.The function detects when another segment is showing an image and prompts the user before switching. The conditional payload construction (lines 993-995) ensures the old segment is properly cleared (fx=0) when switching.
1072-1082: Token insertion properly handles edge cases.The function correctly preserves cursor position, respects the 64-character limit, and handles missing selection positions with nullish coalescing. The Math.min ensures the cursor doesn't overflow the string length.
| const w=+gId('w').value,h=+gId('h').value,fn=gId('fn').value.trim()||'image'; | ||
| const filename=`${fn}.gif`; |
There was a problem hiding this comment.
Sanitize user-provided filename to prevent path traversal.
The filename is constructed from user input without sanitization. While the server should validate, client-side sanitization provides defense-in-depth against path traversal attempts.
Apply this diff to sanitize the filename:
const w=+gId('w').value,h=+gId('h').value,fn=gId('fn').value.trim()||'image';
-const filename=`${fn}.gif`;
+// Sanitize filename: remove path separators and limit to safe characters
+const safeFn = fn.replace(/[^a-zA-Z0-9_-]/g, '_').substring(0, 26);
+const filename=`${safeFn}.gif`;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const w=+gId('w').value,h=+gId('h').value,fn=gId('fn').value.trim()||'image'; | |
| const filename=`${fn}.gif`; | |
| const w = +gId('w').value, | |
| h = +gId('h').value, | |
| fn = gId('fn').value.trim() || 'image'; | |
| // Sanitize filename: remove path separators and limit to safe characters | |
| const safeFn = fn.replace(/[^a-zA-Z0-9_-]/g, '_').substring(0, 26); | |
| const filename = `${safeFn}.gif`; |
🤖 Prompt for AI Agents
In wled00/data/matrixtool/matrixtool.htm around lines 899-900, the code builds
filename from user input without sanitization, allowing path traversal or
malformed names; sanitize the user-provided fn by removing any path separators
and traversal tokens (e.g., strip slashes and backslashes, remove any "../" or
"..\" sequences), allow only a safe subset of characters (e.g., letters, digits,
hyphen, underscore), collapse multiple dots so only a single basename remains,
fall back to 'image' if the sanitized name is empty, then append the fixed
'.gif' extension; ensure the resulting filename contains no directory components
and contains only the allowed characters before assigning to filename.
replaces the pixel magic tool.
This implements a much more feature-rich and user friendly way of displaying images.
Features:
Image conversion tool:
Scrolling Text tool:
Total additional flash usage: 2.5k
For consistency I also updated the button styling in style.css: it now also uses a hover action.
Took some inspiration from @Manut38 (https://github.com/Manut38/WLED-GifPlayer-UI)
Summary by CodeRabbit
New Features
UI Changes
Performance/Behavior
Style