feat(core): RundownPlaylist filters in Action Triggers#1652
Conversation
Add UI for RundownPlaylist filters for AdLibs
WalkthroughAdds Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ActionEditor
participant SwitchFilterType
participant FilterEditor
participant RundownPlaylistFilter
participant Compiler as ActionFilterCompiler
participant Runtime as TriggersRuntime
User->>ActionEditor: open editor / edit filter
ActionEditor->>FilterEditor: render(filterType)
FilterEditor->>SwitchFilterType: render switch (allowedTypes)
User->>SwitchFilterType: click "rundownPlaylist"
SwitchFilterType->>FilterEditor: onChangeType("rundownPlaylist")
FilterEditor->>ActionEditor: onChangeType(index, "rundownPlaylist")
ActionEditor->>RundownPlaylistFilter: render editor for rundownPlaylist
User->>RundownPlaylistFilter: select field (e.g., studioId) and value
RundownPlaylistFilter->>ActionEditor: onChange(link)
ActionEditor->>Compiler: save & compile filter chain
Compiler->>Compiler: compileRundownPlaylistFilter() -> selector
Runtime->>Runtime: evaluate selector vs current rundownPlaylist
alt selector matches
Runtime->>Runtime: continue ad-lib filter evaluation
else selector does not match
Runtime->>Runtime: early-skip ad-lib filters
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
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. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@packages/meteor-lib/src/triggers/actionFilterChainCompilers.ts`:
- Around line 535-536: The code assigns new RegExp(link.value) directly to
selector.name which creates a ReDoS risk for untrusted input; fix by treating
link.value as a literal string unless regex is explicitly required—escape the
value (e.g., use an escapeRegExp utility) before constructing the RegExp or use
a safe substring match alternative, and if you must accept regexes validate the
pattern and wrap the RegExp construction in a try-catch to handle invalid or
malicious patterns; update the logic around selector.name and link.value
accordingly and add tests for both escaped-literal and intentional-regex cases.
- Around line 622-652: The compiled filter mutates closure-scoped state
(adLibPieceTypeFilter.skip, adLibActionTypeFilter.skip, clearAdLibs.length,
stickyAdLibs.length) inside the runtime check, causing future invocations of
compileAdLibFilter to be corrupted; fix by not mutating those outer
variables—inside the inner function compute local values (e.g., localSkip or
localClear/localSticky arrays or filter copies) and either return early with an
empty result or use those local copies when matchAll fails so the closure state
(adLibPieceTypeFilter, adLibActionTypeFilter, clearAdLibs, stickyAdLibs) is
never modified across invocations.
In
`@packages/webui/src/client/ui/Settings/components/triggeredActions/actionEditors/ActionEditor.tsx`:
- Around line 78-85: The onChangeType handler in ActionEditor.tsx currently
mutates only action.filterChain[filterIndex].object, leaving stale field/value
and producing invalid filter links; change onChangeType to replace the entire
filter link at filterIndex with a fresh default link that matches the selected
FilterType (e.g., switch on newType and return a valid
IRundownPlaylistFilterLink, IAdLibFilterLink, IViewFilterLink, etc.), then call
overrideHelper().replaceItem(actionId, action).commit() with the updated action;
update references to action.filterChain and overrideHelper in the onChangeType
function so the old mixed objects are never kept.
In
`@packages/webui/src/client/ui/Settings/components/triggeredActions/actionEditors/filterPreviews/RundownPlaylistFilter.tsx`:
- Around line 41-62: fieldToOptions currently returns only {[t('Active')]: true}
for the activationId case, so users cannot choose the false ("Not Active")
value; update the activationId branch inside function fieldToOptions (which
handles IRundownPlaylistFilterLink['field']) to include both t('Active') -> true
and t('Not Active') -> false, or if excluding the false option was intentional,
add a clarifying comment in the activationId case explaining why filtering for
non-active playlists is unsupported.
🧹 Nitpick comments (2)
packages/meteor-lib/src/triggers/actionFilterChainCompilers.ts (1)
571-573: Remove the unnecessary@ts-ignore.
rundownPlaylistFilteris used at Line 623 (rundownPlaylistFilter.matchAll) and Line 627+ (rundownPlaylistFilter.selector.*), so the "unused" suppression is incorrect.♻️ Proposed fix
- // eslint-disable-next-line `@typescript-eslint/ban-ts-comment` - // `@ts-ignore` ignore unused const rundownPlaylistFilter = compileRundownPlaylistFilter(onlyRundownPlaylistLinks)packages/webui/src/client/ui/Settings/components/triggeredActions/actionEditors/filterPreviews/RundownPlaylistFilter.tsx (1)
64-82:studioIdvalue label shows raw ID instead of studio name.
fieldValueToValueLabelfor'studioId'(Line 73-74) returnsString(link.value), which is the studio's MongoDB_id. For better UX, consider looking up the studio name similarly to howfieldToOptionsalready queriesStudios:Proposed improvement
case 'name': - case 'studioId': return String(link.value) + case 'studioId': { + const studio = Studios.findOne(link.value) + return studio ? `${studio.name} (${link.value})` : String(link.value) + }Note: Since
fieldValueToValueLabelis called outsideuseTracker, this won't be reactive. If reactivity is needed, this lookup would need to move into the component body. However, since the value typically doesn't change while the editor is open, a non-reactive lookup may be acceptable here.
| case 'name': | ||
| selector.name = new RegExp(link.value) |
There was a problem hiding this comment.
ReDoS risk: link.value is used directly in new RegExp() without sanitization.
User-provided filter values flow into this regex constructor. Malicious or complex patterns could cause catastrophic backtracking. This was also flagged by the static analysis tool.
Consider escaping the input or using a safe regex library if the intent is literal substring matching. If regex is intentional, consider wrapping in a try-catch and/or validating the pattern.
🧰 Tools
🪛 ast-grep (0.40.5)
[warning] 535-535: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(link.value)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🤖 Prompt for AI Agents
In `@packages/meteor-lib/src/triggers/actionFilterChainCompilers.ts` around lines
535 - 536, The code assigns new RegExp(link.value) directly to selector.name
which creates a ReDoS risk for untrusted input; fix by treating link.value as a
literal string unless regex is explicitly required—escape the value (e.g., use
an escapeRegExp utility) before constructing the RegExp or use a safe substring
match alternative, and if you must accept regexes validate the pattern and wrap
the RegExp construction in a try-catch to handle invalid or malicious patterns;
update the logic around selector.name and link.value accordingly and add tests
for both escaped-literal and intentional-regex cases.
| const onChangeType = useCallback( | ||
| (filterIndex: number, newType: FilterType) => { | ||
| action.filterChain[filterIndex].object = newType | ||
|
|
||
| overrideHelper().replaceItem(actionId, action).commit() | ||
| }, | ||
| [action, overrideHelper] | ||
| ) |
There was a problem hiding this comment.
Changing object without resetting field/value produces invalid filter links.
When switching types (e.g., from 'adLib' to 'rundownPlaylist'), only the object property is updated. The stale field and value from the previous type remain, producing an invalid hybrid object like { object: 'rundownPlaylist', field: 'label', value: ['test'] } — which doesn't conform to IRundownPlaylistFilterLink.
Similarly, switching from 'view' (which has no field/value) to 'adLib' or 'rundownPlaylist' would leave field and value as undefined.
You should replace the entire filter link with a valid default for the target type.
🐛 Proposed fix
const onChangeType = useCallback(
(filterIndex: number, newType: FilterType) => {
- action.filterChain[filterIndex].object = newType
+ let newLink: ChainLink
+ switch (newType) {
+ case 'view':
+ newLink = { object: 'view' }
+ break
+ case 'rundownPlaylist':
+ newLink = { object: 'rundownPlaylist', field: 'activationId', value: true }
+ break
+ case 'adLib':
+ newLink = { object: 'adLib', field: 'label', value: [] }
+ break
+ }
+ action.filterChain.splice(filterIndex, 1, newLink)
overrideHelper().replaceItem(actionId, action).commit()
},
[action, overrideHelper]
)🤖 Prompt for AI Agents
In
`@packages/webui/src/client/ui/Settings/components/triggeredActions/actionEditors/ActionEditor.tsx`
around lines 78 - 85, The onChangeType handler in ActionEditor.tsx currently
mutates only action.filterChain[filterIndex].object, leaving stale field/value
and producing invalid filter links; change onChangeType to replace the entire
filter link at filterIndex with a fresh default link that matches the selected
FilterType (e.g., switch on newType and return a valid
IRundownPlaylistFilterLink, IAdLibFilterLink, IViewFilterLink, etc.), then call
overrideHelper().replaceItem(actionId, action).commit() with the updated action;
update references to action.filterChain and overrideHelper in the onChangeType
function so the old mixed objects are never kept.
| function fieldToOptions(t: TFunction, field: IRundownPlaylistFilterLink['field']): Record<string, any> { | ||
| switch (field) { | ||
| case 'activationId': | ||
| return { | ||
| [t('Active')]: true, | ||
| } | ||
| case 'name': | ||
| return {} | ||
| case 'rehearsal': | ||
| return { | ||
| [t('In rehearsal')]: true, | ||
| [t('Not in rehearsal')]: false, | ||
| } | ||
| case 'studioId': | ||
| return Studios.find() | ||
| .fetch() | ||
| .map((studio) => ({ name: `${studio.name} (${studio._id})`, value: studio._id })) | ||
| default: | ||
| assertNever(field) | ||
| return field | ||
| } | ||
| } |
There was a problem hiding this comment.
activationId dropdown only offers "Active" — no way to deselect.
fieldToOptions for 'activationId' only provides { [t('Active')]: true }. Since IRundownPlaylistFilterLink allows value: boolean for this field, and fieldValueToValueLabel handles both true ("Active") and false ("Not Active"), the dropdown has no way to select the false option. If this is intentional (filtering for non-active playlists is not a useful scenario), consider adding a brief comment to clarify. Otherwise, add the missing option:
Proposed fix if both options should be available
case 'activationId':
return {
[t('Active')]: true,
+ [t('Not Active')]: false,
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function fieldToOptions(t: TFunction, field: IRundownPlaylistFilterLink['field']): Record<string, any> { | |
| switch (field) { | |
| case 'activationId': | |
| return { | |
| [t('Active')]: true, | |
| } | |
| case 'name': | |
| return {} | |
| case 'rehearsal': | |
| return { | |
| [t('In rehearsal')]: true, | |
| [t('Not in rehearsal')]: false, | |
| } | |
| case 'studioId': | |
| return Studios.find() | |
| .fetch() | |
| .map((studio) => ({ name: `${studio.name} (${studio._id})`, value: studio._id })) | |
| default: | |
| assertNever(field) | |
| return field | |
| } | |
| } | |
| function fieldToOptions(t: TFunction, field: IRundownPlaylistFilterLink['field']): Record<string, any> { | |
| switch (field) { | |
| case 'activationId': | |
| return { | |
| [t('Active')]: true, | |
| [t('Not Active')]: false, | |
| } | |
| case 'name': | |
| return {} | |
| case 'rehearsal': | |
| return { | |
| [t('In rehearsal')]: true, | |
| [t('Not in rehearsal')]: false, | |
| } | |
| case 'studioId': | |
| return Studios.find() | |
| .fetch() | |
| .map((studio) => ({ name: `${studio.name} (${studio._id})`, value: studio._id })) | |
| default: | |
| assertNever(field) | |
| return field | |
| } | |
| } |
🤖 Prompt for AI Agents
In
`@packages/webui/src/client/ui/Settings/components/triggeredActions/actionEditors/filterPreviews/RundownPlaylistFilter.tsx`
around lines 41 - 62, fieldToOptions currently returns only {[t('Active')]:
true} for the activationId case, so users cannot choose the false ("Not Active")
value; update the activationId branch inside function fieldToOptions (which
handles IRundownPlaylistFilterLink['field']) to include both t('Active') -> true
and t('Not Active') -> false, or if excluding the false option was intentional,
add a clarifying comment in the activationId case explaining why filtering for
non-active playlists is unsupported.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/meteor-lib/src/triggers/actionFilterChainCompilers.ts (1)
535-537:⚠️ Potential issue | 🟠 MajorUnsafe dynamic regex construction remains in rundown name filter.
Line 536 uses
new RegExp(link.value)directly. This allows expensive patterns (ReDoS) and can also throw on invalid input.🔧 Safer literal-match approach
+const escapeRegExp = (value: string): string => value.replace(/[.*+?^${}()|[\]\\]/g, '\\$&') + function compileRundownPlaylistFilter(filterChain: IRundownPlaylistFilterLink[]): { @@ case 'name': - selector.name = new RegExp(link.value) + selector.name = new RegExp(escapeRegExp(link.value), 'i') return🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/meteor-lib/src/triggers/actionFilterChainCompilers.ts` around lines 535 - 537, The code in the 'name' switch branch assigns selector.name = new RegExp(link.value) using untrusted input (link.value) which can throw or enable ReDoS; instead, escape regex metacharacters and build a safe anchored regex or use a direct string equality depending on expected semantics: implement/consume an escape helper (e.g. escapeRegex(value)) and replace the assignment with selector.name = new RegExp('^' + escapeRegex(link.value) + '$') (and optionally enforce a max input length and wrap RegExp creation in try/catch), referencing the 'name' case and the selector.name = ... assignment in actionFilterChainCompilers.ts.
🧹 Nitpick comments (1)
packages/meteor-lib/src/triggers/actionFilterChainCompilers.ts (1)
571-573: Remove stale@ts-ignoresuppression.Line 573 assigns
rundownPlaylistFilter, and that symbol is used later (Line 623+). Keeping this suppression can mask real type issues.🧹 Cleanup
- // eslint-disable-next-line `@typescript-eslint/ban-ts-comment` - // `@ts-ignore` ignore unused const rundownPlaylistFilter = compileRundownPlaylistFilter(onlyRundownPlaylistLinks)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/meteor-lib/src/triggers/actionFilterChainCompilers.ts` around lines 571 - 573, Remove the stale eslint/ts-ignore suppression above the declaration of rundownPlaylistFilter: delete the two comment lines and ensure the symbol is properly typed instead of suppressed (e.g., give rundownPlaylistFilter an explicit type such as ReturnType<typeof compileRundownPlaylistFilter> or the concrete filter interface expected where it’s later used). Verify compileRundownPlaylistFilter’s signature matches that type and adjust its return type if needed so the code compiles without any `@ts-ignore`.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/meteor-lib/src/triggers/actionFilterChainCompilers.ts`:
- Around line 631-634: The nameMatches expression incorrectly evaluates to true
when currentRundownPlaylist is undefined because optional chaining yields
undefined which !== null is true; update the check in actionFilterChainCompilers
to explicitly ensure a playlist and name exist before testing the regex
(reference rundownPlaylistFilter.selector.name, currentRundownPlaylist, and
nameMatches). Replace the existing optional-chaining-only check with a boolean
guard that confirms currentRundownPlaylist (and its name) is present and that
name.match(...) returns a non-null value (e.g., check currentRundownPlaylist &&
currentRundownPlaylist.name && currentRundownPlaylist.name.match(...) !== null
or use an explicit null-check/assertion to yield a true/false result).
---
Duplicate comments:
In `@packages/meteor-lib/src/triggers/actionFilterChainCompilers.ts`:
- Around line 535-537: The code in the 'name' switch branch assigns
selector.name = new RegExp(link.value) using untrusted input (link.value) which
can throw or enable ReDoS; instead, escape regex metacharacters and build a safe
anchored regex or use a direct string equality depending on expected semantics:
implement/consume an escape helper (e.g. escapeRegex(value)) and replace the
assignment with selector.name = new RegExp('^' + escapeRegex(link.value) + '$')
(and optionally enforce a max input length and wrap RegExp creation in
try/catch), referencing the 'name' case and the selector.name = ... assignment
in actionFilterChainCompilers.ts.
---
Nitpick comments:
In `@packages/meteor-lib/src/triggers/actionFilterChainCompilers.ts`:
- Around line 571-573: Remove the stale eslint/ts-ignore suppression above the
declaration of rundownPlaylistFilter: delete the two comment lines and ensure
the symbol is properly typed instead of suppressed (e.g., give
rundownPlaylistFilter an explicit type such as ReturnType<typeof
compileRundownPlaylistFilter> or the concrete filter interface expected where
it’s later used). Verify compileRundownPlaylistFilter’s signature matches that
type and adjust its return type if needed so the code compiles without any
`@ts-ignore`.
c76273b to
fb99a01
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
packages/meteor-lib/src/triggers/actionFilterChainCompilers.ts (2)
535-537:⚠️ Potential issue | 🟠 MajorHarden rundown name regex compilation against malformed and hostile input.
At Line 536,
link.valueis passed directly intonew RegExp(...). This can throw on invalid patterns and exposes regex-complexity risk if input is user-controlled.Proposed safer literal-match fix
case 'name': - selector.name = new RegExp(link.value) + selector.name = new RegExp(link.value.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'), 'i') return🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/meteor-lib/src/triggers/actionFilterChainCompilers.ts` around lines 535 - 537, The code in actionFilterChainCompilers that sets selector.name = new RegExp(link.value) is unsafe for user-controlled input; replace it by validating and escaping link.value before constructing a regex (or use an exact-match alternative). Implement a small helper (e.g., escapeRegExp) to escape regex meta-characters, enforce a reasonable max length on link.value, then set selector.name = new RegExp('^' + escapeRegExp(link.value) + '$') inside a try/catch that falls back to a safe equality match (selector.name = link.value or null) on error; reference selector.name, link.value, and new RegExp when editing.
631-634:⚠️ Potential issue | 🟠 MajorFix false-positive
nameMatcheswhen no rundown playlist exists.Line 633 currently evaluates
undefined !== nullastrue, so the name filter can pass whencurrentRundownPlaylistis missing.Boolean guard fix
const nameMatches = rundownPlaylistFilter.selector.name !== undefined - ? currentRundownPlaylist?.name.match(rundownPlaylistFilter.selector.name) !== null + ? currentRundownPlaylist !== undefined && + currentRundownPlaylist.name.match(rundownPlaylistFilter.selector.name) !== null : true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/meteor-lib/src/triggers/actionFilterChainCompilers.ts` around lines 631 - 634, The nameMatches ternary incorrectly treats undefined (no currentRundownPlaylist) as a truthy match; change the conditional to require an actual RegExp match by ensuring currentRundownPlaylist and its name exist before checking match. For example, in the nameMatches assignment (using rundownPlaylistFilter.selector.name and currentRundownPlaylist?.name.match), replace the expression with a check that coerces the match result to boolean (e.g., !!currentRundownPlaylist?.name?.match(rundownPlaylistFilter.selector.name)) or explicitly compare against null/undefined so that when currentRundownPlaylist is missing the name filter evaluates to false. Ensure the else (selector.name undefined) still returns true.
🧹 Nitpick comments (1)
packages/meteor-lib/src/triggers/actionFilterChainCompilers.ts (1)
571-573: Remove stale@ts-ignorearound rundown playlist filter compilation.Lines 571-573 suppress type checking on a normal typed call and can hide future regressions.
Cleanup diff
- // eslint-disable-next-line `@typescript-eslint/ban-ts-comment` - // `@ts-ignore` ignore unused const rundownPlaylistFilter = compileRundownPlaylistFilter(onlyRundownPlaylistLinks)
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/meteor-lib/src/triggers/actionFilterChainCompilers.ts`:
- Around line 535-537: The code in actionFilterChainCompilers that sets
selector.name = new RegExp(link.value) is unsafe for user-controlled input;
replace it by validating and escaping link.value before constructing a regex (or
use an exact-match alternative). Implement a small helper (e.g., escapeRegExp)
to escape regex meta-characters, enforce a reasonable max length on link.value,
then set selector.name = new RegExp('^' + escapeRegExp(link.value) + '$') inside
a try/catch that falls back to a safe equality match (selector.name = link.value
or null) on error; reference selector.name, link.value, and new RegExp when
editing.
- Around line 631-634: The nameMatches ternary incorrectly treats undefined (no
currentRundownPlaylist) as a truthy match; change the conditional to require an
actual RegExp match by ensuring currentRundownPlaylist and its name exist before
checking match. For example, in the nameMatches assignment (using
rundownPlaylistFilter.selector.name and currentRundownPlaylist?.name.match),
replace the expression with a check that coerces the match result to boolean
(e.g.,
!!currentRundownPlaylist?.name?.match(rundownPlaylistFilter.selector.name)) or
explicitly compare against null/undefined so that when currentRundownPlaylist is
missing the name filter evaluates to false. Ensure the else (selector.name
undefined) still returns true.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/meteor-lib/src/triggers/actionFilterChainCompilers.tspackages/webui/src/client/ui/Settings/components/triggeredActions/actionEditors/filterPreviews/SwitchFilterType.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/webui/src/client/ui/Settings/components/triggeredActions/actionEditors/filterPreviews/SwitchFilterType.tsx
|
Having not received any feedback on this for the past 3 weeks, I'm merging it in. |
About the Contributor
This pull request is posted on behalf of the NRK.
Type of Contribution
This is a:
Feature
Current Behavior
The RundownPlaylist filter type for Action Triggers has no UI available.
New Behavior
A UI for different types of Filters is added. Rehearsal state of the Rundown can be used as a filter condition.
Testing
Affected areas
This PR affects the Action Triggers system.
Time Frame
Not urgent, but we would like to get this merged into the in-development release.
Other Information
Status