Increase maximum playlist duration#5455
Merged
Conversation
Contributor
|
Caution Review failedPull request was closed or merged during review WalkthroughUpdated playlist duration handling to milliseconds in firmware; adjusted frontend number-input sanitization and playlist editor inputs (max values, ids, widths); changed a sample build flag in platformio_override. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@wled00/data/index.js`:
- Around line 313-327: The input handler attached via d.addEventListener
currently always calls t.oninput() when the parsed value is a number, causing
redundant handler runs; change the logic in that listener (the block that parses
val, max, min and checks bounds) so t.oninput() is invoked only if clamping
actually changed t.value (i.e., when val > max or val < min or when setting
t.value to a coerced string would modify it). Keep the existing parseFloat
checks and use the same t, val, max, min symbols to decide whether to call
t.oninput().
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2f5d3881-3473-4d9f-b074-3bef54786488
📒 Files selected for processing (2)
wled00/data/index.jswled00/playlist.cpp
Contributor
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 `@wled00/data/index.js`:
- Line 2135: The duration input's HTML max (pl${p}du${i}) is too low compared to
the firmware clamp (playlist.cpp allows 42949672 deciseconds => 4294967.2s),
causing valid backend values to be rejected; update the max on the input element
(and any client-side validation in pleDur) to 4294967.2 to match the firmware,
or alternatively reduce the firmware clamp so both sides agree—ensure changes
reference the pl${p}du${i} input, pleDur handler, and plJson duration values.
- Around line 1975-1979: When duration is reduced the code clamps only the
duration field value but never re-clamps the paired transition, so
plJson[p].transition[i] can remain larger than the new dur; update the paired
transition value whenever duration is changed by computing the new allowed
transition = Math.min(parsedTransition, newDur, parseFloat(field.max) or dv) and
assign it back to plJson[p].transition[i] (and to the corresponding transition
field.value if present). Apply this fix at both places where duration is clamped
(the block using parseFloat(field.max); let val = parseFloat(field.value); val =
Math.min(val, max, dv > 0 ? dv : max); field.value = val;) and the similar block
at the other occurrence so the transition is always re-clamped to not exceed the
reduced duration.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
netmindz
approved these changes
Mar 27, 2026
softhack007
approved these changes
Mar 27, 2026
Member
softhack007
left a comment
softhack007
left a comment
There was a problem hiding this comment.
changes in the C++ part look good to me.
I cannot say much about the JS/DOM changes, i'm still a beginner with JS 🤷♂️
Collaborator
Author
|
there is a slight bug in JS still, not a regression but can be done cleaner. |
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
3 participants
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.
Duration limit is now virtually removed, it can go up to millis() limit, i.e. 49.7 days.
Comes at a cost of increased playlist item size, though only a minor issue and only on EPS8266.
Also fixed a long standing bug in the UI: number input fields were not properly limited, especially playlist duration and playlist transition time. This is now fixed: an even listener enforces the given limits for a much better UX as numbers clip instantly and not only after saving (or not at all).
This PR is a prerequesit for #5379
Summary by CodeRabbit
New Features
Bug Fixes
Chores