Skip to content

Conversation

@IHateToSelectAUsername
Copy link
Contributor

Fixes TypeScript type errors in knx-sync-state-selector-row component.

The issue:
ha-selector-select expects string values, but we were mixing booleans (true/false) with strings ("init", "expire", "every"), causing TypeScript errors.

The fix:

  • Convert booleans to strings internally ("true"/"false")
  • Map back to booleans when sending to backend
  • Backend API stays unchanged (still receives true/false as booleans)

Related to #157

before:
image
after:
image

@farmio farmio requested a review from Copilot October 23, 2025 04:21
@codecov
Copy link

codecov bot commented Oct 23, 2025

Codecov Report

❌ Patch coverage is 0% with 34 lines in your changes missing coverage. Please review.
✅ Project coverage is 3.36%. Comparing base (5556760) to head (f3ba2e8).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
src/components/knx-sync-state-selector-row.ts 0.00% 34 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##            main    #276      +/-   ##
========================================
- Coverage   3.36%   3.36%   -0.01%     
========================================
  Files         55      55              
  Lines      10150   10174      +24     
  Branches     156     156              
========================================
  Hits         342     342              
- Misses      9808    9832      +24     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR fixes TypeScript type errors in the knx-sync-state-selector-row component by ensuring all values passed to ha-selector-select are strings, while maintaining backward compatibility with the backend API that expects boolean values.

Key changes:

  • Convert boolean strategy values to string literals ("true"/"false") for frontend compatibility
  • Add bidirectional conversion between string and boolean representations
  • Update type annotations to reflect string-only internal handling

mode: "dropdown",
options: this._options,
mode: "dropdown" as const,
options: this._options as readonly string[],
Copy link

Copilot AI Oct 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Type assertion required here suggests a type mismatch. Consider updating _options getter return type to readonly string[] instead of using an assertion at the call site, making the type system work for you rather than against you.

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implemented. Changed to readonly string[] which removes the need for the type assertion.

@farmio
Copy link
Member

farmio commented Oct 23, 2025

Hi 👋!

Thank you very much for your contribution!
Feel free to ignore the code coverage tests - we don't really enforce that. For the AI review comments, please have a look - those seem legit to me, but I'm not very good at TS 😬

@farmio farmio requested a review from Copilot October 23, 2025 08:54
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.


// Validate strategy value before assignment
const validStrategies = ["true", "false", "init", "expire", "every"] as const;
if (validStrategies.includes(strategy as any)) {
Copy link

Copilot AI Oct 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using as any defeats TypeScript's type safety. Use a type predicate or assertion function instead: if (validStrategies.includes(strategy as (typeof validStrategies)[number]))

Copilot uses AI. Check for mistakes.
Copy link
Member

@farmio farmio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice 👍 Thank you!

@farmio farmio merged commit f1b11e8 into XKNX:main Oct 23, 2025
4 of 6 checks passed
@IHateToSelectAUsername IHateToSelectAUsername deleted the fix/sync-state-selector-types branch October 24, 2025 07:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants