feat: improve UX in properties editor#1620
Merged
Saftret merged 3 commits intoSofie-Automation:mainfrom Mar 20, 2026
Merged
Conversation
WalkthroughAdded a CLOSE_NOTIFICATIONS event and handlers to coordinate NotificationCenter and PropertiesPanel visibility; introduced hasUserEditableContent checks to gate edit actions and double-click flows; updated properties panel styling and layout; adjusted rundown view padding and conditional rendering between panels. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SourceLayerItem
participant EventBus as RundownViewEventBus
participant RundownView
participant Panels as Notification/Properties Panels
User->>SourceLayerItem: double-click piece/segment
alt content not editable
SourceLayerItem-->>User: no action (blocked)
else content editable
SourceLayerItem->>EventBus: emit CLOSE_NOTIFICATIONS
EventBus->>RundownView: CLOSE_NOTIFICATIONS event
RundownView->>RundownView: clear isNotificationsCenterOpen
RundownView->>Panels: re-render -> show PropertiesPanel, hide NotificationCenter
Panels-->>User: PropertiesPanel shown
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
🚥 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 Tip Migrating from UI to YAML configuration.Use the |
Contributor
Author
|
Note: this PR was created with wrong target first in #1620 62 |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Saftret
added
the
Contribution from BBC
Contributor
|
@nytamin Images are now added to the description |
nytamin
approved these changes
Mar 17, 2026
Julusian
force-pushed
the
feat/SOFIE-284
branch
from
b3ebcf3 to
d43cf78
Compare
![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
🤖 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/webui/src/client/styles/propertiesPanel.scss`:
- Around line 236-244: The .propertiespanel-pop-up__button rule inside
.propertiespanel-pop-up__contents is missing display:flex so its gap: 0.5em has
no effect and Stylelint flags the blank line; update the
.propertiespanel-pop-up__button selector (same block shown) to include display:
flex and remove the empty line before gap so the styles match the
.propertiespanel-pop-up__footer pattern and gap takes effect.
In `@packages/webui/src/client/styles/rundownView.scss`:
- Around line 212-221: Remove the extra blank line immediately before the
background declaration so the CSS/Sass rule has no empty line before the
"background:" property; locate the block containing the "background:
linear-gradient(...)" lines (the repeated linear-gradient entries using
$color-status-fatal) and delete the blank line above it to satisfy stylelint's
declaration-empty-line-before rule.
In `@packages/webui/src/client/ui/RundownView.tsx`:
- Around line 911-915: The onCloseNotifications handler currently only clears
component state (isNotificationsCenterOpen) which leaves the global singleton
flag NotificationCenter.isOpen out of sync; update onCloseNotifications to also
set NotificationCenter.isOpen = false when closing, and apply the same change to
any other auto-close paths (the handlers that open properties via the event bus
/ context menu that currently only clear isNotificationsCenterOpen) so the
global NotificationCenter.isOpen and the React state remain consistent.
In `@packages/webui/src/client/ui/SegmentTimeline/SegmentTimeline.tsx`:
- Around line 1017-1025: Double-click handler in SegmentTimeline should close
notifications before changing selection: when enableUserEdits is true and
hasUserEditableContent(segment) passes, emit the same CLOSE_NOTIFICATIONS event
used in SourceLayerItem.tsx (using the app's notification/event emitter or
context) prior to calling selectElementContext.clearAndSetSelection({ type:
'segment', elementId: segment._id }), and only then proceed to set selection (or
clearAndSetSelection) so the right-hand panel isn't suppressed; update the
onDoubleClick branch around hasUserEditableContent, referencing the existing
symbols hasUserEditableContent, selectElementContext.clearAndSetSelection,
segment._id, and CLOSE_NOTIFICATIONS.
In `@packages/webui/src/client/ui/UserEditOperations/PropertiesPanel.tsx`:
- Around line 25-29: The import of CoreUserEditingDefinition and
CoreUserEditingProperties currently points to
`@sofie-automation/corelib/src/dataModel/UserEditingDefinitions.js`; update that
import to use `@sofie-automation/corelib/dist/dataModel/UserEditingDefinitions` so
it matches other imports (e.g., in RenderUserEditOperations.tsx and
SegmentContextMenu.tsx) and avoids breaking module resolution outside the
monorepo, keeping the imported symbol names (CoreUserEditingDefinition,
CoreUserEditingProperties) unchanged.
- Around line 394-408: The helper hasUserEditableContent currently treats any
userEditOperations as making the panel editable, which causes empty panels for
standalone ops like RETIME_PIECE; change it so it only returns true when
userEditProperties contains real content OR when userEditOperations includes at
least one operation that is actually rendered in the PropertiesPanel.
Concretely, update hasUserEditableContent to filter userEditOperations (e.g.
obj.userEditOperations.some(op => /* op has panel UI */ op.properties ||
op.fields || op.panel || allowedTypes.includes(op.type))) and ignore known
standalone ops such as RETIME_PIECE (see SourceLayerItem.tsx) so only ops that
provide panel fields cause a true result.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ca488cc6-7026-43b6-bdf3-7fff29c5a8e1
📒 Files selected for processing (8)
packages/meteor-lib/src/triggers/RundownViewEventBus.tspackages/webui/src/client/styles/propertiesPanel.scsspackages/webui/src/client/styles/rundownView.scsspackages/webui/src/client/ui/RundownView.tsxpackages/webui/src/client/ui/SegmentTimeline/SegmentContextMenu.tsxpackages/webui/src/client/ui/SegmentTimeline/SegmentTimeline.tsxpackages/webui/src/client/ui/SegmentTimeline/SourceLayerItem.tsxpackages/webui/src/client/ui/UserEditOperations/PropertiesPanel.tsx
Comment on lines
+911
to
+915
| private onCloseNotifications = () => { | ||
| this.setState({ | ||
| isNotificationsCenterOpen: undefined, | ||
| }) | ||
| } |
There was a problem hiding this comment.
Sync the global notification-center flag when these paths auto-close it.
Lines 1200-1204 keep NotificationCenter.isOpen aligned with React state, but these new branches only clear isNotificationsCenterOpen. After opening properties through the event bus or the context menu, the singleton can still think the center is open, which makes later pop-up/toggle behavior drift out of sync.
Suggested change
private onCloseNotifications = () => {
+ NotificationCenter.isOpen = false
this.setState({
isNotificationsCenterOpen: undefined,
})
}
@@
onEditProps={(selection) => {
+ NotificationCenter.isOpen = false
this.setState({ isNotificationsCenterOpen: undefined })
selectionContext.clearAndSetSelection(selection)
}}Also applies to: 1536-1539
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/webui/src/client/ui/RundownView.tsx` around lines 911 - 915, The
onCloseNotifications handler currently only clears component state
(isNotificationsCenterOpen) which leaves the global singleton flag
NotificationCenter.isOpen out of sync; update onCloseNotifications to also set
NotificationCenter.isOpen = false when closing, and apply the same change to any
other auto-close paths (the handlers that open properties via the event bus /
context menu that currently only clear isNotificationsCenterOpen) so the global
NotificationCenter.isOpen and the React state remain consistent.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/webui/src/client/ui/UserEditOperations/PropertiesPanel.tsx`:
- Around line 394-406: hasUserEditableContent currently returns true when
wrapper objects exist even if their schemas are empty; update it to only return
true when there is actual schema/action content: check
obj?.userEditProperties?.pieceTypeProperties?.schema contains entries (e.g.
Object.keys(...).length > 0), check
obj?.userEditProperties?.globalProperties?.schema contains entries, and check
obj?.userEditProperties?.operations has at least one operation that itself
contains content (e.g. operation.schema has entries or operation.actions?.length
> 0); use these precise checks in hasUserEditableContent to mirror what
PropertiesPanel actually renders.
- Around line 43-56: Replace the sticky hadPiece boolean with a tracked
last-piece identity: add a ref/state like lastPieceIdRef and update it whenever
piece is defined (e.g., set lastPieceIdRef.current = piece.id inside the effect
that watches piece), then change the clearing logic in the effect that
references selectedElement and piece to only call clearSelections when
selectedElement?.type === 'piece' && piece === undefined &&
lastPieceIdRef.current !== selectedElement.id (and reset lastPieceIdRef.current
when you clear), so brief intermediate undefined renders for a newly selected
piece do not trigger clearing; reference hadPiece/setHadPiece, piece,
selectedElement, and clearSelections when making these changes.
- Around line 210-216: The inline SVG in PropertiesPanel.tsx next to the span
with class "propertiespanel-pop-up__label-with-icon" is not decorative for
assistive tech and hardcodes its color; make it decorative by adding
aria-hidden="true" and focusable="false" to the <svg>, and replace the hardcoded
fill="#979797" with fill="currentColor" (or remove the fill and rely on CSS) so
the icon color follows CSS states (hover/disabled/theme); ensure the existing
svg className "svg" has the desired color rules in CSS to control appearance.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e0e5f2d4-9276-43bb-9080-9f5680fb6223
📒 Files selected for processing (5)
packages/webui/src/client/styles/propertiesPanel.scsspackages/webui/src/client/styles/rundownView.scsspackages/webui/src/client/ui/RundownView.tsxpackages/webui/src/client/ui/SegmentTimeline/SegmentTimeline.tsxpackages/webui/src/client/ui/UserEditOperations/PropertiesPanel.tsx
✅ Files skipped from review due to trivial changes (1)
- packages/webui/src/client/styles/rundownView.scss
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/webui/src/client/ui/SegmentTimeline/SegmentTimeline.tsx
- packages/webui/src/client/ui/RundownView.tsx
- packages/webui/src/client/styles/propertiesPanel.scss
Comment on lines
+43
to
+56
| const [hadPiece, setHadPiece] = useState(false) | ||
|
|
||
| useEffect(() => { | ||
| if (piece) setHadPiece(true) | ||
| }, [piece]) | ||
|
|
||
| useEffect(() => { | ||
| const pieceChangedId = selectedElement?.type === 'piece' && hadPiece && piece === undefined | ||
|
|
||
| if (pieceChangedId) { | ||
| setHadPiece(false) | ||
| clearSelections() | ||
| } | ||
| }, [selectedElement, piece, hadPiece, clearSelections]) |
There was a problem hiding this comment.
Tie hadPiece to the active selection.
Once any piece has resolved, hadPiece stays true until this branch fires, so pieceChangedId is no longer scoped to the current piece. A normal piece switch that briefly renders piece === undefined will be treated as an ID-change and clear the new selection. Track the last selected piece identity/ref instead of a sticky boolean.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/webui/src/client/ui/UserEditOperations/PropertiesPanel.tsx` around
lines 43 - 56, Replace the sticky hadPiece boolean with a tracked last-piece
identity: add a ref/state like lastPieceIdRef and update it whenever piece is
defined (e.g., set lastPieceIdRef.current = piece.id inside the effect that
watches piece), then change the clearing logic in the effect that references
selectedElement and piece to only call clearSelections when
selectedElement?.type === 'piece' && piece === undefined &&
lastPieceIdRef.current !== selectedElement.id (and reset lastPieceIdRef.current
when you clear), so brief intermediate undefined renders for a newly selected
piece do not trigger clearing; reference hadPiece/setHadPiece, piece,
selectedElement, and clearSelections when making these changes.
Comment on lines
+210
to
+216
| <svg className="svg" viewBox="0 0 20 15" fill="none" xmlns="http://www.w3.org/2000/svg"> | ||
| <path | ||
| d="M2 14.5251H15C16.3261 14.5251 17.5979 13.9984 18.5355 13.0607C19.4732 12.123 20 10.8512 20 9.52515C20 8.19906 19.4732 6.92729 18.5355 5.98961C17.5979 5.05193 16.3261 4.52515 15 4.52515H10V0.475147L5 5.47515L10 10.4751V6.52515H15C15.7956 6.52515 16.5587 6.84122 17.1213 7.40383C17.6839 7.96643 18 8.7295 18 9.52515C18 10.3208 17.6839 11.0839 17.1213 11.6465C16.5587 12.2091 15.7956 12.5251 15 12.5251H2V14.5251Z" | ||
| fill="#979797" | ||
| /> | ||
| </svg> | ||
| <span className="propertiespanel-pop-up__label-with-icon"> |
There was a problem hiding this comment.
Make the inline restore icon decorative and CSS-driven.
The button already has a text label, so this SVG should be hidden from assistive tech. Also, fill="#979797" freezes the icon color and won't follow hover/disabled/theme states.
Suggested change
- <svg className="svg" viewBox="0 0 20 15" fill="none" xmlns="http://www.w3.org/2000/svg">
+ <svg
+ className="svg"
+ viewBox="0 0 20 15"
+ fill="none"
+ xmlns="http://www.w3.org/2000/svg"
+ aria-hidden="true"
+ focusable="false"
+ >
<path
d="M2 14.5251H15C16.3261 14.5251 17.5979 13.9984 18.5355 13.0607C19.4732 12.123 20 10.8512 20 9.52515C20 8.19906 19.4732 6.92729 18.5355 5.98961C17.5979 5.05193 16.3261 4.52515 15 4.52515H10V0.475147L5 5.47515L10 10.4751V6.52515H15C15.7956 6.52515 16.5587 6.84122 17.1213 7.40383C17.6839 7.96643 18 8.7295 18 9.52515C18 10.3208 17.6839 11.0839 17.1213 11.6465C16.5587 12.2091 15.7956 12.5251 15 12.5251H2V14.5251Z"
- fill="#979797"
+ fill="currentColor"
/>
</svg>📝 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.
Suggested change
| <svg className="svg" viewBox="0 0 20 15" fill="none" xmlns="http://www.w3.org/2000/svg"> | |
| <path | |
| d="M2 14.5251H15C16.3261 14.5251 17.5979 13.9984 18.5355 13.0607C19.4732 12.123 20 10.8512 20 9.52515C20 8.19906 19.4732 6.92729 18.5355 5.98961C17.5979 5.05193 16.3261 4.52515 15 4.52515H10V0.475147L5 5.47515L10 10.4751V6.52515H15C15.7956 6.52515 16.5587 6.84122 17.1213 7.40383C17.6839 7.96643 18 8.7295 18 9.52515C18 10.3208 17.6839 11.0839 17.1213 11.6465C16.5587 12.2091 15.7956 12.5251 15 12.5251H2V14.5251Z" | |
| fill="#979797" | |
| /> | |
| </svg> | |
| <span className="propertiespanel-pop-up__label-with-icon"> | |
| <svg | |
| className="svg" | |
| viewBox="0 0 20 15" | |
| fill="none" | |
| xmlns="http://www.w3.org/2000/svg" | |
| aria-hidden="true" | |
| focusable="false" | |
| > | |
| <path | |
| d="M2 14.5251H15C16.3261 14.5251 17.5979 13.9984 18.5355 13.0607C19.4732 12.123 20 10.8512 20 9.52515C20 8.19906 19.4732 6.92729 18.5355 5.98961C17.5979 5.05193 16.3261 4.52515 15 4.52515H10V0.475147L5 5.47515L10 10.4751V6.52515H15C15.7956 6.52515 16.5587 6.84122 17.1213 7.40383C17.6839 7.96643 18 8.7295 18 9.52515C18 10.3208 17.6839 11.0839 17.1213 11.6465C16.5587 12.2091 15.7956 12.5251 15 12.5251H2V14.5251Z" | |
| fill="currentColor" | |
| /> | |
| </svg> | |
| <span className="propertiespanel-pop-up__label-with-icon"> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/webui/src/client/ui/UserEditOperations/PropertiesPanel.tsx` around
lines 210 - 216, The inline SVG in PropertiesPanel.tsx next to the span with
class "propertiespanel-pop-up__label-with-icon" is not decorative for assistive
tech and hardcodes its color; make it decorative by adding aria-hidden="true"
and focusable="false" to the <svg>, and replace the hardcoded fill="#979797"
with fill="currentColor" (or remove the fill and rely on CSS) so the icon color
follows CSS states (hover/disabled/theme); ensure the existing svg className
"svg" has the desired color rules in CSS to control appearance.
Comment on lines
+394
to
+406
| export function hasUserEditableContent( | ||
| obj: | ||
| | ReadonlyDeep<{ | ||
| userEditOperations?: CoreUserEditingDefinition[] | ||
| userEditProperties?: CoreUserEditingProperties | ||
| }> | ||
| | undefined | ||
| ): boolean { | ||
| return !!( | ||
| obj?.userEditProperties?.pieceTypeProperties || | ||
| obj?.userEditProperties?.globalProperties || | ||
| obj?.userEditProperties?.operations?.length | ||
| ) |
There was a problem hiding this comment.
hasUserEditableContent() should gate on real schema content, not wrapper presence.
Right now pieceTypeProperties or globalProperties being present is enough to return true, but this file only renders useful controls when those schemas actually contain entries. For example, an empty pieceTypeProperties.schema still passes this helper and gives you an effectively blank panel. Check for populated schema/action content here so the new open/close gating matches what PropertiesPanel can really render.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/webui/src/client/ui/UserEditOperations/PropertiesPanel.tsx` around
lines 394 - 406, hasUserEditableContent currently returns true when wrapper
objects exist even if their schemas are empty; update it to only return true
when there is actual schema/action content: check
obj?.userEditProperties?.pieceTypeProperties?.schema contains entries (e.g.
Object.keys(...).length > 0), check
obj?.userEditProperties?.globalProperties?.schema contains entries, and check
obj?.userEditProperties?.operations has at least one operation that itself
contains content (e.g. operation.schema has entries or operation.actions?.length
> 0); use these precise checks in hasUserEditableContent to mirror what
PropertiesPanel actually renders.
7 tasks
7 tasks
jstarpl
pushed a commit
to nrkno/sofie-nrk-core
that referenced
this pull request
Apr 10, 2026
* SOFIE-284 | improve UX in properties editor * chore: refactor out util * fix: address automated review feedback --------- Co-authored-by: Julian Waller <git@julusian.co.uk> Co-authored-by: Stig Roar Aftret <saftret@gmail.com>
jstarpl
added a commit
to nrkno/sofie-nrk-core
that referenced
this pull request
Apr 10, 2026
7 tasks
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
4 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 PR is made on behalf of the BBC
Type of Contribution
This is a:
Feature
Current Behavior
Close button ("x") is positioned incorrectly
"Restore from NRCS" icon is misaligned and too close to text
Properties Panel goes blank when piece ID changes after edit (should close instead)
Empty Properties Panel opens for items with no editable properties
Properties Panel doesn't open when notification panels are visible
Clicking Properties icon when notifications are open closes Properties instead of closing notifications
Sidebar has wrong background color when panel is open
Unnecessary spacing between Segments and Properties Panel
Operations lack proper margins
New Behavior
Close button is properly positioned
"Restore from NRCS" icon is correctly aligned with appropriate spacing
Properties Panel closes automatically when piece ID changes
Properties Panel doesn't open for items without editable properties
Properties Panel opens and auto-closes notifications when needed
Properties icon closes notifications and shows Properties Panel
Sidebar displays correct background color
Optimized spacing between Segments and Properties Panel
Operations display with appropriate margins
Testing
Affected areas
Properties editor in rundown view
Time Frame
Not urgent, but we would like to get this merged into the in-development release.
Other Information
Status