feat: UI piece retiming #1544
feat: UI piece retiming #1544
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
rjmunro
force-pushed
the
feat/ui-piece-retiming
branch
from
320f3d4 to
7e84b20
Compare
Saftret
added
the
Contribution from BBC
rjmunro
force-pushed
the
feat/ui-piece-retiming
branch
from
7e84b20 to
eadaa4c
Compare
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughAdds a RETIME_PIECE user operation and edit-mode action; extends core and blueprint typings; introduces a drag-and-drop context and provider in the Rundown UI to start retime interactions, emits edit-mode events, and calls Meteor to persist piece retimes. Changes
Sequence DiagramsequenceDiagram
participant User
participant SourceLayerItem
participant DragContextProvider
participant RundownViewEventBus
participant MeteorBackend
participant UI
User->>SourceLayerItem: mousedown on draggable piece
SourceLayerItem->>DragContextProvider: startDrag(piece, timeScale, position, elementOffset?, limitToPart?, limitToSegment?)
DragContextProvider->>UI: enable drag visuals, set pieceId/piece
User->>UI: move mouse over parts
UI->>DragContextProvider: pointer move events (internal)
SourceLayerItem->>DragContextProvider: setHoveredPart(partId, segmentId, position)
DragContextProvider->>DragContextProvider: update hover state
User->>DragContextProvider: mouseup
DragContextProvider->>RundownViewEventBus: emit EDIT_MODE (state)
DragContextProvider->>MeteorBackend: call retimeUserOperation(RETIME_PIECE, payload)
MeteorBackend->>MeteorBackend: persist retime (update inPoint)
DragContextProvider->>UI: clear drag visuals, reset state
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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 |
rjmunro
force-pushed
the
feat/ui-piece-retiming
branch
3 times, most recently
from
282eb15 to
8075b5b
Compare
rjmunro
force-pushed
the
feat/ui-piece-retiming
branch
from
8075b5b to
97556fc
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @packages/blueprints-integration/src/userEditing.ts:
- Around line 60-64: The field limitToCurrentPart is wrongly declared on
UserEditingDefinitionSofieDefault allowing it for all
DefaultUserOperationsTypes; change the types so limitToCurrentPart exists only
for RETIME_PIECE operations — either create a specific interface (e.g.,
RetimePieceUserEditingDefinition) that extends the base and adds
limitToCurrentPart, or convert UserEditingDefinitionSofieDefault into a
discriminated union keyed by the operation type (with a variant for RETIME_PIECE
that includes limitToCurrentPart), update any uses expecting
UserEditingDefinitionSofieDefault accordingly and keep runtime guards like in
SourceLayerItem.tsx in sync with the new discriminant.
In @packages/corelib/src/dataModel/UserEditingDefinitions.ts:
- Around line 50-55: There are two exported declarations of
CoreUserEditingDefinitionSofie which risks accidental interface-merging; remove
the duplicate declaration so only one exported interface
CoreUserEditingDefinitionSofie remains, keeping the correct version (the one
with type: UserEditingType.SOFIE and id: DefaultUserOperationsTypes) and delete
the redundant copy found later in the file.
In @packages/webui/src/client/ui/RundownView/DragContextProvider.tsx:
- Around line 92-97: The early return when Segments.findOne(part?.segmentId)
yields undefined leaves pieceId set and prevents future drags; update the block
that computes segment and oldSegment so that if segment is falsy you call
cleanup() before returning (i.e., call cleanup() when
Segments.findOne(part?.segmentId) returns undefined), ensuring pieceId is
cleared and the drag-start guard (the pieceId check) is reset; use the existing
cleanup() helper rather than duplicating state logic and keep oldSegment
resolution unchanged.
In @packages/webui/src/client/ui/SegmentTimeline/Parts/SourceLayer.tsx:
- Around line 95-107: The hover handler uses e.target which can be a child
element and yields wrong coords; change the bounding rect call in onMouseEnter
to use e.currentTarget (e.g., (e.currentTarget as
HTMLDivElement).getBoundingClientRect()) so the rect is for the layer container,
then pass those coords to dragCtx.setHoveredPart as before; apply the same
change to the other mouse event handler at the similar location that currently
uses e.target.
🧹 Nitpick comments (4)
packages/webui/src/client/ui/Settings/components/triggeredActions/actionEditors/actionSelector/ActionSelector.tsx (1)
394-427: Inconsistent styling with similar components.The edit mode parameter editor deviates from the pattern established by the shelf action:
- Line 396: Uses
className="mts"instead ofclassName="mt-2"(compare to shelf at line 362)- Line 399: Uses
classNames="input text-input input-m"instead ofclassNames="mb-2"(compare to shelf at line 365)Consider aligning the styling with the shelf implementation for consistency unless there's a specific reason for the divergence.
♻️ Align styling with shelf pattern
case ClientActions.editMode: return ( - <div className="mts"> + <div className="mt-2"> <label className="block">{t('State')}</label> <DropdownInputControl<typeof action.state> - classNames="input text-input input-m" + classNames="mb-2" value={action.state} // placholder={t('State')} options={[packages/webui/src/client/lib/ui/pieceUiClassNames.ts (1)
10-24: Backwards-compatibledraggablestyling hook looks goodOptional param won’t break existing call sites, and the class toggle is idiomatic for
classnames.Optional: consider an options object to stop the param list growing
The function is already at many positional params; a
{ uiState, draggable, ... }options object would be easier to extend without forcingundefinedplaceholders.Also applies to: 60-63
packages/webui/src/client/ui/RundownView/DragContextProvider.tsx (2)
35-139: Consider memoizingstartDragwithuseCallback.The
startDragfunction is recreated on every render because it's not wrapped inuseCallback. This causes the context value (ctx) to change on every render, which may trigger unnecessary re-renders in consuming components. While not critical, this could impact performance with many consumers.♻️ Suggested refactor
- const startDrag = ( + const startDrag = useCallback(( ogPiece: PieceUi, timeScale: number, pos: { x: number; y: number }, elementOffset?: number, limitToPart?: PartInstanceId, limitToSegment?: SegmentId ) => { // ... function body unchanged - } + }, [pieceId, t])Note: You'll also need to memoize
setHoveredPartand useuseMemoforctxto get the full benefit.
132-138: Timeout cleanup doesn't clear the timeout handle on successful completion.The 10-second timeout will still fire and call
cleanup()again even if the drag completed successfully viaonMouseUp. Whilecleanup()is idempotent, storing and clearing the timeout handle would be cleaner.♻️ Suggested improvement
+ let timeoutHandle: ReturnType<typeof setTimeout> | undefined + const onMouseUp = (e: MouseEvent) => { // detach from the mouse document.removeEventListener('mousemove', onMove) document.removeEventListener('mouseup', onMouseUp) + if (timeoutHandle) clearTimeout(timeoutHandle) // ... rest of function } document.addEventListener('mousemove', onMove) document.addEventListener('mouseup', onMouseUp) - setTimeout(() => { + timeoutHandle = setTimeout(() => { // after the timeout we want to bail out in case something went wrong
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
packages/blueprints-integration/src/ingest.tspackages/blueprints-integration/src/triggers.tspackages/blueprints-integration/src/userEditing.tspackages/corelib/src/dataModel/UserEditingDefinitions.tspackages/job-worker/src/blueprints/context/lib.tspackages/meteor-lib/src/triggers/RundownViewEventBus.tspackages/meteor-lib/src/triggers/actionFactory.tspackages/shared-lib/src/core/model/ShowStyle.tspackages/webui/src/client/lib/ui/pieceUiClassNames.tspackages/webui/src/client/styles/elementSelected.scsspackages/webui/src/client/ui/RundownView.tsxpackages/webui/src/client/ui/RundownView/DragContext.tspackages/webui/src/client/ui/RundownView/DragContextProvider.tsxpackages/webui/src/client/ui/SegmentTimeline/Parts/SourceLayer.tsxpackages/webui/src/client/ui/SegmentTimeline/SourceLayerItem.tsxpackages/webui/src/client/ui/Settings/components/triggeredActions/actionEditors/actionSelector/ActionSelector.tsx
🧰 Additional context used
🧬 Code graph analysis (5)
packages/webui/src/client/ui/RundownView/DragContextProvider.tsx (9)
packages/shared-lib/src/core/model/Ids.ts (2)
PieceInstanceId(15-15)PartInstanceId(18-18)packages/corelib/src/dataModel/Ids.ts (1)
SegmentId(67-67)packages/webui/src/client/ui/Collections.tsx (1)
UIParts(48-48)packages/webui/src/client/lib/clientUserAction.ts (2)
doUserAction(154-260)UserAction(13-13)packages/corelib/src/lib.ts (1)
literal(27-27)packages/blueprints-integration/src/ingest.ts (1)
DefaultUserOperationRetimePiece(165-174)packages/meteor-lib/src/triggers/RundownViewEventBus.ts (1)
EditModeEvent(78-80)packages/webui/src/meteor/minimongo/common.js (1)
s(1439-1439)packages/webui/src/client/ui/RundownView/DragContext.ts (2)
IDragContext(5-43)dragContext(45-45)
packages/webui/src/client/ui/RundownView/DragContext.ts (2)
packages/shared-lib/src/core/model/Ids.ts (2)
PartInstanceId(18-18)PieceInstanceId(15-15)packages/corelib/src/dataModel/Ids.ts (1)
SegmentId(67-67)
packages/webui/src/client/ui/SegmentTimeline/Parts/SourceLayer.tsx (3)
packages/webui/src/client/ui/RundownView/DragContext.ts (1)
dragContext(45-45)packages/job-worker/src/ingest/model/implementation/IngestPartModelImpl.ts (1)
pieces(74-76)packages/webui/src/client/lib/lib.tsx (1)
contextMenuHoldToDisplayTime(27-29)
packages/webui/src/client/ui/SegmentTimeline/SourceLayerItem.tsx (3)
packages/webui/src/client/ui/RundownView/DragContext.ts (1)
dragContext(45-45)packages/shared-lib/src/lib/protectedString.ts (1)
unprotectString(35-37)packages/webui/src/client/ui/SegmentTimeline/Renderers/MicSourceRenderer.tsx (1)
MicSourceRenderer(15-227)
packages/webui/src/client/ui/RundownView.tsx (2)
packages/webui/src/client/ui/RundownView/DragContextProvider.tsx (1)
DragContextProvider(24-176)packages/webui/src/client/ui/RundownView/SelectedElementsContext.tsx (1)
SelectedElementsContext(137-137)
🔇 Additional comments (20)
packages/meteor-lib/src/triggers/RundownViewEventBus.ts (1)
32-32: LGTM! Consistent event infrastructure.The new EDIT_MODE event follows the established pattern for similar events (e.g., SHELF_STATE). The interface and event bus mapping are properly integrated.
Also applies to: 78-80, 147-147
packages/webui/src/client/styles/elementSelected.scss (1)
22-24: LGTM! Clean CSS addition.The
.draggable-elementclass provides clear visual indication for draggable items.packages/shared-lib/src/core/model/ShowStyle.ts (1)
110-110: LGTM! Enum value added correctly.The new
editModeaction is properly added to theClientActionsenum.packages/webui/src/client/ui/Settings/components/triggeredActions/actionEditors/actionSelector/ActionSelector.tsx (1)
96-106: LGTM! Edit mode handling follows established patterns.The implementation for
getArguments,hasArguments, andactionToLabelcorrectly mirrors the pattern used for theshelfaction with proper state handling (Enable/Disable/Toggle).Also applies to: 161-162, 209-210
packages/blueprints-integration/src/triggers.ts (2)
275-279:IEditModeActionaddition looks consistent with other GUI-only actionsMatches the existing
IShelfActionpattern (GUI-onlyfilterChain,statesupports toggle), so this should integrate cleanly.
320-341:SomeActionunion extension is correctIncluding
IEditModeActioninSomeActionis the right wiring to make the action usable in triggered actions.Also applies to: 334-335
packages/meteor-lib/src/triggers/actionFactory.ts (2)
453-458: Switch wiring forClientActions.editModelooks correctRoutes to the new action creator the same way as
ClientActions.shelf.
289-298:createEditModeActionimplementation is sound—event is properly consumedThe
EDIT_MODEevent is defined inRundownViewEventBus, emitted in this action, and has an active listener inDragContextProvider.tsx(onSetEditModehandler) that applies the state. No further action needed.packages/webui/src/client/ui/SegmentTimeline/Parts/SourceLayer.tsx (1)
97-101: Drag-awarepieceslist composition is intentional and correctly implementedThe conditional injection of the dragged piece only in its source layer (when
dragCtx.piece.sourceLayer?._id === props.layer._id) is the intended design. ThesetHoveredPart()call only updates internal tracking data for hover position and part, not visual rendering. No ghost or preview rendering is implemented elsewhere in the codebase—the dragged piece stays visually in its source layer during hover, which is the expected behavior.packages/webui/src/client/ui/RundownView/DragContext.ts (1)
1-45: LGTM! Clean interface design for drag context.The interface is well-documented with clear JSDoc comments. The context typing as
IDragContext | undefinedcorrectly allows for cases where the context is not provided.Minor notes:
- Line 12: The comment "consider omitting this is a todo" reads awkwardly. Consider rewording to "consider omitting this. This is a TODO" or similar.
- Line 45: The informal comment
// slay.could be removed for consistency.packages/blueprints-integration/src/ingest.ts (2)
127-134: LGTM! Consistent enum extension.The new
RETIME_PIECEenum value follows the established naming convention with the__sofie-prefix.
165-182: LGTM! Well-structured operation type.The
DefaultUserOperationRetimePiecetype follows the existing pattern for other operations. The comment about future duration support is helpful for maintainability.packages/job-worker/src/blueprints/context/lib.ts (2)
527-555: LGTM! Consistent use ofliteral<>()pattern.The refactor from
satisfies Complete<...>toliteral<...>()aligns with the existing patterns in this file (e.g., line 152literal<Complete<IBlueprintPieceInstance['infinite']>>). This maintains type safety while improving consistency.
586-618: LGTM! Symmetric refactor intranslateUserEditsFromBlueprint.The changes mirror those in
translateUserEditsToBlueprint, maintaining consistency across both translation directions.packages/webui/src/client/ui/RundownView.tsx (2)
1371-1580: LGTM! Proper integration of DragContextProvider.The
DragContextProvideris correctly placed to wrap theSelectedElementsContext.Consumer, ensuring drag context is available to all child components. TheErrorBoundarywrapping around various UI sections provides good error isolation.
1408-1422: All Shelf component props are correctly typed.Verification confirms that
onChangeBottomMargin,rundownLayout, andstudioare all properly declared in theIShelfPropsinterface with appropriate types:
onChangeBottomMargin?: (newBottomMargin: string) => voidrundownLayout?: RundownLayoutShelfBasestudio: UIStudioNo issues found.
packages/webui/src/client/ui/SegmentTimeline/SourceLayerItem.tsx (4)
124-127: LGTM! Clear logic for determining draggable state.The check correctly identifies pieces with the RETIME_PIECE operation, ensuring only appropriately configured pieces can be dragged.
568-593: LGTM! Clean key extraction for renderers.Extracting
elKeyseparately from the spread props is the correct approach since React keys cannot be spread. The comment on line 568-569 provides good documentation.
203-206: Theas anycast could be replaced with a type guard, butlimitToCurrentPartis already defined in the type.The property
limitToCurrentPart?: booleanis already present in theUserEditingDefinitionSofieDefaultinterface (line 64 ofpackages/blueprints-integration/src/userEditing.ts). Theas anycast exists because thefind()method returns a union type (UserEditingDefinition) that TypeScript cannot automatically narrow based on the condition alone.A better approach would be to use a type guard instead of the
as anycast, but this is a minor code quality improvement rather than a missing type definition issue.Likely an incorrect or invalid review comment.
614-618: Theundefinedargument on line 616 is intentional and correctly corresponds to the optionaluiStateparameter in the function signature. The inline comment// this.statealready documents its purpose, indicating this parameter previously held component state but is now intentionally omitted—likely due to refactoring away from class component state. No change needed.Likely an incorrect or invalid review comment.
packages/webui/src/client/ui/RundownView/DragContextProvider.tsx
Outdated
Show resolved
Hide resolved
nytamin
left a comment
nytamin
left a comment
There was a problem hiding this comment.
Looks good! I left a few minor comments.
| /** | ||
| * Whether dragging is enabled | ||
| */ | ||
| enabled: boolean |
There was a problem hiding this comment.
Minor note: The meaning "enabled" could be misunderstood at a glance (does it mean that an element can be dragged, or is currently being dragged? Perhaps clarify the description?
| document.removeEventListener('mousemove', onMove) | ||
| document.removeEventListener('mouseup', onMouseUp) |
There was a problem hiding this comment.
DRY: Should the removal of the events be moved to the cleanup() function (alternatively another cleanupEvents() function? If yes, also make sure that the cleanup is run upon every return from this function, and in the setTimeout further down.
| const segmentIdRef = useRef<undefined | SegmentId>(undefined) | ||
| const limitToPartRef = useRef<undefined | PartInstanceId>(undefined) | ||
|
|
||
| const startDrag = ( |
There was a problem hiding this comment.
Minor note: I don't know if this is true for ContextProviders or not, but should the methods in this component be wrapped in a React.useCallback for performance reasons?
- Move event listener removal into cleanup() to avoid duplication - Fix missing cleanup() call when segment is not found
rjmunro
force-pushed
the
feat/ui-piece-retiming
branch
from
97556fc to
99b01d6
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @packages/webui/src/client/ui/RundownView/DragContextProvider.tsx:
- Around line 132-135: The untracked setTimeout in DragContextProvider that
calls cleanup() after DRAG_TIMEOUT can clear state for subsequent drags; store
the timeout ID when you call setTimeout (e.g., local variable or ref like
dragTimeoutId) and clear it with clearTimeout(...) whenever a new drag starts or
when cleanup() runs (also set the ref to null after clearing) so the timeout for
an earlier drag cannot fire during a later drag; update the code paths around
setTimeout, cleanup(), and the drag-start logic to set/clear this tracked
timeout.
In @packages/webui/src/client/ui/SegmentTimeline/SourceLayerItem.tsx:
- Around line 195-224: The useCallback for itemMouseDown captures
part.instance.segmentId but part is missing from the dependency array; update
the useCallback dependency array for itemMouseDown to include part (alongside
piece, timeScale, dragCtx and any other used external symbols like
hasDraggableElement) so the closure always sees the current
part.instance.segmentId when calling dragCtx.startDrag.
🧹 Nitpick comments (4)
packages/webui/src/client/ui/Settings/components/triggeredActions/actionEditors/actionSelector/ActionSelector.tsx (1)
394-427: Consider aligning CSS classes with theshelfaction editor for consistency.The
editModeeditor usesclassName="mts"andclassNames="input text-input input-m"while the adjacentshelfeditor (lines 361-393) usesclassName="mt-2"andclassNames="mb-2". This may result in inconsistent spacing and styling between similar action editors.Suggested fix for consistency
case ClientActions.editMode: return ( - <div className="mts"> + <div className="mt-2"> <label className="block">{t('State')}</label> <DropdownInputControl<typeof action.state> - classNames="input text-input input-m" + classNames="mb-2" value={action.state}packages/webui/src/client/ui/SegmentTimeline/Parts/SourceLayer.tsx (1)
97-100: Consider potential performance concern with piece reordering on every render.The
piecescomputation runs on every render. When a drag is active (dragCtx?.piece), it filters and concatenates arrays. For layers with many pieces, consider memoizing this computation.Optional memoization
+import React, { MouseEventHandler, useCallback, useContext, useMemo, useState } from 'react' ... - const pieces = - dragCtx?.piece && dragCtx.piece.sourceLayer?._id === props.layer._id - ? (props.layer.pieces ?? []).filter((p) => p.instance._id !== dragCtx.piece?.instance._id).concat(dragCtx.piece) - : props.layer.pieces + const pieces = useMemo(() => { + if (dragCtx?.piece && dragCtx.piece.sourceLayer?._id === props.layer._id) { + return (props.layer.pieces ?? []) + .filter((p) => p.instance._id !== dragCtx.piece?.instance._id) + .concat(dragCtx.piece) + } + return props.layer.pieces + }, [dragCtx?.piece, props.layer._id, props.layer.pieces])packages/webui/src/client/ui/SegmentTimeline/SourceLayerItem.tsx (2)
124-127: Consider extracting the retime operation once to avoid duplicate lookups.The same
findoperation to locate the retime user operation is performed here and again initemMouseDown(lines 204-206). Consider storing the found operation to avoid the duplicate search.♻️ Suggested refactor
const dragCtx = useContext(dragContext) - const hasDraggableElement = !!piece.instance.piece.userEditOperations?.find( + const retimeOperation = piece.instance.piece.userEditOperations?.find( (op) => op.type === UserEditingType.SOFIE && op.id === DefaultUserOperationsTypes.RETIME_PIECE ) + const hasDraggableElement = !!retimeOperationThen in
itemMouseDown, useretimeOperationdirectly instead of searching again.
204-208: Consider using proper typing instead ofas any.The
as anytype assertion on line 206 bypasses TypeScript's type checking. IflimitToCurrentPartis a known property on the RETIME_PIECE operation type, consider using a more specific type assertion or type guard.♻️ Example approach
- const retimeOp = piece.instance.piece.userEditOperations?.find( - (op) => op.type === UserEditingType.SOFIE && op.id === DefaultUserOperationsTypes.RETIME_PIECE - ) as any + const retimeOp = piece.instance.piece.userEditOperations?.find( + (op): op is UserEditingDefinitionSofie & { limitToCurrentPart?: boolean } => + op.type === UserEditingType.SOFIE && op.id === DefaultUserOperationsTypes.RETIME_PIECE + )Alternatively, define a proper type for the RETIME_PIECE operation that includes
limitToCurrentPart.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
packages/blueprints-integration/src/ingest.tspackages/blueprints-integration/src/triggers.tspackages/blueprints-integration/src/userEditing.tspackages/corelib/src/dataModel/UserEditingDefinitions.tspackages/job-worker/src/blueprints/context/lib.tspackages/meteor-lib/src/triggers/RundownViewEventBus.tspackages/meteor-lib/src/triggers/actionFactory.tspackages/shared-lib/src/core/model/ShowStyle.tspackages/webui/src/client/lib/ui/pieceUiClassNames.tspackages/webui/src/client/styles/elementSelected.scsspackages/webui/src/client/ui/RundownView.tsxpackages/webui/src/client/ui/RundownView/DragContext.tspackages/webui/src/client/ui/RundownView/DragContextProvider.tsxpackages/webui/src/client/ui/SegmentTimeline/Parts/SourceLayer.tsxpackages/webui/src/client/ui/SegmentTimeline/SourceLayerItem.tsxpackages/webui/src/client/ui/Settings/components/triggeredActions/actionEditors/actionSelector/ActionSelector.tsx
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/shared-lib/src/core/model/ShowStyle.ts
- packages/meteor-lib/src/triggers/RundownViewEventBus.ts
- packages/corelib/src/dataModel/UserEditingDefinitions.ts
- packages/webui/src/client/ui/RundownView/DragContext.ts
- packages/blueprints-integration/src/triggers.ts
- packages/webui/src/client/lib/ui/pieceUiClassNames.ts
🧰 Additional context used
🧬 Code graph analysis (4)
packages/webui/src/client/ui/SegmentTimeline/Parts/SourceLayer.tsx (3)
packages/webui/src/client/ui/RundownView/DragContext.ts (1)
dragContext(46-46)packages/job-worker/src/ingest/model/implementation/IngestPartModelImpl.ts (1)
pieces(74-76)packages/webui/src/client/lib/lib.tsx (1)
contextMenuHoldToDisplayTime(27-29)
packages/webui/src/client/ui/RundownView.tsx (2)
packages/webui/src/client/ui/RundownView/DragContextProvider.tsx (1)
DragContextProvider(24-183)packages/webui/src/client/ui/RundownView/SelectedElementsContext.tsx (1)
SelectedElementsContext(137-137)
packages/job-worker/src/blueprints/context/lib.ts (5)
packages/corelib/src/lib.ts (2)
literal(27-27)omit(36-38)packages/blueprints-integration/src/userEditing.ts (3)
UserEditingDefinitionAction(18-36)UserEditingDefinitionForm(41-51)UserEditingDefinitionSofieDefault(56-65)packages/corelib/src/dataModel/UserEditingDefinitions.ts (3)
CoreUserEditingDefinitionAction(15-31)CoreUserEditingDefinitionForm(36-48)CoreUserEditingDefinitionSofie(95-104)packages/corelib/src/TranslatableMessage.ts (1)
wrapTranslatableMessageFromBlueprints(102-110)packages/shared-lib/src/lib/protectedString.ts (1)
unprotectStringArray(38-40)
packages/webui/src/client/ui/SegmentTimeline/SourceLayerItem.tsx (2)
packages/webui/src/client/ui/RundownView/DragContext.ts (1)
dragContext(46-46)packages/shared-lib/src/lib/protectedString.ts (1)
unprotectString(35-37)
🔇 Additional comments (17)
packages/meteor-lib/src/triggers/actionFactory.ts (2)
289-298: Implementation follows established pattern correctly.The
createEditModeActionfunction mirrors the structure ofcreateShelfActionand other similar action creators in this file. The implementation is consistent with the codebase conventions.Note: The AI summary claims there are "two identical function declarations" of
createEditModeAction, but only one definition is present in the code. This appears to be an AI summary inconsistency.
456-457: LGTM!The switch case correctly delegates to
createEditModeActionfollowing the same pattern as the adjacentClientActions.shelfcase. TheClientActions.editModeenum is properly defined inshared-lib, thecreateEditModeActionfunction has the correct signature, and the action type interface includes both requiredstateandfilterChainproperties.packages/webui/src/client/styles/elementSelected.scss (1)
22-24: LGTM!Simple CSS addition for draggable element visual feedback. The dotted border provides clear drag affordance.
packages/blueprints-integration/src/userEditing.ts (1)
60-64: LGTM!The optional
limitToCurrentPartproperty is well-documented and appropriately scoped to RETIME_PIECE operations.packages/webui/src/client/ui/Settings/components/triggeredActions/actionEditors/actionSelector/ActionSelector.tsx (1)
96-106: LGTM!The
editModecase correctly mirrors the existingshelfaction pattern with appropriate Enable/Disable/Toggle labels and exhaustive state handling viaassertNever.packages/blueprints-integration/src/ingest.ts (1)
165-174: LGTM!The
DefaultUserOperationRetimePiecetype is well-structured and follows the established pattern for default user operations. The comment noting future duration support is helpful for maintainability.packages/job-worker/src/blueprints/context/lib.ts (1)
544-549: LGTM!The
limitToCurrentPartproperty is correctly mapped in bothtranslateUserEditsToBlueprintandtranslateUserEditsFromBlueprintfunctions, ensuring proper bidirectional conversion between core and blueprint types.packages/webui/src/client/ui/SegmentTimeline/Parts/SourceLayer.tsx (1)
102-107: LGTM!The
onMouseEnterhandler correctly updates the drag context with the hovered part information, enabling cross-part dragging. The early return whendragCtxis undefined is appropriate defensive coding.packages/webui/src/client/ui/RundownView/DragContextProvider.tsx (4)
140-151: LGTM!The
setHoveredPartcallback has appropriate guards: checking for active drag (pieceId), avoiding redundant updates (same part), and respecting segment/part constraints via refs.
161-166: LGTM!Proper cleanup pattern for the event bus subscription using the useEffect return function.
67-116: This review comment is based on a misunderstanding ofdoUserAction's behavior.The
doUserActionfunction already calls the callback in all cases: when there's an error response, when the operation succeeds, and when an exception is caught in the promise.catch()block. The callback parameter(err: any, res?: Result) => void | booleanis invoked with an error argument in both the error response path (line 195:callback(res)) and the exception path (line 241:callback(err)).Therefore,
cleanup()will be called regardless of success or failure—no additional error handling is needed.Likely an incorrect or invalid review comment.
23-23: The cross-rundown drag constraint already exists in the codebase. Dragging is limited to segments viasegmentIdRefinsetHoveredPart(line 144), which effectively prevents cross-rundown dragging since each segment belongs to exactly one rundown. The comment at line 23 is outdated and does not reflect the current implementation.Likely an incorrect or invalid review comment.
packages/webui/src/client/ui/RundownView.tsx (2)
1371-1380: LGTM: DragContextProvider integration looks correct.The
DragContextProvideris properly positioned in the component hierarchy, wrapping the content that needs drag context access. Thettranslation function is correctly passed from the translated component.
1386-1422: LGTM: Error boundary wrappers and component structure.The ErrorBoundary components provide appropriate error isolation for each major UI section, preventing failures in one component from crashing the entire view.
packages/webui/src/client/ui/SegmentTimeline/SourceLayerItem.tsx (3)
177-194: LGTM: Double-click handler updates.The unconditional
preventDefault()andstopPropagation()calls are appropriate for this interactive element to prevent text selection and event bubbling when double-clicking pieces.
568-593: LGTM: Key extraction and renderer updates.Extracting
elKeyonce and reusing it across all renderer components is cleaner than repeatingunprotectString(piece.instance._id)multiple times.
604-618: LGTM: Drag state visual feedback integration.The
hasDraggableElement && dragCtx?.enabledcondition correctly applies the draggable styling only when the piece supports retiming and edit mode is enabled.
| setTimeout(() => { | ||
| // after the timeout we want to bail out in case something went wrong | ||
| cleanup() | ||
| }, DRAG_TIMEOUT) |
There was a problem hiding this comment.
Timeout cleanup can interfere with subsequent drag operations.
The setTimeout at line 132-135 calls cleanup() unconditionally after 10 seconds. If the user completes a drag successfully and starts a new drag within that 10-second window, the timeout from the first drag will clear the state of the second drag.
Recommended fix: Track and clear the timeout
const startDrag = useCallback(
(
ogPiece: PieceUi,
timeScale: number,
pos: { x: number; y: number },
elementOffset?: number,
limitToPart?: PartInstanceId,
limitToSegment?: SegmentId
) => {
if (pieceId) return // a drag is currently in progress....
const inPoint = ogPiece.renderedInPoint ?? 0
segmentIdRef.current = limitToSegment
limitToPartRef.current = limitToPart
positionRef.current = pos
setPieceId(ogPiece.instance._id)
let localPiece = ogPiece
+ let timeoutId: ReturnType<typeof setTimeout> | undefined
const onMove = (e: MouseEvent) => {
// ... existing code
}
const onMouseUp = (e: MouseEvent) => {
// ... existing code
}
const cleanup = () => {
+ if (timeoutId) {
+ clearTimeout(timeoutId)
+ timeoutId = undefined
+ }
document.removeEventListener('mousemove', onMove)
document.removeEventListener('mouseup', onMouseUp)
setPieceId(undefined)
setPiece(undefined)
partIdRef.current = undefined
segmentIdRef.current = undefined
}
document.addEventListener('mousemove', onMove)
document.addEventListener('mouseup', onMouseUp)
- setTimeout(() => {
+ timeoutId = setTimeout(() => {
cleanup()
}, DRAG_TIMEOUT)
},
[pieceId, t]
)🤖 Prompt for AI Agents
In @packages/webui/src/client/ui/RundownView/DragContextProvider.tsx around
lines 132 - 135, The untracked setTimeout in DragContextProvider that calls
cleanup() after DRAG_TIMEOUT can clear state for subsequent drags; store the
timeout ID when you call setTimeout (e.g., local variable or ref like
dragTimeoutId) and clear it with clearTimeout(...) whenever a new drag starts or
when cleanup() runs (also set the ref to null after clearing) so the timeout for
an earlier drag cannot fire during a later drag; update the code paths around
setTimeout, cleanup(), and the drag-start logic to set/clear this tracked
timeout.
| /** | ||
| * Whether dragging is enabled | ||
| */ | ||
| enabled: boolean |
| const segmentIdRef = useRef<undefined | SegmentId>(undefined) | ||
| const limitToPartRef = useRef<undefined | PartInstanceId>(undefined) | ||
|
|
||
| const startDrag = ( |
| document.removeEventListener('mousemove', onMove) | ||
| document.removeEventListener('mouseup', onMouseUp) |
packages/webui/src/client/ui/RundownView/DragContextProvider.tsx
Outdated
Show resolved
Hide resolved
4 participants
About the Contributor
This pull request is posted on behalf of the BBC.
Type of Contribution
This is a Feature.
Current Behavior
Items in the timeline are fixed.
New Behavior
If a piece has a
userEditOperationof typeUserEditingType.SOFIEand idDefaultUserOperationsTypes.RETIME_PIECEand editing is enabled, you can drag it around the timeline.Dragging.demo.mov
Testing
Affected areas
Time Frame
Other Information
This is a rebase of bbc#35 to the upstream release 53.
Status
UI Piece Retiming Feature
Adds support for dragging and retiming timeline pieces in the Rundown view UI when the feature is enabled through user edit operations.
Changes Overview
Blueprint Integration & Type Definitions
RETIME_PIECEoperation type inDefaultUserOperationsTypesDefaultUserOperationRetimePieceinterface with payload containingsegmentExternalId,partExternalId, andinPointDefaultUserOperationsunion to include the new operation typelimitToCurrentPartfield toUserEditingDefinitionSofieDefaultandCoreUserEditingDefinitionSofieEdit Mode Action Support
editModemember toClientActionsenumIEditModeActioninterface withstate(true/false/'toggle') and filter chain supportEDIT_MODEevent type inRundownViewEventswithEditModeEventpayloadcreateEditModeActionfor emitting edit mode eventsDrag & Drop Implementation
IDragContext,dragContext) to manage drag lifecycleDragContextProvidercomponent that:RundownViewEventBusfor edit mode togglingstartDragandsetHoveredPartmethods for drag managementUI Components
RundownViewto wrap content withDragContextProviderSourceLayerto integrate with drag context, reordering pieces during drag operations and updating hover stateSourceLayerItemto:userEditOperations(retime piece operations)draggable-elementclassName to draggable piecespieceUiClassNamesfunction to accept optionaldraggableparameter and apply stylingStyling & UI Editor
.draggable-elementSCSS class with dotted white border stylingActionSelector) to supporteditModeaction configuration with state dropdown (Enable/Disable/Toggle)Internal Implementation
job-workercontext library to handlelimitToCurrentPartfield and use explicit literal type wrappersCurrent Status
packages/job-worker/src/blueprints/context/lib.ts)