Conversation
📝 WalkthroughWalkthroughPresets were reworked: conversion now returns both a presets Map and UI sections; controls and IPC incorporate a variablesHash for variable-driven variants; local variable overrides can be injected; web UI migrated from category lists to section/group/template rendering with fuzzy search and updated drag payloads. Changes
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
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 |
|
Maybe a good moment to bring back two long standing feature requests about presets:
So both of those changes would be completely backwards compatible and give a very easy upgrade path for module developers if they want to enhance preset organization. This would also bring the regular button presets to the same level of text presets but with less effort at declaration. |
With this change, it goes back to relying on the order the presets/sections/groups are provided in
This adds a single additonal level of groups, replacing the current text definiiton with the name+descritpion of a group. I realised that the text definition type needs to go away for searching to be sane, otherwise we will need to either discard the text nodes early (loosing the context), or many searches will be left with just the text nodes.
I almost did do an unlimited depth for these, but I dont think we actually want that from a UX perspective. I'm not sure that even the groups I have added would want to collapse, but maybe they could (perhaps automatically when they grow above some threshold of number of contents). I dont think that going further than that will be nice to use.
This is a decision to make here; Do we want something backwards compatible but harder to interpret and understand (both on companion and module dev side), or a breaking change that gives a chance to clean it up? The other idea I had was to basically don't touch the current structure (other than require an unique id on each preset), but to have a second optional parameter that defines the new structure, referencing the array of presets by id. Personally, I think that a bit of a breaking change would be beneficial. My hope is that by forcing devs to review their preset code a little, they are more likely to update it to newer functionality. Like the matrix group; which will cut down the volume of presets many provide dramatically. A good example of why is there was an issue a couple of years back with bmd-videohub where it kept crashing for 288x288 videohubs, because of the volume of presets. It was trying to produce 288x288x3. All those could now be replaced with just a handful |
Julusian
force-pushed
the
feat/presets-restructure
branch
from
87cdf3b to
63f460c
Compare
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
# Conflicts: # package.json # shared-lib/lib/ModuleApiVersionCheck.ts # webui/src/Buttons/Presets/PresetsConnectionList.tsx # yarn.lock
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
webui/src/Buttons/Presets/PresetDefinitionsStore.tsx (1)
33-41:⚠️ Potential issue | 🟠 MajorMobX reactivity gap with in-place patching on plain
RecordvaluesThanks for the thorough restructuring! 👍 I wanted to double-check one thing—the values stored in the observable map are plain
Recordobjects, and whenapplyJsonPatchInPlacemutates them in place, MobX won't detect those nested changes since plain objects aren't deeply observable. This means observer components reading these presets (likePresetsSectionsList) won't re-render after apatchupdate, even though the@actiondecorator wraps the method.The
initandaddcases trigger reactivity by calling.set()on the map, butpatchonly mutates the existing object reference without notifying observers.Could you verify that downstream consumers correctly re-render after patches? If they don't, you'll want to either:
- Replace the map entry after patching to trigger reactivity:
this.presets.set(update.connectionId, { ...currentPresets })- Or wrap stored values with
observable()so MobX tracks nested mutations
🧹 Nitpick comments (10)
companion/lib/Variables/Util.ts (1)
377-397: Looks solid! Just a friendly note about the mutation pattern. 🔧
injectOverriddenLocalVariableValuesmutateslocalVariable.options.startup_valuein place on the input array elements. This is fine if callers always pass cloned data (e.g., fromstructuredClone), but could surprise future callers who don't expect mutation. A brief JSDoc note mentioning "mutates the provided entities" would help keep things clear for other contributors.Otherwise, the filtering logic and value wrapping with
isExpressionOrValue/exprVallook correct. Nice work! 😊webui/src/Buttons/Presets/fuzzyMatch.ts (1)
1-23: Nice little utility — clean and focused! 👍A couple of small thoughts you might consider:
The
FUZZY_THRESHOLDof-5000is quite permissive. This is probably intentional to keep results forgiving for preset discovery, but it might be worth adding a brief comment explaining the reasoning so future contributors understand the choice.Minor thought: if
targetis an empty array,Array.some()returnsfalse, which seems like the right behavior — just noting it's handled correctly.Welcome addition for the presets search feature!
webui/src/Buttons/Presets/PresetSectionCollapse.tsx (1)
162-169: Minor: empty<p>rendered when onlynameis set anddescriptionis undefined.The parent condition
grp.name || grp.descriptioncan be true when onlynameis set, resulting in an empty<p></p>. Not a big deal visually, but you could conditionally render it:✨ Optional tweak
function PresetText({ grp }: Readonly<PresetTextProps>) { return ( <div className="mx-2 mt-2"> <h5>{grp.name}</h5> - <p>{grp.description}</p> + {grp.description && <p>{grp.description}</p>} </div> ) }webui/src/Buttons/Presets/PresetsSectionsList.tsx (3)
16-28:moduleInfoprop is declared but unusedJust a heads-up —
moduleInfois defined in thePresetsSectionsListPropsinterface (line 19) but isn't destructured or used anywhere in the component. If it's planned for future use, maybe a brief comment would help; otherwise, feel free to remove it to keep things tidy!
43-44: Consider passingkeywordsas an array tofuzzyMatchinstead of joiningThe
fuzzyMatchutility already supportsstring[]inputs (seefuzzyMatch.tslines 8-11), so joining keywords with' 'loses the per-keyword matching granularity. For instance, searching "foo" would also match a partial hit on "foobar" when keywords are joined, while array matching would test each keyword individually.This same pattern appears on lines 52, 58, 72, and 79.
💡 Suggested change (example for line 44)
- const sectionMatchesSearch = - !searchQuery || fuzzyMatch(searchQuery, section.name, section.description, section.keywords?.join(' ')) + const sectionMatchesSearch = + !searchQuery || fuzzyMatch(searchQuery, section.name, section.description, section.keywords)
38-103:allSectionsas auseComputeddependency defeats memoizationSince
allSectionsis recomputed inline every render (lines 32-34), it produces a new array reference each time, causinguseComputedto always recreate its computation. This isn't a bug — the filtering still works correctly — but it meansuseComputeddoesn't provide any caching benefit here.If you'd like to preserve memoization, you could either
useMemoforallSectionswith thepresetsreference as a dependency, or move the entire computation (sorting + filtering) insideuseComputed. Totally fine to leave as-is if perf isn't a concern at this scale though!companion/lib/Controls/Controller.ts (1)
721-728: Minor: unreachable null check afternewconstructorLine 728 (
if (!newControl) return null) is unreachable sincenew ControlButtonPreset(...)always returns an instance. Not harmful, but you could remove it to reduce noise.companion/lib/Instance/Definitions.ts (3)
323-341: Consider cloning the model whenvariableValuesis null too, for defensive consistency.When
variableValuesis non-null, you return a cloned model (Line 333-336). But on Line 331, returningdefinition.modeldirectly exposes the stored definition to mutation by callers. If this was the same behavior before, it's a pre-existing concern, but since you're already cloning in one branch, it might be worth being consistent.Not a blocker at all — just something to keep in mind if callers ever start mutating the returned model.
478-519: Inputpresetsmap values are mutated in-place before cloning — this may surprise callers.Lines 478–519 mutate properties on the preset objects inside the
presetsparameter (e.g.,preset.model.style.text,feedback.options,action.options). Then Line 532 stores astructuredClone.Since the parameter is typed
ReadonlyMap(implying no modification), the in-place mutation of the map's values can be surprising. When called fromsetPresetDefinitions, the caller's original data gets silently mutated. When called fromupdateVariablePrefixesForLabel, the previously-stored clone is mutated (less harmful since it's immediately replaced).A cleaner approach would be to clone before mutating, so the input is never touched:
💡 Suggested approach
+ const presetsClone = structuredClone(presets) + - for (const preset of presets.values()) { + for (const preset of presetsClone.values()) { if (preset.model.style) { preset.model.style.text = replaceAllVariables(preset.model.style.text, label, allowedSet)Then on Line 532:
- this.#presetDefinitions[connectionId] = structuredClone(presets) + this.#presetDefinitions[connectionId] = presetsCloneNot urgent if callers currently discard their references, but it'd be a nice safeguard against future surprises. 😊
546-549: Tiny nit:jsonPatch.comparealways returns an array, sodiff &&is redundant.
jsonPatch.comparereturnsOperation[]— it's never null/undefined. Thediff.length > 0check alone is sufficient. Totally harmless as-is though!
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
companion/lib/Controls/Controller.ts (1)
728-728: Tiny nit:newalways returns an instance (or throws)Line 728 (
if (!newControl) return null) is effectively dead code since callingnew ControlButtonPreset(...)will always return an object or throw an exception — it can never be falsy. Not a big deal at all, but figured I'd mention it in case you're tidying up! 😊webui/src/Buttons/Presets/PresetSectionCollapse.tsx (1)
162-169: Consider guarding against empty<h5>/<p>elements.The parent already checks
grp.name || grp.description, but insidePresetTextboth elements render unconditionally. If only one is set, you'd get an empty<h5>or<p>in the DOM — not a huge deal, but easy to tidy up if you'd like:💅 Optional cleanup
function PresetText({ grp }: Readonly<PresetTextProps>) { return ( <div className="mx-2 mt-2"> - <h5>{grp.name}</h5> - <p>{grp.description}</p> + {grp.name && <h5>{grp.name}</h5>} + {grp.description && <p>{grp.description}</p>} </div> ) }
2 participants
closes #2843
closes #3937
see bitfocus/companion-module-base#182
Allows for:
Screencast.From.2026-02-01.21-41-17.mp4
Summary by CodeRabbit
New Features
UI/Style Changes