Conversation
4021ca6 to
266d65d
Compare
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe PR introduces a new preset structuring system by adding Changes
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ 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. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/companion-module-host/src/instance.ts (1)
287-295:⚠️ Potential issue | 🔴 CriticalPre-existing:
this.destroy()looks like it recurses infinitelyHey, this isn't from your changes, but I noticed
await this.destroy()on line 291 calls theInstanceWrapper.destroy()method itself — likely should beawait this.#instance.destroy(). This would cause a stack overflow when called. Might be worth fixing while you're in the neighborhood! 😊🐛 Suggested fix
async destroy(): Promise<void> { await this.#lifecycleQueue.add(async () => { if (!this.#initialized) throw new Error('Not initialized') - await this.destroy() + await this.#instance.destroy() this.#initialized = false }) }
🧹 Nitpick comments (2)
packages/companion-module-base/src/module-api/preset/definition.ts (1)
7-7: Consider usingimport typeforCompanionActionSchema
CompanionActionSchemaappears to only be used in type positions (generic constraints). Usingimport typewould make that intent clearer and ensure it's erased at compile time. No big deal though!-import { CompanionActionSchema } from '../action.js' +import type { CompanionActionSchema } from '../action.js'packages/companion-module-host/src/internal/presets.ts (1)
11-16:structureparameter currently unused, but validation idea is worth consideringThanks for the contribution! I noticed the
structureparameter was added to the function signature but isn't actually used in the validation logic right now. SinceCompanionPresetSection.definitionscan containCompanionPresetReference[](which are preset ID strings), it could be handy down the road to validate that those references actually point to existing keys in thepresetsmap.That said, totally understand if that's being saved for a later iteration—just wanted to flag it in case it's helpful! The current validation logic for action and feedback IDs is looking solid. 🎉
bitfocus/companion#3931
The aim here is to formalise a slightly more hierarchical structure for presets.
Instead of a flat structure with a 'category' name set on each preset, this has changed to a lightly nested structure.
Instead of arbitrary 'text' presets, there is now a concept of groups instead. Each 'section' (previously category) can now contain either an array of groups or presets.
Local variables are supported! Currently limited to just 'user value' type.
There is a new concept of a 'matrix' group. Inspired a little by the github actions matrix syntax.
Instead of having to define the same preset multiple times with just changing an option to an action/feedback, some value substitution can be done. This is done via local-variables, with the matrix overriding the default value of those local variables
Types have been tweaked a little too, to tidy up a few patches of messiness that have crept in.
Summary by CodeRabbit