Uh oh!
There was an error while loading. Please reload this page.
Refactor DeviceAttribute generics to eliminate "as T" assertions - #110
Open
heavyrubberslave wants to merge 4 commits into
Open
Refactor DeviceAttribute generics to eliminate "as T" assertions#110heavyrubberslave wants to merge 4 commits into
heavyrubberslave wants to merge 4 commits into
Conversation
Split DeviceAttribute's single generic T into two: V (concrete value kind, e.g. boolean/string/Int/Float) and T (V | undefined, tracks presence). fromString()/isValidValue() now return/narrow the concrete V instead of the abstract T, which TypeScript can prove sound without "as T" assertions. Removes the "as T" assertions (and their eslint-disable comments) from BoolDeviceAttribute, StrDeviceAttribute, IntDeviceAttribute, FloatDeviceAttribute and IntRangeDeviceAttribute. IntRangeDeviceAttribute now also uses Int.from() in fromString(), fixing a separate bug where a plain unbranded number was laundered into the branded Int type. ListDeviceAttribute's two "as IKey" assertions remain, since its value kind is chosen by the caller per instance rather than fixed per subclass - documented as expected in the issue. Closes#107
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughThe attribute hierarchy now separates value types from initialization state. Initialized attributes expose defined values through protocol APIs. Parsing and validation methods return concrete value types. Protocol type tests and unit fixtures now use initialized attributes and verify runtime guards. ChangesAttribute State Refactor
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
…ds V | undefined Replace the T extends V | undefined split from the previous commit with the stricter presence-flag design also proposed in issue #107: DeviceAttribute<V, IsSet> now uses a boolean IsSet parameter, and the storage/getter/setter type is computed as `IsSet extends true ? V : V | undefined` (AttributeStorage<V, IsSet>). This closes the residual gap of the simpler split, where nothing prevented T from degenerating to exactly `undefined`. Every Initialized*DeviceAttribute alias now means <..., true> instead of <V> (e.g. InitializedBoolDeviceAttribute = BoolDeviceAttribute<true>), and DeviceAttribute.hasValue() narrows to `this is { value: V }` instead of `this is { value: T }`. Because TypeScript treats two differently-parameterized instantiations of the same generic class as mutually non-assignable once a conditional type makes a parameter measured-invariant, several consumer type declarations that always construct attributes via createInitialized() had to be updated from the bare (unset-only) attribute type to the Initialized variant to keep compiling; this also makes their "always has a value" contract explicit at the type level: - EStim2bDeviceAttributes (estim2bDevice.ts) - all fields are always constructed with a value in estim2bDeviceFactory.ts - ButtplugIoDeviceAttributes (buttplugIoDevice.ts) - all attributes are always constructed with a value in buttplugIoDeviceFactory.ts - PiperVirtualDeviceAttributes.queuing, TtsVirtualDeviceAttributes.speaking/ queuing/queueLength - always constructed via createInitialized() - Zc95DevicePatternAttributes - always constructed via createInitialized() in getAttributesFromPatternDetails() Zc95DevicePowerChannelAttributes keeps its bare (possibly-unset) type since those attributes are genuinely constructed unset and only get a value once a power status message is received; Zc95Device.allPowerChannelValuesDefined() now narrows via an intersection type (`Attr & { value: Int }`) rather than the Initialized alias, since a type predicate's type must stay assignable to its original parameter type. Updated tests accordingly: - estim2bDevice.spec.ts / buttplugIoDevice.spec.ts test attribute factories now use createInitialized() to match the stricter production types - buttplugIoDevice.spec.ts's "undefined value" test now goes through the untyped AnyDevice interface, since the typed setAttribute() signature no longer accepts undefined for these attributes (the runtime guard in buttplugIoDevice.ts is still reachable via that untyped boundary, e.g. automation scripts) - zc95Device.spec.ts's power channel test helper now constructs unset attributes and assigns .value afterward, matching production - tests/type/*.test-d.ts updated to expect the now-narrower (non-undefined) setAttribute() return/parameter types for initialized attributes
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/device/attribute/intRangeDeviceAttribute.ts`:
- Line 8: Override isValidValue() in IntRangeDeviceAttribute so it returns
Number.isInteger(value), preserving the runtime Int invariant and preventing
fractional values from reaching createPatternMinMaxChange().
In `@src/device/attribute/numberDeviceAttribute.ts`:
- Line 31: Make NumberDeviceAttribute.isValidValue abstract instead of accepting
every JavaScript number, then implement concrete validation in the Int and Float
subclasses: require Number.isInteger() for Int and Number.isFinite() for Float,
while preserving type narrowing. Add runtime coverage through AnyDevice to
verify invalid numeric values cannot be written via setAttribute().
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ea0d3b1c-aff2-4456-8cdd-a599d8544f17
📒 Files selected for processing (20)
src/device/attribute/boolDeviceAttribute.tssrc/device/attribute/deviceAttribute.tssrc/device/attribute/floatDeviceAttribute.tssrc/device/attribute/intDeviceAttribute.tssrc/device/attribute/intRangeDeviceAttribute.tssrc/device/attribute/listDeviceAttribute.tssrc/device/attribute/numberDeviceAttribute.tssrc/device/attribute/strDeviceAttribute.tssrc/device/protocol/buttplugIo/buttplugIoDevice.tssrc/device/protocol/estim2b/estim2bDevice.tssrc/device/protocol/virtual/audio/piperVirtualDeviceLogic.tssrc/device/protocol/virtual/audio/ttsVirtualDeviceLogic.tssrc/device/protocol/zc95/zc95Device.tstests/type/device/buttplugIo.test-d.tstests/type/device/estim2b.test-d.tstests/type/device/virtual/virtual.test-d.tstests/type/device/zc95/zc95.test-d.tstests/unit/device/protocol/buttplugIo/buttplugIoDevice.spec.tstests/unit/device/protocol/estim2b/estim2bDevice.spec.tstests/unit/device/protocol/zc95/zc95Device.spec.ts
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
…rename IsSet to IsInitialized AttributeValue was a flat, non-generic type (BaseAttributeValue | undefined) used only as the loose/untyped value type at the AnyDevice erasure boundary (device.ts, scriptVmFactory.ts). AttributeStorage<V, IsSet> was the generic conditional type computing an attribute's concrete storage/getter/setter type. Since AttributeStorage<V, IsSet> defaults to AllowedAttributeType | undefined when called with no type arguments, it can serve both roles - merged them under the AttributeValue name and dropped AttributeStorage and the now-redundant BaseAttributeValue (renamed to AllowedAttributeType). Also renamed the IsSet type parameter to IsInitialized throughout, matching the existing Initialized*DeviceAttribute naming convention. No behavioral change; bare `AttributeValue` still resolves to `AllowedAttributeType | undefined`, identical to the previous flat type.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/device/attribute/numberDeviceAttribute.ts (1)
31-33:⚠️ Potential issue | 🟠 MajorKeep the numeric validation fix open.
At Line [31],
typeof value === 'number'does not provevalue is V. The guard accepts fractional values,NaN, andInfinityforInt. It acceptsNaNandInfinityforFloat.IntRangeDeviceAttributeinherits the same guard.Make
NumberDeviceAttribute.isValidValue()abstract. ImplementNumber.isInteger()inIntDeviceAttributeandIntRangeDeviceAttribute. ImplementNumber.isFinite()inFloatDeviceAttribute. Add runtime tests for the untyped validation path.This is the same unresolved finding as the previous review comment.
Proposed fix
- public override isValidValue(value: unknown): value is V {- return typeof value === 'number';- }+ public abstract override isValidValue(value: unknown): value is V;Add concrete validators to the numeric subclasses:
publicoverrideisValidValue(value: unknown): valueisInt{returntypeofvalue==='number'&&Number.isInteger(value);}publicoverrideisValidValue(value: unknown): valueisFloat{returntypeofvalue==='number'&&Number.isFinite(value);}#!/bin/bashset -euo pipefail rg -n -C 4 \ 'class (Int|Float)DeviceAttribute|isValidValue|Number\.isInteger|Number\.isFinite' \ src/device/attribute🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/device/attribute/numberDeviceAttribute.ts` around lines 31 - 33, Make NumberDeviceAttribute.isValidValue abstract so numeric subclasses must define type-appropriate validation. Implement integer-and-number checks with Number.isInteger in IntDeviceAttribute and IntRangeDeviceAttribute, and finite-number checks with Number.isFinite in FloatDeviceAttribute. Add runtime tests covering the untyped validation path, including fractional values, NaN, and Infinity.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@src/device/attribute/numberDeviceAttribute.ts`:
- Around line 31-33: Make NumberDeviceAttribute.isValidValue abstract so numeric
subclasses must define type-appropriate validation. Implement integer-and-number
checks with Number.isInteger in IntDeviceAttribute and IntRangeDeviceAttribute,
and finite-number checks with Number.isFinite in FloatDeviceAttribute. Add
runtime tests covering the untyped validation path, including fractional values,
NaN, and Infinity.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0261ca7e-0cad-41ee-9b94-481d7b602702
📒 Files selected for processing (8)
src/device/attribute/boolDeviceAttribute.tssrc/device/attribute/deviceAttribute.tssrc/device/attribute/floatDeviceAttribute.tssrc/device/attribute/intDeviceAttribute.tssrc/device/attribute/intRangeDeviceAttribute.tssrc/device/attribute/listDeviceAttribute.tssrc/device/attribute/numberDeviceAttribute.tssrc/device/attribute/strDeviceAttribute.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- src/device/attribute/intRangeDeviceAttribute.ts
- src/device/attribute/strDeviceAttribute.ts
- src/device/attribute/boolDeviceAttribute.ts
- src/device/attribute/intDeviceAttribute.ts
- src/device/attribute/listDeviceAttribute.ts
- src/device/attribute/floatDeviceAttribute.ts
…Value NumberDeviceAttribute.isValidValue() only checked typeof value === 'number', so a fractional value passed isValidValue() for Int attributes, and NaN/ Infinity passed for both Int and Float attributes - despite the isValidValue predicate now claiming `value is V` (Int/Float) after the presence-flag refactor. These invalid values can reach hardware protocol commands (e.g. Zc95's createPatternMinMaxChange, EStim2b's power/pulse commands, buttplug.io's scalar writes) via untyped callers of setAttribute() - the HTTP PATCH endpoint and automation scripts. Removed the shared loose check from NumberDeviceAttribute (now abstract again, inherited from DeviceAttribute) and added concrete overrides: - IntDeviceAttribute / IntRangeDeviceAttribute: Number.isInteger(value) - FloatDeviceAttribute: Number.isFinite(value) (matches Float.from()'s own NaN/Infinity rejection) Added unit tests for isValidValue on all three classes covering integers, fractional numbers, NaN, Infinity, and non-number values. Addresses CodeRabbit review comments on PR #110.
heavyrubberslave
commented
Aug 11, 2026
MemberAuthor
@coderabbitai review |
✅ Action performedReview finished.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes#107
Summary
DeviceAttribute<T>used a single genericTfor both "what kind of value" (e.g.boolean) and "is it possibly unset" (| undefined). BecauseTcould theoretically be instantiated narrower than the concrete kind,fromString()couldn't return a plainboolean/string/Int/Floatwithout an unsafeas Tassertion.This implements the presence-flag solution from the issue (the stricter of the two proposed designs):
DeviceAttribute<V, IsSet>splits the value kind (V, fixed per subclass) from a booleanIsSetflag, and computes the storage/getter/setter type asIsSet extends true ? V : V | undefined(AttributeStorage<V, IsSet>).fromString()/isValidValue()operate on the concreteV, which TypeScript can verify without assertions. Unlike the simplerT extends V | undefinedsplit, this closes the gap where nothing preventedTfrom degenerating to exactlyundefined.Changes
deviceAttribute.ts– base class takes<V, IsSet>now; addedBaseAttributeValue(concrete kinds, noundefined) andAttributeStorage<V, IsSet>;AttributeValuekeeps the same public meaning as before;hasValue()narrows tothis is { value: V }boolDeviceAttribute.ts,strDeviceAttribute.ts–fromString/isValidValuereturn the concrete type directly, no assertions;Initialized*aliases now mean<true>numberDeviceAttribute.ts– shared abstract base for int/float, generic overV extends Int | FloatintDeviceAttribute.ts,floatDeviceAttribute.ts–fromStringreturnsInt.from(num)/Float.from(num)directlyintRangeDeviceAttribute.ts– same, and fixes a separate pre-existing bug whereparseInt()'s plainnumberwas laundered into the brandedInttype viaas T; now usesInt.from(res)listDeviceAttribute.ts– mechanical update to the new base signature; its twoas IKeyassertions remain, since the value kind is chosen by the caller per instance rather than fixed per subclass (documented in the issue as an expected, separate limitation)Since TypeScript treats two differently-parameterized instantiations of the same generic class as mutually non-assignable once a conditional type makes a parameter measured-invariant, several consumer type declarations that always construct attributes via
createInitialized()had to move from the bare (unset-only) attribute type to theInitialized*variant — this also makes their "always has a value" contract explicit at the type level:EStim2bDeviceAttributes– all fields always constructed with a value inestim2bDeviceFactory.tsButtplugIoDeviceAttributes– all attributes always constructed with a value inbuttplugIoDeviceFactory.tsPiperVirtualDeviceAttributes.queuing,TtsVirtualDeviceAttributes.speaking/queuing/queueLength– always constructed viacreateInitialized()Zc95DevicePatternAttributes– always constructed viacreateInitialized()Zc95DevicePowerChannelAttributeskeeps its bare (possibly-unset) type since those attributes are genuinely constructed unset and only get a value once a power status message is received;Zc95Device.allPowerChannelValuesDefined()narrows via an intersection type (Attr & { value: Int }) rather than theInitializedalias, since a type predicate's type must stay assignable to its original parameter type.Tests updated accordingly (factories now use
createInitialized()to match the stricter production types; one buttplug.io test goes through the untypedAnyDeviceinterface to still exercise the runtimeundefined-guard, which stays reachable via that erasure boundary;tests/type/*.test-d.tsupdated to expect the now-narrowersetAttribute()types for initialized attributes).Verification
npm run typecheck– passesnpm run lint– passes (allconsistent-type-assertions/no-unsafe-type-assertiondisables gone from bool/str/int/float/intRange; list's two remain as expected)npm test– 449/449 tests passSummary by CodeRabbit
Improvements
undefinedhandling.Tests