Repository navigation
Conversation
There was a problem hiding this comment.
Pull request overview
Refines battery hold detection so pending-charge is treated as a configured threshold hold only within a real limit band.
Changes:
- Adds threshold-aware hold detection to the CLI and power panel.
- Threads raw limits through panel model helpers.
- Expands battery-status and power-model tests.
Tip
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
bin/omarchy-battery-status |
Validates pending-charge against threshold bounds. |
shell/plugins/panels/power/Model.js |
Adds threshold-aware panel logic. |
shell/plugins/panels/power/Panel.qml |
Passes reported limits into model helpers. |
test/shell.d/battery-status-test.sh |
Tests default, in-band, and out-of-band states. |
test/shell.d/power-test.sh |
Tests model threshold handling. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| function batteryIcon() { | ||
| var device = UPower.displayDevice | ||
| return Model.batteryIcon(device, root.discharging, upowerStates()) | ||
| return Model.batteryIcon(device, root.discharging, upowerStates(), root.batteryLimits) |
| const inBand = { start: '75', end: '80' } | ||
|
|
||
| assert(power.chargeThresholdActive({ isPresent: true, percentage: 0.8, state: states.PendingCharge }, false, states, inBand), 'power detects threshold by pending charge state inside the limit band') | ||
| assert(!power.chargeThresholdActive({ isPresent: true, percentage: 0.1, state: states.PendingCharge }, false, states, { start: '', end: '100' }), 'power ignores pending charge outside a real limit band') |
tahadx
left a comment
There was a problem hiding this comment.
Reviewer disclosure: this review was written by an AI coding assistant (ox-alpha) at the contributor's request, and posted with their approval. Please weigh it alongside human review.
What I checked
The two-layer fix reads well: omarchy-battery-status stops treating kernel pending-charge as a threshold hold unless a real limit is configured (threshold_end < 99) and the level sits inside the band, while Model.chargeThresholdActive gets the same discipline via the new chargeLimitsInBand helper. Emitting raw threshold_start/threshold_end keys from --shell output so the panel owns its own policy (rather than duplicating it) is a clean split of responsibilities.
Observations
- Integer safety confirmed:
percentage,threshold_start, andthreshold_endare all normalized through awkint()before reaching the(( ))band check, so fractional UPower output cannot break bash arithmetic on this path. - Small asymmetry between layers: the bash side accepts any band with
end < 99, butchargeLimitsInBandadditionally requiresend >= 1. A hypotheticalend == 0configuration would report holding in shell output but not in the panel icon. Harmless today; aligning the two predicates would prevent future drift if either copy evolves independently. - Test coverage mirrors the bug report: dead pack at 0% under kernel-default limits (not holding, raw keys still exposed), in-band 78% at 75–80 (holding plus label), out-of-band 90% (not holding), plus panel-level cases for missing limits. The refactored parameterized
setup_batteryharness makes those cases readable — good test ergonomics.
tahadx
left a comment
There was a problem hiding this comment.
Reviewer disclosure: this review was written by an AI assistant at the contributor's request and posted with their approval. I am an AI, not a human reviewer — please weigh it alongside human review.
Rich code review — #7915 (pending-charge hold)
The two-layer guard is sound. The CLI now only reports a hold when a real limit is configured (threshold_end < 99) and the level sits inside the band, and the panel mirrors that through the new chargeLimitsInBand helper instead of duplicating the policy. Emitting raw threshold_start/threshold_end from --shell so the panel owns its own logic is a clean single-source split.
Verified
- Bash path:
percentage >= ${threshold_start:-0}andthreshold_end < 99are guarded by the outer-n $threshold_end, and the(( ))check degrades gracefully whenthreshold_startis empty (:-0). Correct and safe. - JS path:
Number("") == 0makes a missing/empty limit return false;end >= 99rejects kernel defaults;end >= 1guards degenerate configs. On first render beforebatteryInfofills,Number(undefined)is NaN and!(NaN >= 1)short-circuits safely. - Wiring:
batteryInfois populated byModel.parseKeyValuefrom the--shelloutput, so the new keys land as reactive properties andbatteryLimitsre-evaluates on refresh. Verified thetargetName === "battery"path. - Tests: battery-status + power model tests pass.
Nit (non-blocking)
The bash side accepts any band with end < 99, while the JS side additionally requires end >= 1. A hypothetical end == 0 config would report holding in the CLI but not in the panel icon. Harmless today; aligning the two predicates would prevent future drift.
Verdict: ship.
|
Reviewed at db68d4f, merged onto current The reported bug is fixed. I stubbed UPower and sysfs to match #7803: a dead pack at 0%, sysfs limits 0/100, UPower reporting its hwdb default of 75/80. On One regression: a battery above its limit no longer reads as held.
The same happens with an end-only 80% limit at 95%. On It depends on where the limits come from. Lines 65-69 read UPower's thresholds first and fall back to sysfs. Where UPower reports its hwdb default rather than the active limit (#7374, #9586), the new start check compares against the wrong number. Example: a TLP limit of 55/60 held at 60% while UPower says 75/80 would now read as not held. #9612 reads sysfs first and fixes that, so this should land after it or together with it. Tests: the Related: #10196 also changes Next step is yours, @tahadx: drop the upper bound in both places and flip the 90% assertions. Once that is in, it goes to the maintainer, who will also decide the order with #9612. |
db68d4f to
fb703b5
Compare
|
Reproduced on a ThinkPad X1 Carbon Gen 11 (Omarchy 4.0.3-1, UPower, After plugging in the charger, the EC leaves the battery on Timeline from That is about 4s of |
|
The head is now fb703b5. It has the same tree as db68d4f, which I reviewed above, so the code has not changed and those findings still stand. The main one: the @dipunm's report is a second case this PR fixes: a pack at 9% with a 75-80 limit, sitting in This pass was read only, by Claude Opus 5.5. Nothing was run on a worker, because the code is unchanged. Re-classifying the PR at the new head needs a second opinion from Codex Medium. The reviewer is up, but omabot has used its review budget for today, so it did not run. The classification stays unsettled at this head until a later run gets that second opinion. Next step is still yours, @tahadx: drop the upper bound in both places and flip the above-limit assertions. After that it goes to the maintainer, who also decides the order with #9612 and #10196. |
|
quattro rewrote battery-status-test.sh around an Apple Silicon fixture and an idle-charging hold fixture, and its upower stub now has to say "power supply: yes" for the script to pick the battery. Keep quattro's tests as they are and run this branch's pending-charge scenarios on a fixture of their own, with that line added. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
UPower's percentage reaches the model as a fraction, and multiplying it back by 100 lands just under some whole numbers (0.29 * 100 is 28.999...). A pack held exactly at a 29% or 58% start then read as held in omarchy-battery-status, which compares integers, and as charging in the panel. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Co-Authored-By: Codex Medium <noreply@openai.com>
The bar needs the limits while the panel is closed, but a per-monitor Timer that starts omarchy-battery-status every 30 seconds runs forever on an idle laptop, which agents/skills/shell-dev.md rules out for state UPower already signals. The limits only decide a PendingCharge hold, so read them at startup, when a battery appears, and whenever UPower's state changes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Co-Authored-By: Codex Medium <noreply@openai.com>
Checking only that "holding" is absent passes on empty output, or on any other wrong state. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Co-Authored-By: Codex Medium <noreply@openai.com>
With the 30-second poll gone, a startup lookup that returned nothing left the bar without limits until UPower's state next changed, so a pack already held showed as charging. Retry while closed only until the first lookup lands, the bounded poll agents/skills/shell-dev.md allows, and assert that is the only timer calling the battery script. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Co-Authored-By: Codex Medium <noreply@openai.com>
refreshBattery() dropped a request while omarchy-battery-status was already running, but that run may have read the charge limits before the state change that asked for the refresh. With the panel closed and no further state change, the bar then kept the old limits. Queue one follow-up lookup instead, the way the wifiqr panel queues a request that arrives while its process runs. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Co-Authored-By: Codex Medium <noreply@openai.com>
|
Reviewed again at 95301dc and run on a disposable Omarchy worker VM. Reviewed by Claude Opus 5.5 and, as a second opinion, Codex Medium over four rounds. It saw my reasoning, so where it agrees that is not a fully independent check. Thanks @tahadx, both earlier findings are fixed. A pack held above its end limit reads as held again, and a pack at 0% is never read as a hold, including with an end-only or zero-start limit, as greptile pointed out. The bug is fixed. On the worker I stubbed UPower and sysfs. On What I pushed to your branch:
What is left: if the limits are changed while the battery stays in pending-charge, the bar keeps the old ones until the state next changes or the panel is opened. Nothing announces that change. The limits are sysfs attributes, which inotify does not report, and no Omarchy command writes them. Codex Medium judged that acceptable for this fix. Tests: Related: #10196 and #9612 both overlap this one. #10196 fixes a different bug in Nothing is left for you to do, @tahadx. It now waits on the maintainer, who will also decide the order with #9612 and #10196. |
|
This no longer merges into I resolved the conflict locally by keeping both sets of tests, and pushed nothing. The second opinion (Codex Medium) is up but has used up today's review budget, so it could not re-check the classification at this head or review the merge. Nothing here runs ahead of that check. This pass was read only, by Claude Opus 5.5. Nothing ran on a worker. Nothing is needed from you, @tahadx. Once a second opinion is available again, omabot will push the merge, run it on a worker and bring the PR back to the maintainer. |
Closes #7803
A battery parked in kernel
pending-chargewas always reported as athreshold hold, even when the hold came from something other than a
configured limit — a dead pack at 0% with kernel-default limits (0/100)
showed "Holding at 75-80%".
UPower's
pending-chargeonly means the EC is not charging the pack. Acharge limit is one reason, but so are a dead battery, an undersized
adapter, or inhibit-charge. Both the status script and the power panel now
only treat it as a hold when a real limit is configured (
end < 99) andthe level sits inside its band:
omarchy-battery-statusgates thepending-chargebranch onthreshold_end < 99andpercentageinside${threshold_start:-0}..$threshold_end, and emits rawthreshold_start/threshold_endkeys in--shelloutput.Model.chargeThresholdActive,which now requires them for the
PendingChargebranch; machines withno limit data fall back to the plain charging/on-battery presentation.
Icon and mode-label helpers thread the same limits through.
The stalled-charging and fully-charged holds are unchanged.
Tests: new battery-status scenarios cover a dead pack at 0% under default
limits (not holding), an in-band 78% at 75-80 (holding plus raw keys), and
an out-of-band 90% (not holding); power model tests cover in-band,
out-of-band, and missing-limit pending charge.