refactor(!): improved type-narrowing when working with action settings (relates #133) - #164
Conversation
🦋 Changeset detectedLatest commit: 89ce484 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
Pull request overview
Refactors the plugin action type system to better enforce an Action<T>’s settings shape at setSettings call sites, and propagates explicit JsonObject typing through internal stores/controllers where the concrete settings type is unknown.
Changes:
- Tightens
Action.setSettingsto accept onlyT(removing the independent generic that allowed unrelated settings objects). - Removes default type parameters from
Action,DialAction, andKeyAction, and updates internal APIs/stores to useJsonObjectexplicitly where needed. - Updates action-service tests and adds changesets to ship the breaking type changes as a major version.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/plugin/src/plugin/ui.ts | Updates stored/returned action types to use `DialAction |
| packages/plugin/src/plugin/devices/device.ts | Tightens Device.actions iterator type to explicitly carry JsonObject settings. |
| packages/plugin/src/plugin/actions/store.ts | Makes the action store explicitly store `DialAction |
| packages/plugin/src/plugin/actions/key.ts | Removes default settings generic to require explicit settings typing. |
| packages/plugin/src/plugin/actions/dial.ts | Removes default settings generic to require explicit settings typing. |
| packages/plugin/src/plugin/actions/action.ts | Tightens setSettings to accept only T, improving type narrowing at call sites. |
| packages/plugin/src/plugin/actions/tests/service.test.ts | Updates casts to include explicit settings generic arguments. |
| .changeset/wild-bikes-vanish.md | Declares a major release note for the setSettings type-narrowing change. |
| .changeset/free-oranges-lose.md | Declares a major release note for required explicit settings typing. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.
Suppressed comments (2)
packages/plugin/src/plugin/actions/action.ts:112
setSettingsis now correctly constrained toT, butgetSettingsstill exposes an unconstrained generic (getSettings<U extends JsonObject = T>()), so callers can still bypassAction<T>by explicitly providing an unrelatedU. If the goal is to makeAction<T>actually enforce settings shape (per #133),getSettingsshould also be constrained to returnPromise<T>without a caller-supplied generic.
* @param value Settings to persist.
* @returns `Promise` resolved when the settings are sent to Stream Deck.
*/
public setSettings(value: T): Promise<void> {
settingsCache.delete(this.id);
packages/plugin/src/plugin/actions/action.ts:27
- Removing the default type parameter from
Action<T>makes bareActiontype references invalid. There is at least one remaining usage (packages/plugin/src/plugin/actions/__tests__/action.test.tshaslet action!: Action;) that will fail TypeScript compilation unless updated to include a concrete type argument (e.g.Action<JsonObject>).
export class Action<T extends JsonObject> extends ActionContext {
…ng, improve JSDocs.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 20 changed files in this pull request and generated 1 comment.
Suppressed comments (8)
packages/plugin/src/plugin/actions/singleton-action.ts:111
setTitleis implemented byKeyActionandDialAction, notActionBase, so this newly targeted API link cannot resolve to a real member.
* Occurs when the user updates an action's title settings in the Stream Deck application. See also {@link ActionBase.setTitle}.
packages/plugin/src/plugin/actions/dial.ts:58
setFeedbackis declared onDialAction, notActionBase, so this API link cannot resolve correctly.
* Use in conjunction with {@link ActionBase.setFeedback} to update the layout's current items' settings.
packages/plugin/src/plugin/actions/action-base.ts:120
- The core regression is type-level, but the updated tests only exercise runtime forwarding with actions effectively typed as
JsonObject. A future reintroduction of a method-level generic would therefore pass them. Add a compile-time case using a concrete settings type that accepts its schema and marks an unrelated object with@ts-expect-error.
public setSettings(value: TSettings): Promise<void> {
packages/plugin/src/plugin/actions/action-base.ts:46
- This method no longer declares
U, so the@template Utag is stale; the trailingaction.Dis also erroneous. Document the class-level settings type through the return value instead.
* Gets the settings associated this action instance.
* @template U The type of settings associated with the action.D
* @returns Promise containing the action instance's settings.
packages/plugin/src/plugin/actions/singleton-action.ts:105
ActionBasehas nosendToPropertyInspectormethod; messages to the property inspector are sent bystreamDeck.ui. The current link points users to a nonexistent API.
This issue also appears on line 111 of the same file.
* Occurs when a message was sent to the plugin _from_ the property inspector. The plugin can also send messages _to_ the property inspector using {@link ActionBase.sendToPropertyInspector}.
packages/plugin/src/plugin/actions/service.ts:177
ActionBasedoes not definesetTitle; the operation belongs to the concrete key and dial action classes. Link those actual API members instead.
* Occurs when the user updates an action's title settings in the Stream Deck application. See also {@link ActionBase.setTitle}.
packages/plugin/src/plugin/actions/dial.ts:41
setFeedbackLayoutis declared onDialAction, notActionBase, so this link now targets a nonexistent member.
This issue also appears on line 58 of the same file.
* to users, and can be assigned in the manifest, or dynamically via {@link ActionBase.setFeedbackLayout}.
packages/plugin/src/plugin/actions/tests/action.test.ts:336
- This assertion invokes
showAlerton theKeyActiondeclared above;ActionBasehas no such method. The updated documentation link therefore points to a nonexistent member.
* Asserts {@link ActionBase.showAlert} forwards the command to the {@link connection}.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated 1 comment.
Suppressed comments (9)
Previously missed (8) — in code that hasn't changed since the last review.
packages/plugin/src/plugin/actions/action-base.ts:45
- This method no longer declares
U, so the@template Udocumentation is stale and describes the loophole this refactor removes. Document the action-bound settings type instead.
* Gets the settings associated this action instance.
* @template U The type of settings associated with the action.D
packages/plugin/src/plugin/actions/action-base.ts:48
- The core change is a compile-time contract, but the updated tests only exercise runtime behavior and the package build excludes
__tests__. Add a type-checked fixture that verifies unrelated values and explicit return-type overrides are rejected, while the declared settings type is accepted.
public async getSettings(): Promise<TSettings> {
.changeset/free-oranges-lose.md:5
NeoInfobarActionalso lost its default settings type and is publicly exported, so this breaking-change note omits one affected API.
`Action`, `KeyAction`, and `DialAction` types now require their setting type be defined.
packages/plugin/src/plugin/actions/singleton-action.ts:105
ActionBasehas nosendToPropertyInspectormethod; that API belongs tostreamDeck.ui, so this new link cannot resolve to the documented operation.
* Occurs when a message was sent to the plugin _from_ the property inspector. The plugin can also send messages _to_ the property inspector using {@link ActionBase.sendToPropertyInspector}.
packages/plugin/src/plugin/actions/singleton-action.ts:111
ActionBasedoes not definesetTitle; only concrete key and dial actions do. Remove this unresolved link from the public event documentation.
* Occurs when the user updates an action's title settings in the Stream Deck application. See also {@link ActionBase.setTitle}.
packages/plugin/src/plugin/actions/service.ts:177
ActionBasedoes not definesetTitle, so this link points to a nonexistent member. Keep the event description without that invalid cross-reference.
* Occurs when the user updates an action's title settings in the Stream Deck application. See also {@link ActionBase.setTitle}.
packages/plugin/src/plugin/actions/dial.ts:41
setFeedbackLayoutis declared onDialAction, notActionBase; the new link targets a nonexistent base-class member.
This issue also appears on line 58 of the same file.
* to users, and can be assigned in the manifest, or dynamically via {@link ActionBase.setFeedbackLayout}.
packages/plugin/src/plugin/actions/tests/action.test.ts:336
- This test invokes
showAlerton aKeyAction, whileActionBasehas no such method. The updated link therefore points to a nonexistent member.
* Asserts {@link ActionBase.showAlert} forwards the command to the {@link connection}.
packages/plugin/src/plugin/actions/dial.ts:58
setFeedbackis declared onDialAction, notActionBase; update the link so generated documentation resolves to the actual method.
* Use in conjunction with {@link ActionBase.setFeedback} to update the layout's current items' settings.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated no new comments.
Suppressed comments (4)
Previously missed (4) — in code that hasn't changed since the last review.
packages/plugin/src/plugin/actions/action-base.ts:47
- The JSDoc for
getSettingsstill mentions a genericUand contains a typo (action.D), but the method no longer has a generic parameter. This documentation is now incorrect and misleading.
/**
* Gets the settings associated this action instance.
* @template U The type of settings associated with the action.D
* @returns Promise containing the action instance's settings.
*/
packages/plugin/src/plugin/actions/neo-infobar.ts:11
DialActionandKeyActionuseTSettingsfor their settings type parameter, butNeoInfobarActionusesT. Aligning the generic parameter name improves consistency and readability across action types.
/**
* Provides a contextualized instance of an infobar action found on Stream Deck Neo.
* @template T The type of settings associated with the action.
*/
export class NeoInfobarAction<T extends JsonObject> extends ActionBase<T> {
packages/plugin/src/plugin/actions/singleton-action.ts:71
- Typo in JSDoc: "when the the settings" has a duplicated word.
* Occurs when the settings associated with an action instance are requested using {@link ActionBase.getSettings}, or when the the settings were updated by the property inspector.
packages/plugin/src/plugin/actions/service.ts:34
ActionBaseis imported as a value but not used anywhere in this module (it only appears in JSDoc). This can trigger unused-import errors in TS/lint configurations and also adds an unnecessary runtime import.
import { ActionBase } from "./action-base.js";
import type { Action } from "./action.js";
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated 2 comments.
Suppressed comments (5)
Previously missed (3) — in code that hasn't changed since the last review.
packages/plugin/src/plugin/actions/singleton-action.ts:105
ActionBasedoesn’t definesendToPropertyInspector(the API is onstreamDeck.ui). The JSDoc link currently points to a non-existent member.
This issue also appears on line 111 of the same file.
* Occurs when a message was sent to the plugin _from_ the property inspector. The plugin can also send messages _to_ the property inspector using {@link ActionBase.sendToPropertyInspector}.
packages/plugin/src/plugin/actions/dial.ts:42
setFeedbackLayoutis aDialActionmethod (not onActionBase), so this JSDoc link points to a non-existent member.
This issue also appears on line 56 of the same file.
* Sets the feedback for the current layout associated with this action instance, allowing for the visual items to be updated. Layouts are a powerful way to provide dynamic information
* to users, and can be assigned in the manifest, or dynamically via {@link ActionBase.setFeedbackLayout}.
*
packages/plugin/src/plugin/actions/action-base.ts:45
- Minor grammar: missing “with” in this sentence.
* Gets the settings associated this action instance.
* @returns Promise containing the action instance's settings.
packages/plugin/src/plugin/actions/singleton-action.ts:111
ActionBasedoesn’t definesetTitle; it’s implemented onKeyAction/DialAction. The JSDoc link should point to the actual API surface.
* Occurs when the user updates an action's title settings in the Stream Deck application. See also {@link ActionBase.setTitle}.
packages/plugin/src/plugin/actions/dial.ts:59
setFeedbackis aDialActionmethod (not onActionBase), so this JSDoc link currently points to a non-existent member.
/**
* Sets the layout associated with this action instance. The layout must be either a built-in layout identifier, or path to a local layout JSON file within the plugin's folder.
* Use in conjunction with {@link ActionBase.setFeedback} to update the layout's current items' settings.
* @param layout Name of a pre-defined layout, or relative path to a custom one.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated no new comments.
Suppressed comments (4)
Previously missed (4) — in code that hasn't changed since the last review.
packages/plugin/src/plugin/actions/singleton-action.ts:22
ActionBaseis only referenced in JSDoc in this file, so this type-only import is unused and is likely to fail strict linting (unused imports). Remove the import (or switch the JSDoc links to an import-qualified reference if you need ActionBase for documentation).
import type streamDeck from "../index.js";
import type { Action } from "../index.js";
import type { ActionBase } from "./action-base.js";
import { actionStore } from "./store.js";
packages/plugin/src/plugin/actions/singleton-action.ts:72
- The JSDoc links now reference
ActionBase.*, but@elgato/streamdeckonly exports./dist/plugin/index.js(package.jsonexports), and./actions/index.tsdoes not re-exportActionBase. This means consumers can’t importActionBasefrom the public entrypoint and generated docs may have broken links. Consider re-exportingActionBase(type-only is fine) from./actions/index.ts/ root index, or adjust the JSDoc links to reference methods on the concrete action types instead.
/**
* Occurs when the settings associated with an action instance are requested using {@link ActionBase.getSettings}, or when the settings were updated by the property inspector.
* @param ev Information about the event, including the source action and contextual payload information.
packages/plugin/src/plugin/actions/action-base.ts:44
- Minor grammar in the new JSDoc: “associated this action instance” should be “associated with this action instance”.
* Gets the settings associated this action instance.
packages/plugin/src/plugin/actions/action-base.ts:149
- Use strict equality when comparing event contexts.
ev.contextandthis.idare both expected to be strings, so===avoids accidental coercion and is consistent with typical TS style.
if (ev.context == this.id) {
getSettingsandsetSettingsto always use the type defined on the action instance.Action<T>to represent a union of possible action types, e.g. dial, key, Neo infobar, etc.ActionBase<T>to replace previousAction<T>(remove from export).Relates to: #133