Repository navigation
fanctrlplus2: apply a field's floor on blur, not while typing (#17) - #18
Conversation
Typing 20 into the interval field gave 50: the first digit was raised to the 5-second floor before the second one arrived. The floor now applies when the field is left; while typing, only the ceiling applies, and a value below the floor gets an outline and a tooltip naming the minimum.
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 54 minutes. View limit detailsLimit details: You’ve used the included review currently available. Review configuration: ⚙️ Run configuration
📒 Files selected for processing (2)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (3)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughInterval fields now preserve below-minimum values during editing and enforce the minimum on blur. Below-minimum values receive a temporary tooltip and amber outline. Initialization continues to enforce both bounds. ChangesInterval entry
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix · Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to Interval fields keep values below the minimum while the user types and raise them to the minimum on blur. This fixes the bug where typing 20 could produce 50. No merge-blocking risk was identified. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files. (2 skipped: 2 unsupported.) ✨ Finishing Touches 💡 1📝 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 |
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a6944baa07
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| }); | ||
|
|
||
| enforceUnit(); // Apply once during initialization. | ||
| enforceUnit(true); // Apply once during initialization. |
There was a problem hiding this comment.
When an Add Fan or Add Disk Group AJAX request completes while the user is editing another field, the call sites at lines 521–531 and 1482–1484 invoke bindUnitInputs with selectors that also match every existing input. This unconditional initialization therefore applies the floor to the active partial value, so typing 2 toward 20 can still become 5 when the asynchronous callback runs; it can also capture the temporary minimum-warning tooltip as the field's ownTitle. Initialize only newly inserted inputs, or skip inputs that were already bound.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 5056f50. bindUnitInputs marks each input it binds and skips it on later calls, so Add Fan and Add Disk Group only initialize the new fields. That also stops the duplicate listeners. I added a test that rebinds while a field holds a partial value.
Add Fan and Add Disk Group rebind every matching field, so a field being typed in was re-initialized and raised to its floor mid-entry.
1 participant
Fixes #17.
Typing
20into the Interval field gave50.bindUnitInputsraised the first digit to the 5-second floor before the second digit arrived.Low Temp, comes back afterwards.cfg_interval_seconds.This affects every
bindUnitInputsfield (percent and temperature too); before, their floors also fired mid-entry.Tests:
tests/interval_ui_test.jsdrives the realbindUnitInputs. It fails onmainand passes here, andtests/run-all.shis green. I also checked it by hand in headless Chromium against a real input.Summary by CodeRabbit