feat: User Edit operations on AdLib Pieces and Actions - #1790
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (28)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (26)
Included review availability: Your plan includes up to 2 reviews per rolling hour; 0 remain after this review. WalkthroughIntroduces user-editable properties across blueprint contracts, core data models, job-worker conversion, and webui selection and editing flows. It also adds typed operation targets and updates several independent type, form, ingest, and styling declarations. ChangesUser Editing Properties Feature
Maintenance Changes
Estimated code review effort: 4 (Complex) | ~60 minutes Merge Risk: ⚪ Minimal · up to This change adds editing support for AdLib Pieces and Actions; no actionable merge-blocking risk remains beyond normal checks and review. Sequence Diagram(s)sequenceDiagram
participant RundownView
participant Shelf
participant SelectedElementsContext
participant PropertiesPanel
participant MeteorCall
RundownView->>Shelf: enableUserEdits, onEditProps
Shelf->>SelectedElementsContext: selected element
SelectedElementsContext->>SelectedElementsContext: resolve piece or AdLib entity
SelectedElementsContext-->>PropertiesPanel: selectedObjects, type, rundownId
PropertiesPanel->>PropertiesPanel: getTargetForSelectedElement
PropertiesPanel->>MeteorCall: executeUserChangeOperation(target, change)
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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! |
7 tasks
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (3)
packages/webui/src/client/ui/RundownView/SelectedElementsContext.tsx (2)
306-370: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSet
lastValidSmallestElement.currentbefore relying on the fallback — the ref is only ever read here, so the fallback never triggers and transient lookup misses still clear the panel. Update it wheneverpiece/adLibPiece/rundownBaselineAdLibPiece/adLibAction/rundownBaselineAdLibActionresolves to a document.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/webui/src/client/ui/RundownView/SelectedElementsContext.tsx` around lines 306 - 370, The fallback in SelectedElementsContext’s useEffect never works because lastValidSmallestElement.current is only read, not updated, so transient lookup misses still clear the selected panel. Update lastValidSmallestElement.current whenever one of the document lookups in the Tracker.autorun block resolves (piece, adLibPiece, rundownBaselineAdLibPiece, adLibAction, or rundownBaselineAdLibAction), and keep the existing fallback switch so it can reuse the last valid document on temporary misses.
308-370: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winResolve
partInstanceselection through the parent part.SelectedElement.elementIdis aPartInstanceIdin this branch, butUIParts.findOnelooks upPartIds, soselectedObjects.partstays undefined and thepartInstanceedit props path never gets data.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/webui/src/client/ui/RundownView/SelectedElementsContext.tsx` around lines 308 - 370, The selection lookup in SelectedElementsContext’s useEffect is using selectedElement?.elementId directly as a UIParts key for the partInstance branch, but that value is a PartInstanceId and must be resolved through the parent part first. Update the logic around the piece/adLib lookups and the UIParts.findOne call so the part is fetched via the selected item’s startPartId/partId relationship rather than the instance id, and then derive the segment from that resolved part. This should allow selectedObjects.part to populate correctly for the partInstance path.packages/webui/src/client/ui/Shelf/ShelfContextMenu.tsx (1)
173-225: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRe-check the discriminant before accessing
context.details.adLib.pieceHasEditableContentis only a boolean, so it doesn’t narrowcontexthere; use an explicitcontext?.type === ContextType.ADLIBguard in this branch, or make the helper a type predicate.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/webui/src/client/ui/Shelf/ShelfContextMenu.tsx` around lines 173 - 225, The `pieceHasEditableContent` boolean in `ShelfContextMenu` does not narrow `context`, so the `enableUserEdits && pieceHasEditableContent` branch still accesses `context.details.adLib` unsafely. Update the `MenuItem`/edit-properties block to re-check `context?.type === ContextType.ADLIB` before reading `context.details.adLib`, or change `hasUserEditableContent` to a type predicate so the `ShelfContextMenu` render path is properly narrowed.
♻️ Duplicate comments (1)
packages/webui/src/client/lib/rundown.ts (1)
43-44: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winSame
/src/vs/dist/import path concern as inSelectedElementsContext.tsx.Both new imports resolve via
@sofie-automation/corelib/src/...rather than/dist/..., which is the convention used elsewhere for corelib types (e.g.postProcess.ts's import of the same-generationUserEditingDefinitionstypes). This risks build/type-resolution failures depending on corelib's package.json exports policy.🩹 Suggested fix
-import type { AdLibAction } from '`@sofie-automation/corelib/src/dataModel/AdlibAction.js`' -import type { RundownBaselineAdLibAction } from '`@sofie-automation/corelib/src/dataModel/RundownBaselineAdLibAction.js`' +import type { AdLibAction } from '`@sofie-automation/corelib/dist/dataModel/AdlibAction.js`' +import type { RundownBaselineAdLibAction } from '`@sofie-automation/corelib/dist/dataModel/RundownBaselineAdLibAction.js`'🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/webui/src/client/lib/rundown.ts` around lines 43 - 44, The new corelib type imports in rundown.ts use the /src/ path instead of the /dist/ convention used elsewhere, which can break type resolution depending on corelib exports. Update the AdLibAction and RundownBaselineAdLibAction imports to match the same package import pattern used in other webui files like postProcess.ts and SelectedElementsContext.tsx, keeping the symbols unchanged but pointing them at the generated/dist entry points.
🧹 Nitpick comments (2)
packages/corelib/src/dataModel/AdlibAction.ts (1)
6-6: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider
import typefor consistency.
CoreUserEditingDefinition/CoreUserEditingPropertiesare pure types, but this import isn't markedimport type, while sibling changes inaction.tsandpieceGeneric.tsin the same PR explicitly convert equivalent type-only imports toimport type. Purely stylistic; flagging for consistency only.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/corelib/src/dataModel/AdlibAction.ts` at line 6, The import in AdlibAction should be converted to a type-only import for consistency with the sibling updates in action.ts and pieceGeneric.ts. Update the existing import of CoreUserEditingDefinition and CoreUserEditingProperties from UserEditingDefinitions.js to use import type, since both symbols are pure types and do not need a runtime import.packages/documentation/docs/for-developers/for-blueprint-developers/manipulating-ingest-data.md (1)
74-100: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSame stale variable name left unfixed just below the fix.
Line 58 was corrected from
ingestRundownChanges.changestochanges(matching the function'schangesparameter), but the nearly-identical example at Line 86 still passesingestRundownChanges.changes, which doesn't exist in this function's scope.📝 Proposed fix
const groupedResult = context.groupPartsInRundownAndChanges( nrcsIngestRundown, previousNrcsIngestRundown, - ingestRundownChanges.changes, + changes, (segments) => {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/documentation/docs/for-developers/for-blueprint-developers/manipulating-ingest-data.md` around lines 74 - 100, The example in processIngestData still uses the stale ingestRundownChanges.changes reference even though the function receives changes as its parameter. Update that call inside groupPartsInRundownAndChanges to use the existing changes variable, matching the earlier fix, so the snippet is consistent and references only in-scope symbols.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/blueprints-integration/src/action.ts`:
- Around line 132-142: Update the JSDoc for IBlueprintActionManifest fields
userEditOperations and userEditProperties so it refers to the action/manifest,
not “this piece,” and replace the incorrect operation type name with the real
enum symbol DefaultUserOperationsTypes.UPDATE_PROPS. Apply the same wording
correction in AdlibAction.ts where the copied “piece” language appears, and
ensure any duplicated docs in pieceGeneric.ts and Piece.ts use the correct
symbol name from ingest.ts rather than the nonexistent
DefaultUserOperationEditProperties reference.
In
`@packages/documentation/docs/for-developers/for-blueprint-developers/manipulating-ingest-data.md`:
- Around line 110-114: The documentation wording around userEditOperations is
incomplete and the typedoc link is pointing to the wrong symbol. Update the
description to include AdLib Pieces and AdLib Actions alongside Rundowns,
Segments, Parts, and Pieces, and change the anchor to
IBlueprintPieceGeneric#usereditoperations. Also correct the text to “built-in”
and “defined”, and keep the reference to DefaultUserOperationsTypes accurate.
In `@packages/webui/src/client/lib/forms/SchemaFormOneOfButtons/OneOfButtons.tsx`:
- Around line 181-193: The two sync effects in OneOfButtons are overlapping and
the first one updates oldValue.current before the second effect can observe the
initial undefined state, so the push-default update path never fires. Merge the
logic into a single useEffect in OneOfButtons.tsx that preserves the original
mutually exclusive branches: when selected and value changes from a defined
prior value, sync editingValue and oldValue.current; when selected and
oldValue.current is still undefined on the initial value push, call
handleUpdateRef.current with editingValue before recording the new value.
In `@packages/webui/src/client/ui/RundownView/SelectedElementsContext.tsx`:
- Around line 28-31: SelectedElementsContext.tsx has one deep import that still
points to the corelib src path instead of the shipped dist subpath. Update the
import of CoreUserEditingDefinition and CoreUserEditingProperties to use the
same `@sofie-automation/corelib/dist/`... pattern used elsewhere in the file so it
matches the package exports and stays consistent with the rest of the imports.
In `@packages/webui/src/client/ui/UserEditOperations/PropertiesPanel.tsx`:
- Around line 96-133: The revert flow in PropertiesPanel.handleRevertChanges
currently rejects the exact selections it should support because the target-type
guard only allows already-segment/part targets and returns early for piece/adlib
selections. Update the conversion logic so piece, adlibPiece, and adlibAction
selections are mapped through getTargetForSelectedElement into a valid part or
segment target before calling MeteorCall.userAction.executeUserChangeOperation,
and keep the existing DefaultUserOperationsTypes choice based on
selectedElement.type.
- Around line 52-69: The auto-close guard in PropertiesPanel only tracks
selectedObjects.piece, so deselecting AdLib-related items will not clear
selections. Update the hadSmallestElement tracking effect and the pieceChangedId
check in PropertiesPanel to include selectedObjects.adLibPiece,
selectedObjects.rundownBaselineAdLibPiece, selectedObjects.adLibAction, and
selectedObjects.rundownBaselineAdLibAction alongside piece, then keep the
clearSelections() reset behavior when any of those smallest-element selections
are removed.
---
Outside diff comments:
In `@packages/webui/src/client/ui/RundownView/SelectedElementsContext.tsx`:
- Around line 306-370: The fallback in SelectedElementsContext’s useEffect never
works because lastValidSmallestElement.current is only read, not updated, so
transient lookup misses still clear the selected panel. Update
lastValidSmallestElement.current whenever one of the document lookups in the
Tracker.autorun block resolves (piece, adLibPiece, rundownBaselineAdLibPiece,
adLibAction, or rundownBaselineAdLibAction), and keep the existing fallback
switch so it can reuse the last valid document on temporary misses.
- Around line 308-370: The selection lookup in SelectedElementsContext’s
useEffect is using selectedElement?.elementId directly as a UIParts key for the
partInstance branch, but that value is a PartInstanceId and must be resolved
through the parent part first. Update the logic around the piece/adLib lookups
and the UIParts.findOne call so the part is fetched via the selected item’s
startPartId/partId relationship rather than the instance id, and then derive the
segment from that resolved part. This should allow selectedObjects.part to
populate correctly for the partInstance path.
In `@packages/webui/src/client/ui/Shelf/ShelfContextMenu.tsx`:
- Around line 173-225: The `pieceHasEditableContent` boolean in
`ShelfContextMenu` does not narrow `context`, so the `enableUserEdits &&
pieceHasEditableContent` branch still accesses `context.details.adLib` unsafely.
Update the `MenuItem`/edit-properties block to re-check `context?.type ===
ContextType.ADLIB` before reading `context.details.adLib`, or change
`hasUserEditableContent` to a type predicate so the `ShelfContextMenu` render
path is properly narrowed.
---
Duplicate comments:
In `@packages/webui/src/client/lib/rundown.ts`:
- Around line 43-44: The new corelib type imports in rundown.ts use the /src/
path instead of the /dist/ convention used elsewhere, which can break type
resolution depending on corelib exports. Update the AdLibAction and
RundownBaselineAdLibAction imports to match the same package import pattern used
in other webui files like postProcess.ts and SelectedElementsContext.tsx,
keeping the symbols unchanged but pointing them at the generated/dist entry
points.
---
Nitpick comments:
In `@packages/corelib/src/dataModel/AdlibAction.ts`:
- Line 6: The import in AdlibAction should be converted to a type-only import
for consistency with the sibling updates in action.ts and pieceGeneric.ts.
Update the existing import of CoreUserEditingDefinition and
CoreUserEditingProperties from UserEditingDefinitions.js to use import type,
since both symbols are pure types and do not need a runtime import.
In
`@packages/documentation/docs/for-developers/for-blueprint-developers/manipulating-ingest-data.md`:
- Around line 74-100: The example in processIngestData still uses the stale
ingestRundownChanges.changes reference even though the function receives changes
as its parameter. Update that call inside groupPartsInRundownAndChanges to use
the existing changes variable, matching the earlier fix, so the snippet is
consistent and references only in-scope symbols.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ae6efeae-b0bc-44b2-9e5e-4b1ff9de8934
📒 Files selected for processing (28)
packages/blueprints-integration/src/action.tspackages/blueprints-integration/src/api/studio.tspackages/blueprints-integration/src/documents/piece.tspackages/blueprints-integration/src/documents/pieceGeneric.tspackages/blueprints-integration/src/ingest.tspackages/blueprints-integration/src/triggers.tspackages/corelib/src/TranslatableMessage.tspackages/corelib/src/dataModel/AdLibPiece.tspackages/corelib/src/dataModel/AdlibAction.tspackages/corelib/src/dataModel/Piece.tspackages/corelib/src/worker/ingest.tspackages/documentation/docs/for-developers/for-blueprint-developers/manipulating-ingest-data.mdpackages/job-worker/src/blueprints/context/lib.tspackages/job-worker/src/blueprints/postProcess.tspackages/webui/src/client/lib/forms/SchemaFormOneOfButtons/OneOfButtons.tsxpackages/webui/src/client/lib/rundown.tspackages/webui/src/client/styles/contextMenu.scsspackages/webui/src/client/ui/RundownView.tsxpackages/webui/src/client/ui/RundownView/DragContextProvider.tsxpackages/webui/src/client/ui/RundownView/RundownDetachedShelf.tsxpackages/webui/src/client/ui/RundownView/SelectedElementsContext.tsxpackages/webui/src/client/ui/SegmentTimeline/SegmentContextMenu.tsxpackages/webui/src/client/ui/Shelf/AdLibPanel.tsxpackages/webui/src/client/ui/Shelf/ExternalFramePanel.tsxpackages/webui/src/client/ui/Shelf/Shelf.tsxpackages/webui/src/client/ui/Shelf/ShelfContextMenu.tsxpackages/webui/src/client/ui/UserEditOperations/PropertiesPanel.tsxpackages/webui/src/client/ui/UserEditOperations/__tests__/PropertiesPanel.test.tsx
💤 Files with no reviewable changes (1)
- packages/blueprints-integration/src/documents/piece.ts
jstarpl
force-pushed
the
feat/adlib-user-edits
branch
from
July 6, 2026 12:42
3e9b01e to
eb99798
Compare
nytamin
approved these changes
Aug 5, 2026
… data Add information about actual `userEditOperations` declarations and `DefaultUserOperationsTypes` instead of vague language.
jstarpl
force-pushed
the
feat/adlib-user-edits
branch
from
August 17, 2026 10:06
eb99798 to
62355ea
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
About the Contributor
This pull request is posted on behalf of the NRK.
Type of Contribution
This is a:
Feature
Current Behavior
While Segments, Parts and Pieces can have User Edit actions and properties, AdLib Pieces and Actions do not.
New Behavior
AdLib Pieces and Actions can also be edited using the Properties panel.
Testing
Affected areas
Breaking change: This PR affects the "User Edits" functionality and tightens the API this functionality uses to integrate with the Blueprints.
Time Frame
Not urgent, but we would like to get this merged into the in-development release.
Other Information
Status