Repository navigation
Conversation
FTW's rule is that the UI never shows a raw minus: it shows a size and says the direction in a word or a mark (Fredrik, 26 October 2026). - formatW returns a size. A new flowMark adds ↑ for a power into the home (import, charge) and ↓ for one out of it (export, discharge), as the charts draw them; formatFlowW joins the two. - Live chart: axis labels are sizes, with "↓ export · discharge · solar" under the zero line. The headroom strip below zero in the Energy view has no labels. Tooltip rows for the grid and batteries, and their targets, carry the mark. - 24 h battery chart: both edge labels are sizes, with "↑ charge ↓ discharge" under the chart. - Plan and diagnose power axes are sizes (powerAxisLabel). The plan's legend reads "↑ import / charge ↓ export / discharge / solar", and diagnose writes its two sides beside the zero line. The plan tooltip shows the battery's size with its word. - Readouts that showed a signed power without a word take the mark: the live strip's grid and battery, the battery and grid targets, the grid target slider, V2X send status, driver battery and meter rows, dispatch targets and the plan brief's safety adjustment. No sign the code computes with changes, only the text on screen. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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: 28c62a190d
ℹ️ 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".
| return (abs / 1000).toFixed(1) + " kW"; | ||
| } | ||
| return Math.round(w) + " W"; | ||
| return Math.round(abs) + " W"; | ||
| } |
There was a problem hiding this comment.
When a battery undershoots its dispatch target, the tracking-deviation row still calls formatW(dev), which now discards the sign without adding a direction word or mark. For example, a 2 kW charging target with actual power of either 1 kW or 3 kW now produces the same Δ 1.0 kW, hiding whether the device is below or above its target. Update this caller to express the deviation direction explicitly, such as “below target” or “above target”, while retaining magnitude-only power formatting.
Useful? React with 👍 / 👎.
|
Back to draft after review. The arrows are being replaced with words (import/export, charge/discharge), as the phone app uses: the Flow hero's daily line already uses ↓ for import, the opposite of this PR. The review also found three missed spots that get fixed in the same pass: the Flow hero's signed battery kW (a paired web app copy follows), the V2X card's direction, and the per-phase watts. The after shots attached so far show the arrow version. |
Review of the first pass: arrows needed a key, and the grid's ↑ for import contradicted the Flow hero, which already uses ↓ for import on its daily line. The phone app uses words, so the box page does too. - web/power-words.js holds the one rule: a size, plus "import" or "export" for the grid and "charge" or "discharge" for a battery or a two-way charger, none within 10 W of zero. index.html loads it before app.js; plan-brief.js and plan.js import it for its side effect. - Live strip (word on its own line), battery and grid targets, the grid target slider, tooltips, V2X status, Power and DC rows, driver battery and meter rows, dispatch targets, per-phase watts under the fuse bars and the plan's safety note use it. A target that goes the same way as its reading shows only its size. - The tracking deviation says "300 W short", "300 W over" or "on target", by size. - Each chart names both sides of zero beside its zero line, on a backing in the panel colour: above, what adds to the grid meter's reading (import · charge · home · car); below, what takes away from it (export · discharge · solar), for the parts each chart draws. - Flow hero: fmtKw prints the size; the battery bubble keeps the readings' own "charging" or "discharging" under it. The target it used to show there did not fit beside that word. - Tests run the dashboard's strip, target, tooltip and driver row functions on both signs, find functions by name rather than by what follows them, fail if a signed grid or battery value goes back to a bare size, and check the sign of every chart's fillText. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
frahlg
changed the title
|
Reworked in b4d2488: words instead of arrows, the Flow hero fixed (paired with srcfl/ftw-webapp#113), plus the V2X rows, per-phase watts, and the short/over tracking text. The description is updated. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b4d2488d41
ℹ️ 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".
| var gap = Math.abs(Number(actualW) || 0) - Math.abs(Number(targetW) || 0); | ||
| if (Math.abs(gap) <= QUIET_W) return "on target"; | ||
| return formatW(gap) + (gap < 0 ? " short" : " over"); |
There was a problem hiding this comment.
When measured battery power and the dispatch target have opposite signs, subtracting their magnitudes understates the deviation and can report success: trackingText(-2000, 2000) returns on target even though the battery is discharging against a charging command, a 4 kW discrepancy. The new magnitude subtraction introduces this opposite-direction failure despite fixing the earlier same-direction shortfall display. Preserve direction when comparing actual power with the target, then format the resulting discrepancy as a magnitude with appropriate wording so the driver card does not describe reversed operation as following the command.
AGENTS.md reference: AGENTS.md:L22-L23
Useful? React with 👍 / 👎.
|
Draft again: the second review found that the tracking text ignores sign (charging 1.3 kW against a 1.0 kW discharge target reads "300 W over"), and that a negative Home use, which Core shows on purpose, now reads as ordinary use. Fixes are coming in the next commit. |
Second review round on the no-raw-minus change. - trackingText measures along the target's direction, so a battery charging 1.3 kW against a 1.0 kW discharge target is "2.3 kW short". The driver card's warning colour uses the same gap. - Home use keeps Core's sign in the flow hub, the Values card, the live strip, the live and forecast tooltips until a word is chosen (#1768). The flow's planets print sizes. - An observed battery keeps its direction word; "observe only" moves to the title's second line ("observed" on a phone), because both do not fit one line of the bubble. - The plan draws its side words after the bars and the SoC line, and says "load" instead of "home". Its grid tooltip row and the charger card's battery column use the words and the 10 W rule. - The slot badge uses the flow's arrows: down import, up export. - app.js falls back to sizes if power-words.js fails to load; the unused formatSignedWh and batterySub are gone. - Tests: opposite-sign tracking, the forecast row on both signs, a ban on hand-made "W" strings for signed values, and a check that knows every fillText on the charts and why none starts with a sign. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
miravoss26
left a comment
miravoss26
left a comment
There was a problem hiding this comment.
Reviewed the full diff and power-words.js's logic against its own test file.
What it does: replaces every raw-minus power display (charts, tooltips, readouts, the flow hero) with a size plus a direction word (import/export, charge/discharge), via one shared helper (power-words.js). Home use is the deliberate exception, keeps its sign per #1768.
Correctness: traced formatTarget, trackingGap and trackingText by hand against all the cases in no-raw-minus.test.mjs, including the sign-mismatch and near-zero edge cases (trackingGap's sign(target)*(actual-target) branch vs the |actual|-|target| fallback when the target is near zero). All check out. The formatSignedW(-0.3) -> "0 W" (not "-0 W") edge case is handled correctly. The removed formatOptionalW's one call site got an equivalent inline replacement, no behavior change there.
Test coverage is unusually tight for a UI PR: the fillText allowlist (the "knows every text a chart draws" test) means any new canvas text has to justify itself or the test fails, so this isn't just testing today's call sites, it's locking the property in going forward.
Security/boundary screen: no new dependencies, no new network destinations, no secrets, no injection surface, existing escapeHtml() calls preserved where user-adjacent strings go into innerHTML.
The diff matches what the body describes, nothing hiding outside it. Pixel-fit concerns the body itself raises (side-word backing panels overlapping chart data at narrow widths, the hero's target line being dropped for space) are real visual judgment calls, not bugs I can verify by reading code, so I'd weight the PR's own "needs a human look before merge" note rather than wave past it.
Safe to merge from my read, pending that visual check.
Fredrik said yes on 26 October 2026. FTW's rule is that the UI never shows a raw minus: it shows a magnitude, with the direction in a word. The box page broke that rule in several places:
formatWprinted "−1.2 kW");"-" + … + "kW");How direction is shown
Words, as the phone app does. Only four are used: "import" and "export" for the grid, and "charge" and "discharge" for the battery and V2X. Within 10 W of zero there is no word.
web/power-words.js:formatPower(w, kind)gives a size plus its word.formatTargetadds the target's word only when the target goes the other way from the reading.trackingTextgives "300 W short", "300 W over" or "on target", measured along the target's direction: charging 1.3 kW against a 1.0 kW discharge target reads "2.3 kW short".SIDESanddrawSideshold the side words.app.js,plan.jsandplan-brief.jsall read it.aggregateGroupsstill sums the signedkw. An observed battery says so on its title's second line ("OBSERVE ONLY" on desktop, "OBSERVED" on a phone), because "observe only · discharging" does not fit the bubble.Choice to look at
Left, and why
settings/tabs/devices.js), and the diagnose decision table.Pair
The Flow hero is vendored into the web app with a digest: srcfl/ftw-webapp
hero-no-minus(PR linked below). Merge both together, and re-copy if this branch changes before merge.Contract-pair: srcfl/ftw-webapp@0a80d02
Evidence
make verifyis clean.web/no-raw-minus.test.mjs:updateLiveStat,batteryTargetLine,tooltipPowerText,forecastRowTextandbatteryRowPartson both signs;formatWorMath.round(…) + " W"on a signed grid or battery value;fillTextinapp.js,plan.js,diagnose.jsandpower-words.js, fails on a new one until it has a reason, and fails on any literal that starts with a sign (the plan's lowest price is the one value allowed one);fmtKw, observe-only, the slot badge, the charger plan table, and the fallback to sizes whenpower-words.jsfails to load.git merge-treeagainst feat(web): the charts take the iOS app's palette #1859 (web-ios-charts@ 928c2d1): clean, and web tests on the merged tree pass 918/918. No new conflicts with Add EV charging energy and costs for day, week and month #1547, feat(history): show EV charging energy and cost for 7 and 30 days #1546, feat(modbus): share one TCP session per device and proxy it #1003, fix(charging): carry two-phase cars through plans, current limits and Easee #1792 or fix(control): fuse protection as one function #1789./api/configwas never called. They show the Overview flow hero, Energy live with tooltip, Plan, the driver card and the fuse.plan-side-words-r3,driver-row-opposite-sign-r3("1.3 kW charge → 1.0 kW discharge, short" in the warning colour) andhero-observe-only-*-r3.The box page needs a human look before merge (AGENTS.md).
🤖 Generated with Claude Code