Expand CompanionActionDefinition type

[phillipivan]

Make CompanionActionDefinition a union of CompanionActionDefinitionWithSubscribeHooks and CompanionActionDefinitionWithoutSubscribeHooks. Discussed here on slack.

Thus when the subscribe callback is present, optionsToMonitorForSubscribe must be present. The purpose of this is to prompt developers to set this field during development, rather than waiting to find the current run time log messages warning the field is absent.

optionsToMonitorForSubscribe and skipUnsubscribeOnOptionsChange must be absent when the subscribe and unsubscribe callbacks are absent to avoid a confusing and pointless, but probably harmless configuration.

No longer relevant:

At the moment any combination of subscribe and unsubscribe callbacks are valid in the ...WithSubscribeHook type (ie either or both), however @Julusian indicated possible acceptability of mandating both or neither. In the past I have had use cases for the subscribe without unsubscribe such as in the ember plus module, but I can't think of a use case for an unsubscribe without a subscribe. So perhaps at least the subscribe should be required, which would simplify the type def a bit.

Summary by CodeRabbit

• Refactor
  • Reworked action definition structure to enforce clear variants for subscribe vs. non-subscribe behaviors.
  • Made action callbacks mandatory while keeping auxiliary settings optional.
  • Strengthened option-monitoring and subscription lifecycle rules to improve validation and reduce unexpected integration behavior.

[coderabbitai]

Warning

Rate limit exceeded

@Julusian has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 59 minutes before requesting another review.

To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 06788928-4794-4123-89ec-da890f41ba55

📥 Commits

Reviewing files that changed from the base of the PR and between 87fef27 and 1550358.

📒 Files selected for processing (1)

• packages/base/src/module-api/action.ts

📝 Walkthrough

Walkthrough

Refactors action typing: extracts shared fields into CompanionActionDefinitionBase (making callback required), introduces CompanionActionDefinitionSubscribeHooks (requires subscribe hooks) and CompanionActionDefinitionNoSubscribeHooks (forbids them), and redefines CompanionActionDefinition as a union of base+either variant.

Changes

Action type refactor

Layer / File(s)	Summary
Data Shape packages/base/src/module-api/action.ts	Introduce CompanionActionDefinitionBase<TOptions> (shared metadata; callback now required).
Subscribe Variants packages/base/src/module-api/action.ts	Add CompanionActionDefinitionSubscribeHooks<TOptions> (requires optionsToMonitorForSubscribe, subscribe; unsubscribe and skipUnsubscribeOnOptionsChange optional) and CompanionActionDefinitionNoSubscribeHooks (mapped optional never to forbid subscribe fields).
Public Type Composition packages/base/src/module-api/action.ts	Redefine exported CompanionActionDefinition<TOptions> as `CompanionActionDefinitionBase & (CompanionActionDefinitionSubscribeHooks

Poem

Types align like ships at tide,
Base holds truth, two paths divide,
One invites the subscribe song,
One says "no" and moves along,
Small refactor — tidy stride 🎈

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'Expand CompanionActionDefinition type' directly and clearly describes the main refactoring work—transforming the interface into a typed union/intersection model with new variants.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Review rate limit: 0/1 reviews remaining, refill in 59 minutes.}

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (1)

packages/base/src/module-api/action.ts (1)

99-99: Consider exporting CompanionActionDefinitionWithSubscribeHooks for symmetry 📦

Small thing — CompanionActionDefinitionWithoutSubscribeHooks is exported on line 84, but its sibling here is kept private. Module authors who want to type a helper or a factory specifically for the "with hooks" variant currently can't reference it by name. Exporting it would make the two branches of the union equally addressable, and matches what the PR title (“Expand CompanionActionDefinition type”) implies for the public API surface. Totally optional though — happy either way!

♻️ Suggested change

-type CompanionActionDefinitionWithSubscribeHooks<TOptions extends CompanionOptionValues = CompanionOptionValues> =
+export type CompanionActionDefinitionWithSubscribeHooks<TOptions extends CompanionOptionValues = CompanionOptionValues> =
 	CompanionActionDefinitionBase<TOptions> & {

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: ad05b4a2-5e3e-45d1-b728-fee273b5c9d6

📥 Commits

Reviewing files that changed from the base of the PR and between b4d202b and 7122962.

📒 Files selected for processing (1)

• packages/base/src/module-api/action.ts

[phillipivan]

After some more thought, I think at least the subscribe property should be mandatory, for the sake of simpler types, and because I can't think of an instance where you would want an unsubscribe without a subscribe; however I will wait until others have a chance to chime in before making further changes.

[jswalden]

If #217 is taken in some form, CompanionActionDefinition is going to grow increasingly complicated in the number of subtly different flavors it supports. Also, if this starts changing from an interface into an intersection type, the generated docs for it are going to get accordingly less readable for it.

It's entirely possible this is simply unavoidable.

Still, worth flagging to see if we can somehow manage to keep the complexity down, to the extent possible.

[phillipivan]

Making at least the subscribe property mandatory would make the ...withSubscribeHooks type much less complex,and I think would be my preference, which comes at the expense of sacrificing the current possibility of only having an unsubscribe flow. Or we just make both required as Julian alluded to, and if need be a stub can be defined by the developer.

[jswalden]

I have very little sense of how sub/unsub tends to be used. But I think it's quite reasonable to say if you want one, you have to do 'em both.

(The only module I control that uses subscribe/unsubscribe, does so essentially as hackaround to avoid creating a mass of variables up front. I would have no complaints about having to define a vacuous unsubscribe, if it came to it.)

[phillipivan]

From memory (ie, untrustworthy) the only module I've worked on with an unsubscribe flow for actions is the Qsys one. I find them a bit more common for feedbacks; especially if the module is checking feedbacks by id (which I've come to use fairly often of late because I prefer the development flow (rather than for performance gains)).

[phillipivan]

I've changed subscribe to be a required property in the CompanionActionDefinitionWithSubscribeHooks type. This sacrifices a little bit of the existing flexibility for a simpler and easier to understand type, which I think is a worthwhile trade off. If there is a genuine use case for an action with an unsubscribe flow, without a subscribe flow, a subscribe stub would need to be defined.

[Julusian]

@phillipivan I've pushed a commit to this branch, adjusting it to use composition instead of inheritence. The hope is to avoid naming many flavours once we add a second type of variation.

Let me know what you think, I'm open to reverting my change if you disagree.
It would be great if you can confirm it doesn't break your case, it seems to type correctly in my tests.

I havent checked (either version), but hopefully the typedoc will remain sane

[phillipivan]

Haven't tested but lgtm. The only downside I can see, which is more theoretical that anything, is if someone wanted to explicitly type an action definition with subscribe hooks that would be more fiddly (rather than just leaving it to the type system to figure it out). However given this is not an option anyone has had to date, I expect no one will think to use it or miss it.

[Julusian]

However given this is not an option anyone has had to date, I expect no one will think to use it or miss it.

yeah that is my hope. and I think the modularity will be a benefit to us, so if it discourages using specific interfaces in typing then that could also be a good thing