Grid Charge UI: toggle limit

[naltatis]

fixes #24949

Add checkbox to quickly disable/enable price/co2 limit on loadpoint or battery while keeping last limit.

• ☑️ toggle limit with on click
• 💾 save last limit for quick enable
• 🚘🔋 applies to battery and loadpoint

Todos

• adjust e2e tests
• verify smart feedin priority

Constraints: The last selected value is kept in browser. So disabled limit values wont sync across devices.

[sourcery-ai]

Hey there - I've reviewed your changes and they look great!

Prompt for AI Agents

Please address the comments from this code review:

## Individual Comments

### Comment 1
<location> `assets/js/settings.ts:52-54` </location>
<code_context>
   };
 }

+function readNumber(key: string) {
+  return read(key) ? parseFloat(read(key)) : undefined;
+}
</code_context>

<issue_to_address>
**suggestion:** readNumber may return NaN if localStorage value is not a valid number.

Add a check to return undefined if parseFloat returns NaN, ensuring only valid numbers are returned.

```suggestion
function readNumber(key: string) {
  const value = read(key);
  if (!value) return undefined;
  const num = parseFloat(value);
  return isNaN(num) ? undefined : num;
}
```
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[andig]

Shouldn't we use the same "active" metapher as in the planner?

[naltatis]

What do you mean with metaphor? Since it's a simpler use-case here (just enabling/disabling a value) I opted to go with a compact solution. The interaction should be self-explanatory even with an "active" label.

[naltatis]

Or do you mean the toggle-switch component instead of a checkbox? I haven't tried that. Would also be a good option since larger click target.

[andig]

Genau

[naltatis]

Converted to switch ✨

switch.webm

[sourcery-ai]

Hey there - I've reviewed your changes - here's some feedback:

• The lastLimit prop defaults to 0, which will preselect a zero limit on first activation—consider using null or undefined as the default to avoid unintentionally forcing a zero limit.
• The activeLabel computed property in SmartTariffBase is defined but not used in the template; either render it to improve accessibility or remove it for clarity.
• The saveLastLimit logic is duplicated across SmartCostLimit and SmartFeedInPriority components—consider extracting it into a shared composable or mixin to reduce repetition.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- The `lastLimit` prop defaults to 0, which will preselect a zero limit on first activation—consider using null or undefined as the default to avoid unintentionally forcing a zero limit.
- The `activeLabel` computed property in SmartTariffBase is defined but not used in the template; either render it to improve accessibility or remove it for clarity.
- The `saveLastLimit` logic is duplicated across SmartCostLimit and SmartFeedInPriority components—consider extracting it into a shared composable or mixin to reduce repetition.

## Individual Comments

### Comment 1
<location> `assets/js/settings.ts:52-54` </location>
<code_context>
   };
 }

+function readNumber(key: string) {
+  return read(key) ? parseFloat(read(key)) : undefined;
+}
</code_context>

<issue_to_address>
**suggestion (bug_risk):** readNumber may return NaN if the stored value is not a valid number.

Add a check to return undefined if parseFloat returns NaN, preventing unintended propagation of NaN values.

```suggestion
function readNumber(key: string) {
  const value = read(key);
  if (value === undefined || value === null) return undefined;
  const num = parseFloat(value);
  return isNaN(num) ? undefined : num;
}
```
</issue_to_address>

### Comment 2
<location> `tests/smart-cost.spec.ts:43-44` </location>
<code_context>
     await expectModalVisible(modal);

     await modal.getByRole("link", { name: "Grid charging" }).click();
+    await modal.getByLabel("Enable limit").check();
     await modal.getByLabel("CO₂ limit").selectOption({ label: "≤ 150 g/kWh" });
+    await expect(modal.getByTestId("active-hours")).toHaveText(["Active time", "48 hr"].join(""));
</code_context>

<issue_to_address>
**suggestion (testing):** Missing test for toggling the limit off and restoring the last value.

Add a test to verify that disabling and re-enabling the limit restores the previous value, confirming correct persistence in local storage.
</issue_to_address>

### Comment 3
<location> `tests/smart-cost.spec.ts:14-17` </location>
<code_context>
 } from "./simulator";

 test.use({ baseURL: baseUrl() });
+test.describe.configure({ mode: "parallel" });

-test.beforeAll(async () => {
</code_context>

<issue_to_address>
**suggestion (testing):** Parallel test mode enabled; consider potential state leakage.

Ensure tests are isolated by clearing localStorage between runs to prevent flaky results from shared state.

```suggestion
test.beforeEach(async ({ page }) => {
  await page.evaluate(() => localStorage.clear());
  await startSimulator();
  await start(simulatorConfig());
});
```
</issue_to_address>

### Comment 4
<location> `tests/smart-feedin.spec.ts:32-34` </location>
<code_context>
     await expectModalVisible(modal);

     await modal.getByRole("link", { name: "Grid charging" }).click();
+    await modal.getByLabel("Enable limit").check();
     await modal.getByLabel("CO₂ limit").selectOption({ label: "≤ 150 g/kWh" });
+    await expect(modal.getByTestId("active-hours")).toHaveText(["Active time", "48 hr"].join(""));
</code_context>

<issue_to_address>
**suggestion (testing):** Missing test for restoring last feed-in limit value after toggling.

Add a test that disables and then re-enables the feed-in limit, confirming the last selected value is restored. This will fully cover the UI and localStorage behavior.
</issue_to_address>

### Comment 5
<location> `tests/battery-settings-co2.spec.ts:22-24` </location>
<code_context>
     await expectModalVisible(modal);

     await modal.getByRole("link", { name: "Grid charging" }).click();
+    await modal.getByLabel("Enable limit").check();
     await modal.getByLabel("CO₂ limit").selectOption({ label: "≤ 150 g/kWh" });
+    await expect(modal.getByTestId("active-hours")).toHaveText(["Active time", "48 hr"].join(""));
     await expect(modal).toContainText("20 g – 150 g");
     await page.getByRole("button", { name: "Close" }).click();
</code_context>

<issue_to_address>
**suggestion (testing):** Missing test for toggling CO₂ limit and restoring last value.

Add a test that disables and then re-enables the CO₂ limit, confirming the last selected value is restored to ensure correct persistence behavior.
</issue_to_address>

### Comment 6
<location> `tests/battery-settings.spec.ts:50-52` </location>
<code_context>
     await expectModalVisible(modal);

     await modal.getByRole("link", { name: "Grid charging" }).click();
+    await modal.getByLabel("Enable limit").check();
     await modal.getByLabel("CO₂ limit").selectOption({ label: "≤ 150 g/kWh" });
+    await expect(modal.getByTestId("active-hours")).toHaveText(["Active time", "48 hr"].join(""));
     await expect(modal).toContainText("20 g – 150 g");
     await page.getByRole("button", { name: "Close" }).click();
</code_context>

<issue_to_address>
**suggestion (testing):** Missing test for toggling price limit and restoring last value for battery.

Add a test that toggles the price limit off and on, verifying that the previous battery value is restored.
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}