fix: only show Edit ... Properties menu items on items that actually have the DefaultUserOperationsTypes.UPDATE_PROPS defined#1685
Draft
jstarpl wants to merge 1 commit intoSofie-Automation:mainfrom
Conversation
…have the DefaultUserOperationsTypes.UPDATE_PROPS defined
WalkthroughModified SegmentContextMenu component to conditionally render edit menu items for Segment, Part, and Piece properties. Added a helper function Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes 🚥 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)
📝 Coding Plan
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 You can enable review details to help with troubleshooting, context usage and more.Enable the |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Contributor
Author
|
@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: 1
🧹 Nitpick comments (1)
packages/webui/src/client/ui/SegmentTimeline/SegmentContextMenu.tsx (1)
94-106: Consider using.some()instead of.find()for semantic clarity.Since the result is only used in a truthy context,
.some()more clearly expresses the intent of checking existence rather than retrieving the operation. Additionally, extracting this function outside the component would avoid recreation on each render, though the impact is negligible for a context menu.♻️ Proposed refactor
Extract above the component and use
.some():+const doesItemSupportUserEditUpdateProps = ( + userEditOperations: + | readonly ( + | ReadonlyObjectDeep<CoreUserEditingDefinitionAction> + | ReadonlyObjectDeep<CoreUserEditingDefinitionForm> + | ReadonlyObjectDeep<CoreUserEditingDefinitionSofie> + )[] + | undefined +): boolean => { + return ( + userEditOperations?.some( + (op) => op.type === UserEditingType.SOFIE && op.id === DefaultUserOperationsTypes.UPDATE_PROPS + ) ?? false + ) +} + export function SegmentContextMenu({Then remove lines 94-106 from inside the component.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/webui/src/client/ui/SegmentTimeline/SegmentContextMenu.tsx` around lines 94 - 106, The helper doesItemSupportUserEditUpdateProps currently uses .find() inside the component to check existence; extract this function out of the component and change its implementation to use .some() to express boolean intent (i.e., check userEditOperations.some(op => op.type === UserEditingType.SOFIE && op.id === DefaultUserOperationsTypes.UPDATE_PROPS)); keep the same name and signature so callers are unchanged and ensure imports/references to UserEditingType.SOFIE and DefaultUserOperationsTypes.UPDATE_PROPS remain available after moving the function.
🤖 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/SegmentTimeline/SegmentContextMenu.tsx`:
- Around line 313-337: The <hr /> can render alone when enableUserEdits is true
but none of the edit menu items are eligible; update the SegmentContextMenu
render so the divider is only output when at least one edit item will render by
either moving the <hr /> inside the same conditional checks that render the
MenuItem(s) or by computing a boolean like hasAnyEditPropsItem using segment,
part, piece and doesItemSupportUserEditUpdateProps (checking the same properties
used for the MenuItem rendering) and then rendering {enableUserEdits &&
hasAnyEditPropsItem && <hr />} before the MenuItem blocks; keep existing
onEditProps calls and the checks on segment.userEditProperties,
part.instance.part.userEditProperties, piece.instance.piece.userEditProperties
and their corresponding doesItemSupportUserEditUpdateProps calls.
---
Nitpick comments:
In `@packages/webui/src/client/ui/SegmentTimeline/SegmentContextMenu.tsx`:
- Around line 94-106: The helper doesItemSupportUserEditUpdateProps currently
uses .find() inside the component to check existence; extract this function out
of the component and change its implementation to use .some() to express boolean
intent (i.e., check userEditOperations.some(op => op.type ===
UserEditingType.SOFIE && op.id === DefaultUserOperationsTypes.UPDATE_PROPS));
keep the same name and signature so callers are unchanged and ensure
imports/references to UserEditingType.SOFIE and
DefaultUserOperationsTypes.UPDATE_PROPS remain available after moving the
function.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4bd0990b-08c0-4bf2-b013-200a8f427bfa
📒 Files selected for processing (1)
packages/webui/src/client/ui/SegmentTimeline/SegmentContextMenu.tsx
Comment on lines
313
to
337
| {enableUserEdits && ( | ||
| <> | ||
| <hr /> | ||
| <MenuItem onClick={() => onEditProps({ type: 'segment', elementId: part.instance.segmentId })}> | ||
| <span>{t('Edit Segment Properties')}</span> | ||
| </MenuItem> | ||
| <MenuItem onClick={() => onEditProps({ type: 'part', elementId: part.instance.part._id })}> | ||
| <span>{t('Edit Part Properties')}</span> | ||
| </MenuItem> | ||
| {piece && piece.instance.piece.userEditProperties && ( | ||
| <MenuItem onClick={() => onEditProps({ type: 'piece', elementId: piece.instance.piece._id })}> | ||
| <span>{t('Edit Piece Properties')}</span> | ||
| </MenuItem> | ||
| )} | ||
| {segment && | ||
| segment.userEditProperties && | ||
| doesItemSupportUserEditUpdateProps(segment.userEditOperations) && ( | ||
| <MenuItem onClick={() => onEditProps({ type: 'segment', elementId: part.instance.segmentId })}> | ||
| <span>{t('Edit Segment Properties')}</span> | ||
| </MenuItem> | ||
| )} | ||
| {part && | ||
| part.instance.part.userEditProperties && | ||
| doesItemSupportUserEditUpdateProps(part.instance.part.userEditOperations) && ( | ||
| <MenuItem onClick={() => onEditProps({ type: 'part', elementId: part.instance.part._id })}> | ||
| <span>{t('Edit Part Properties')}</span> | ||
| </MenuItem> | ||
| )} | ||
| {piece && | ||
| piece.instance.piece.userEditProperties && | ||
| doesItemSupportUserEditUpdateProps(piece.instance.piece.userEditOperations) && ( | ||
| <MenuItem onClick={() => onEditProps({ type: 'piece', elementId: piece.instance.piece._id })}> | ||
| <span>{t('Edit Piece Properties')}</span> | ||
| </MenuItem> | ||
| )} | ||
| </> |
There was a problem hiding this comment.
Potential orphan <hr /> when no edit menu items render.
If enableUserEdits is true but none of segment, part, or piece satisfy their conditions (i.e., none have both userEditProperties and UPDATE_PROPS support), the block will render only the <hr /> on line 315 with no menu items following it, leaving a visual artifact at the end of the context menu.
🛠️ Proposed fix
Move the <hr /> inside a condition that checks if at least one item will render:
{enableUserEdits && (
<>
- <hr />
+ {(segment?.userEditProperties && doesItemSupportUserEditUpdateProps(segment.userEditOperations)) ||
+ (part?.instance.part.userEditProperties && doesItemSupportUserEditUpdateProps(part.instance.part.userEditOperations)) ||
+ (piece?.instance.piece.userEditProperties && doesItemSupportUserEditUpdateProps(piece.instance.piece.userEditOperations)) ? (
+ <hr />
+ ) : null}
{segment &&
segment.userEditProperties &&
doesItemSupportUserEditUpdateProps(segment.userEditOperations) && (Alternatively, compute a boolean flag earlier:
const hasAnyEditPropsItem =
(segment?.userEditProperties && doesItemSupportUserEditUpdateProps(segment.userEditOperations)) ||
(part?.instance.part.userEditProperties && doesItemSupportUserEditUpdateProps(part.instance.part.userEditOperations)) ||
(piece?.instance.piece.userEditProperties && doesItemSupportUserEditUpdateProps(piece.instance.piece.userEditOperations))Then use {enableUserEdits && hasAnyEditPropsItem && <hr />} before the menu items.
📝 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
| {enableUserEdits && ( | |
| <> | |
| <hr /> | |
| <MenuItem onClick={() => onEditProps({ type: 'segment', elementId: part.instance.segmentId })}> | |
| <span>{t('Edit Segment Properties')}</span> | |
| </MenuItem> | |
| <MenuItem onClick={() => onEditProps({ type: 'part', elementId: part.instance.part._id })}> | |
| <span>{t('Edit Part Properties')}</span> | |
| </MenuItem> | |
| {piece && piece.instance.piece.userEditProperties && ( | |
| <MenuItem onClick={() => onEditProps({ type: 'piece', elementId: piece.instance.piece._id })}> | |
| <span>{t('Edit Piece Properties')}</span> | |
| </MenuItem> | |
| )} | |
| {segment && | |
| segment.userEditProperties && | |
| doesItemSupportUserEditUpdateProps(segment.userEditOperations) && ( | |
| <MenuItem onClick={() => onEditProps({ type: 'segment', elementId: part.instance.segmentId })}> | |
| <span>{t('Edit Segment Properties')}</span> | |
| </MenuItem> | |
| )} | |
| {part && | |
| part.instance.part.userEditProperties && | |
| doesItemSupportUserEditUpdateProps(part.instance.part.userEditOperations) && ( | |
| <MenuItem onClick={() => onEditProps({ type: 'part', elementId: part.instance.part._id })}> | |
| <span>{t('Edit Part Properties')}</span> | |
| </MenuItem> | |
| )} | |
| {piece && | |
| piece.instance.piece.userEditProperties && | |
| doesItemSupportUserEditUpdateProps(piece.instance.piece.userEditOperations) && ( | |
| <MenuItem onClick={() => onEditProps({ type: 'piece', elementId: piece.instance.piece._id })}> | |
| <span>{t('Edit Piece Properties')}</span> | |
| </MenuItem> | |
| )} | |
| </> | |
| {enableUserEdits && ( | |
| <> | |
| {(segment?.userEditProperties && doesItemSupportUserEditUpdateProps(segment.userEditOperations)) || | |
| (part?.instance.part.userEditProperties && doesItemSupportUserEditUpdateProps(part.instance.part.userEditOperations)) || | |
| (piece?.instance.piece.userEditProperties && doesItemSupportUserEditUpdateProps(piece.instance.piece.userEditOperations)) ? ( | |
| <hr /> | |
| ) : null} | |
| {segment && | |
| segment.userEditProperties && | |
| doesItemSupportUserEditUpdateProps(segment.userEditOperations) && ( | |
| <MenuItem onClick={() => onEditProps({ type: 'segment', elementId: part.instance.segmentId })}> | |
| <span>{t('Edit Segment Properties')}</span> | |
| </MenuItem> | |
| )} | |
| {part && | |
| part.instance.part.userEditProperties && | |
| doesItemSupportUserEditUpdateProps(part.instance.part.userEditOperations) && ( | |
| <MenuItem onClick={() => onEditProps({ type: 'part', elementId: part.instance.part._id })}> | |
| <span>{t('Edit Part Properties')}</span> | |
| </MenuItem> | |
| )} | |
| {piece && | |
| piece.instance.piece.userEditProperties && | |
| doesItemSupportUserEditUpdateProps(piece.instance.piece.userEditOperations) && ( | |
| <MenuItem onClick={() => onEditProps({ type: 'piece', elementId: piece.instance.piece._id })}> | |
| <span>{t('Edit Piece Properties')}</span> | |
| </MenuItem> | |
| )} | |
| </> | |
| )} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/webui/src/client/ui/SegmentTimeline/SegmentContextMenu.tsx` around
lines 313 - 337, The <hr /> can render alone when enableUserEdits is true but
none of the edit menu items are eligible; update the SegmentContextMenu render
so the divider is only output when at least one edit item will render by either
moving the <hr /> inside the same conditional checks that render the MenuItem(s)
or by computing a boolean like hasAnyEditPropsItem using segment, part, piece
and doesItemSupportUserEditUpdateProps (checking the same properties used for
the MenuItem rendering) and then rendering {enableUserEdits &&
hasAnyEditPropsItem && <hr />} before the MenuItem blocks; keep existing
onEditProps calls and the checks on segment.userEditProperties,
part.instance.part.userEditProperties, piece.instance.piece.userEditProperties
and their corresponding doesItemSupportUserEditUpdateProps calls.
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
1 participant
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:
Bug fix
Current Behavior
The
Edit ... Propertiesmenu items are shown on all Items (Segments, Parts, Pieces) when user editing is enabled, even if those items do not have theDefaultUserOperationsTypes.UPDATE_PROPSoperation declared inuserEditOperations.New Behavior
Only show the items on relevant items.
Testing
Affected areas
Time Frame
Other Information
Status