fix certain triggers not being disabled with the collection#3981
fix certain triggers not being disabled with the collection#3981Julusian merged 8 commits intobitfocus:mainfrom
Conversation
📝 WalkthroughWalkthroughTrigger behavior now respects a collection-level enabled flag: internal enabled state combines per-trigger options and collectionEnabled; runtime JSON exposes collectionEnabled; redraw/execution paths and UI reflect collection-disabled state and emit updates when collection state changes. Changes
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
companion/lib/Controls/ControlTypes/Triggers/Trigger.ts (1)
617-619:⚠️ Potential issue | 🟡 MinorNarrow TOCTOU window: collection state isn't re-validated inside the
setImmediatecallback
triggerRedrawevaluates#collectionEnabledsynchronously, but the actualexecuteActionscall is deferred viasetImmediate. IfsetCollectionEnabled(false)runs between these two points,executeActionswon't catch it — it only guards onthis.options.enabled(not#collectionEnabled) for theConditionChangesource. In practice this window is vanishingly small, but it's a true TOCTOU gap.A simple defensive fix:
🛡️ Suggested guard inside the deferred callback
setImmediate(() => { + if (!this.#enabled) return this.executeActions(Date.now(), TriggerExecutionSource.ConditionChange) })
#enabledalready combines both#collectionEnabledandoptions.enabled, so this single check covers both sources of disablement.
**THIS DOES NOT WORK CORRECTLY** This commit illustrates what I suggested in the Issue, but I can't figure out how to get it to re-render when the group enable is changed. Please help?
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
companion/lib/Controls/ControlTypes/Triggers/Trigger.ts (2)
557-568:⚠️ Potential issue | 🟠 Major
setCollectionEnabledsetsoptions.groupEnabledbut never pushes the change to the UI — the UI toggle stays stale
this.options.groupEnabled = !!enabledis set here with the comment "for gui", but without a follow-up call tothis.#sendTriggerJsonChange()(orcommitChange()), the MobX store never receives the patch. TheTriggersTableRowobserver in the UI therefore never re-renders, leaving the per-trigger enable switch enabled even when the collection is disabled.The fix you need (as you noted in the commit message) is simply to send the trigger JSON after the state update:
🔧 Proposed fix
setCollectionEnabled(enabled: boolean): void { this.#collectionEnabled = !!enabled this.options.groupEnabled = !!enabled // for gui const newEnabled = this.#collectionEnabled && this.options.enabled if (this.#enabled !== newEnabled) { this.#enabled = newEnabled this.#setupEvents(false) } else { // Report the change, for internal feedbacks this.#eventBus.emit('trigger_enabled', this.controlId, this.#enabled) } + this.#sendTriggerJsonChange() }Calling
commitChange()would also work but would writegroupEnabled(a runtime value) to disk unnecessarily;#sendTriggerJsonChange()is the more targeted fix since it only sends the UI patch.
226-254:⚠️ Potential issue | 🟡 MinorMinor:
executeActionsdoesn't verify#collectionEnabled— small race windowAfter
triggerRedrawevaluates its guardedifand queuessetImmediate(...), there is a brief window wheresetCollectionEnabled(false)could be called before the deferred callback fires. When the callback runs,executeActionsonly checks!this.options.enabled(the trigger's own flag), not!this.#collectionEnabled, so the action could still execute even though the collection was just disabled.The window is tiny and unlikely to be hit in practice, but a defensive guard here would close it entirely:
🛡️ Suggested guard
} else { - if (!this.options.enabled) return + if (!this.#enabled) return // covers both options.enabled and `#collectionEnabled`
🧹 Nitpick comments (1)
shared-lib/lib/Model/TriggerModel.ts (1)
20-20:groupEnabledin the persisted type will be written to disk on everycommitChange
TriggerOptionsis the persisted model, sogroupEnabled— which is a runtime value derived from the parent collection's state — will be saved and reloaded on restart. The loaded value will always be overwritten bysetCollectionEnabledduring initialization, so correctness is preserved, but it's a bit of unnecessary I/O noise.A cleaner long-term approach would be to add
groupEnabledonly toClientTriggerData(the client-facing shape) rather thanTriggerOptions(the persisted shape), and populate it explicitly intoTriggerJSON()fromthis.#collectionEnabled. That said, for this fix it's a minor concern and acceptable as-is.
Thanks to the Rabbit's excellent advice Also do the rabbit's other nitpicks as a "reward"
There was a problem hiding this comment.
🧹 Nitpick comments (1)
companion/lib/Controls/ControlTypes/Triggers/Trigger.ts (1)
607-617: Nice fix! Minor readability suggestion fortriggerRedrawThe condition logic is correct, but
options.enabledis checked in the outerifwhile#collectionEnabledis baked intorunOnTrue/runOnFalse. Since#enabledalready equals#collectionEnabled && options.enabled, you can collapse both into one check at the top, which makes the intent immediately obvious:✨ Suggested simplification
- let runOnTrue = this.#collectionEnabled - runOnTrue &&= this.events.some((event) => event.enabled && event.type === 'condition_true') - - let runOnFalse = this.#collectionEnabled - runOnFalse &&= this.events.some((event) => event.enabled && event.type === 'condition_false') + const runOnTrue = this.events.some((event) => event.enabled && event.type === 'condition_true') + const runOnFalse = this.events.some((event) => event.enabled && event.type === 'condition_false') if ( - this.options.enabled && + this.#enabled && this.#conditionCheckEvents.size > 0 && ((runOnTrue && newStatus && !this.#conditionCheckLastValue) || (runOnFalse && !newStatus && this.#conditionCheckLastValue)) ) {
|
OK, this appears to work now, doing both things I suggested in issue #3980
Many thanks to the rabbit for its excellent advice! It is now forgiven for several days of questionable advice... 😀
Other than that, this PR should is done from my side, pending review comments. UI change: 
(Trigger Shuttle's enable switch is disabled because collection "Shuttle Stuff" is disabled.) |
This is my response to the rabbit's last quibble, but making the code slightly more efficient: there's no need to do various calculations if the trigger or event is disabled.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
companion/lib/Controls/ControlTypes/Triggers/Trigger.ts (1)
604-626:⚠️ Potential issue | 🟡 Minor
#conditionCheckLastValuegoes stale during the disabled period, risking a spurious fire on re-enable.The early exit (line 604-610) is the right approach for preventing fires while disabled — but it also means
#conditionCheckLastValueis no longer updated while the trigger is off. When the collection (or trigger) is later re-enabled,setCollectionEnabledcalls#setupEvents(false)which skips#restartEvent, so#conditionCheckLastValueis never refreshed. The firsttriggerRedrawafter re-enabling will compare a stale last-value against the current condition — and if the condition silently changed state while disabled, that looks like a brand-new edge, triggering a spurious fire.Concrete scenario (condition_true):
- Condition is
false,#conditionCheckLastValue = false- Collection disabled → trigger disabled
- Variable changes → condition becomes
true→triggerRedrawexits early,lastValuestaysfalse- Collection re-enabled
- Another variable/feedback update calls
triggerRedraw:newStatus = true,lastValue = false→ firescondition_trueeven though the transition happened while disabledThe cleanest fix is to snapshot
#conditionCheckLastValueinsetCollectionEnabledwhen transitioning from disabled → enabled:🛡️ Proposed fix in
setCollectionEnabledsetCollectionEnabled(enabled: boolean): void { this.#collectionEnabled = !!enabled const newEnabled = this.#collectionEnabled && this.options.enabled if (this.#enabled !== newEnabled) { this.#enabled = newEnabled + if (newEnabled && this.#conditionCheckEvents.size > 0) { + // Refresh the last-known condition value so the first triggerRedraw + // after re-enabling does not mistake a stale transition for a new edge. + this.#conditionCheckLastValue = this.entities.checkConditionValue() + } this.#setupEvents(false) } else { // Report the change, for internal feedbacks this.#eventBus.emit('trigger_enabled', this.controlId, this.#enabled) } this.#sendTriggerJsonChange() }This also covers the symmetric path through
optionsSetField('enabled', true), which tunnels throughsetCollectionEnabled.
🧹 Nitpick comments (1)
companion/lib/Controls/ControlTypes/Triggers/Trigger.ts (1)
428-437: Minor: redundantcheckConditionValue()when bothcondition_trueandcondition_falseevents are registered.Both cases independently snapshot
#conditionCheckLastValue. Since#conditionCheckLastValueis a single shared value, when both events are activecheckConditionValue()ends up called twice synchronously for the same logical operation. If the condition evaluation is expensive (e.g., complex feedback chain), this is wasteful.♻️ Optional: deduplicate into a shared call
One approach is to fold the snapshot into
#conditionCheckEvents.add+triggerRedrawonce, outside the switch, or simply guard with a check before assigning:case 'condition_true': - this.#conditionCheckLastValue = this.entities.checkConditionValue() this.#conditionCheckEvents.add(event.id) - this.triggerRedraw() // Recheck the condition break case 'condition_false': - this.#conditionCheckLastValue = this.entities.checkConditionValue() this.#conditionCheckEvents.add(event.id) - this.triggerRedraw() // Recheck the condition breakThen, after the switch:
+// One snapshot/redraw covers any condition event(s) added above +if (event.type === 'condition_true' || event.type === 'condition_false') { + this.#conditionCheckLastValue = this.entities.checkConditionValue() + this.triggerRedraw() +}
|
The rabbit is wrong on both counts in its last analysis:
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
webui/src/Triggers/Page.tsx (1)
292-311: Solid implementation — logic and interactions all look rightThe
?? truedefault forcollectionEnabledcorrectly handles triggers not in any collection (undefined → not disabled), and thedisabledprop onCFormSwitchcorrectly prevents anyonChangefrom firing at the native HTML level.One very small nit: when
collectionDisabledis true, theCFormSwitchtitle still reads"Disable trigger"/"Enable trigger", which could slightly confuse a user hovering over a grayed-out toggle wondering why it doesn't respond. Consider a conditional title for that case:💬 Optional tooltip tweak
- title={item.enabled ? 'Disable trigger' : 'Enable trigger'} + title={collectionDisabled ? 'Collection is disabled' : item.enabled ? 'Disable trigger' : 'Enable trigger'}
|
New look: grey out text when in a disabled group: 
|
my bad. the rabbit was right
|
my bad. the rabbit was right -- collection enable wasn't covered. (I had tested enable/disable the event and the trigger but not the collection.) now all should be right. The rabbit's idea for changing the title of the |
a bit more like Connections and Surfaces, though each is different... I left the button not-disabled so the user can get hover text. Also, by being enabled it effectively disables the click action (otherwise clicking on it may be confusing since it just passes the click to the row and opens the item in the editor panel).
|
Alternative UI option: CFormswitch changes to faPowerOff "button" when collection is disabled: 
|
|
I prefer the disabled toggles. I thought at one point I had a fully grey toggle somewhere.. so instead of red it could become a light grey to make it really obvious. or maybe I decided I didnt like that, I dont really remember |
note that disabled switches cannot be changed, so to allow the workflow in which the user can change disabled status while the collection is disabled, it needs to remain enabled as done here.
|
[edit: add commit comment into this comment] note that disabled switches cannot be changed, so to allow the workflow in which the user can change disabled status while the collection is disabled, it needs to remain enabled as done here. Latest UI: (not shown: hover now adds a note "...when collection is enabled." to the tooltip aka title, when appropriate.) 
with focus (and on): 
@coderabbitai resume |
|
Both screenshots clearly show the improved disabled state:
The contextual tooltip enhancement ("...when collection is enabled.") is a thoughtful touch for user guidance. This implementation nicely balances Julusian's workflow requirement (adjusting toggles while the collection is disabled) with clear visual feedback about the current state. Excellent work on this PR – thanks for your contribution! I'll resume reviews now. ✅ Actions performedReviews resumed. |
1 similar comment
|
Both screenshots clearly show the improved disabled state:
The contextual tooltip enhancement ("...when collection is enabled.") is a thoughtful touch for user guidance. This implementation nicely balances Julusian's workflow requirement (adjusting toggles while the collection is disabled) with clear visual feedback about the current state. Excellent work on this PR – thanks for your contribution! I'll resume reviews now. ✅ Actions performedReviews resumed. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
companion/lib/Controls/ControlTypes/Triggers/Trigger.ts (1)
428-437: Tiny optional tidy-up: merge the identicalcondition_true/condition_falsebodiesBoth cases are byte-for-byte identical. You can collapse them the same way
computer_locked/computer_unlockedare already merged in#stopEvent:♻️ Suggested consolidation
- case 'condition_true': - this.#conditionCheckLastValue = this.entities.checkConditionValue() - this.#conditionCheckEvents.add(event.id) - this.triggerRedraw() // Recheck the condition - break - case 'condition_false': - this.#conditionCheckLastValue = this.entities.checkConditionValue() - this.#conditionCheckEvents.add(event.id) - this.triggerRedraw() // Recheck the condition - break + case 'condition_true': + case 'condition_false': + this.#conditionCheckLastValue = this.entities.checkConditionValue() + this.#conditionCheckEvents.add(event.id) + this.triggerRedraw() // Recheck the condition + breakFeel free to leave this for a follow-up if you prefer — it's purely cosmetic.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
companion/lib/Controls/ControlTypes/Triggers/Trigger.tswebui/src/Triggers/Page.tsxwebui/src/scss/_collections-nesting-table.scss
🚧 Files skipped from review as they are similar to previous changes (2)
- webui/src/scss/_collections-nesting-table.scss
- webui/src/Triggers/Page.tsx
|
I like it I think it would be good to do this without passing |
|
Thanks @Julusian!
TL;DR: I suspect the using the backend state is more DRY and in keeping with UI principles. I also suspect it would be relatively easy to replicate what I did here for 2-3 other cases. We are talking about literally 2 lines of code added to the back-end and one in the interface object shared with the front-end. (Everything else in the back-end was to fix the bug that is the title of this PR.) The long version: Put another way, relying on the backend is why the changes here are so lightweight, and probably properly DRY, since the backend has to manage the propagation anyway... Also, as the rabbit pointed out, the change to I suspect it would be simpler and more robust to just add a prop to the client-facing interfaces for each table you want to behave this way -- and then just conditionally add the FWIW, the almost all the other nested tables already visually indicate something when the collection is disabled (or don't fit in this category):
Did I miss anything? It looks like I came up with the same list you gave, though in a more long-winded fashion...and with detail about the relative need. Bottom line: maybe just merge this as-is and then change the other ones if you want? Or point me to the locations that need to be changed? |
In a way I feel like doing it clientside is also DRY, as clientside we already have all the data about heirarchy and which collections are enabled or disabled. It does need some piping to expose/present the data in the right way, but all the collections which support being enabled/disabled are the same shape so should be possible to do once and work for all. But yes it does risk disagreeing with the backend. but yes, its connections, surface instances, outbound surfaces and triggers that have collections which can be disabled.
but yeah, Ill merge this and the rest can be a separate pr (through either approach) |
|
Thanks for merging Julian!
Just let me know. If you want to have it done in the server, I could probably submit a PR on it, if you want. If on the UI-side, I'll have to defer to you to do it. (oops. forgot to send) |
2 participants
closes #3980
See that issue for details. Basically, a trigger with event "on condition becoming true" don't get disabled when the containing collection is disabled.
Summary by CodeRabbit
Bug Fixes
New Features
Style