feat: Director screen improvements#1600
feat: Director screen improvements#1600anteeek wants to merge 3 commits intoSofie-Automation:mainfrom
Conversation
|
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
WalkthroughThe changes introduce a new timing reference calculation feature for the director screen. A utility function computes Changes
Sequence DiagramsequenceDiagram
participant Util as Utility Layer
participant DS as DirectorScreen
participant DST as DirectorScreenTop
participant UI as UI Display
Util->>Util: getSelectedPartInstances()
Util->>Util: Compute partInstanceToCountTimeFrom<br/>based on filtering & sorting
Util->>DS: Return with partInstanceToCountTimeFrom
DS->>DS: getDirectorScreenReactive()
DS->>DS: Extend return with<br/>partInstanceToCountTimeFrom
DS->>DST: Pass playlist &<br/>partInstanceToCountTimeFrom
DST->>DST: useTiming()
DST->>DST: Calculate expected start,<br/>duration, end
DST->>DST: getPlaylistTimingDiff()
DST->>DST: Compute now, over/under clock
DST->>UI: Render Planned End,<br/>Time To/Since,<br/>Over/Under display
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
@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: 3
🤖 Fix all issues with AI agents
In `@packages/webui/src/client/lib/rundownPlaylistUtil.ts`:
- Around line 120-133: Rename the misleading boolean variable areAllPartsTimed
to hasAnyTimedPart to reflect that it checks for the existence of any timed
part; update the declaration and every usage (including the ['part.untimed']
check in the second UIPartInstances.findOne that assigns
partInstanceToCountTimeFrom) so the logic remains identical but the name
accurately conveys intent.
In `@packages/webui/src/client/ui/ClockView/DirectorScreen/DirectorScreen.tsx`:
- Line 491: Replace the hard-coded label "Time to planned start" in the
DirectorScreen component with the i18n translation call used elsewhere; locate
the element with className "director-screen__body__part__timeto-name" inside the
DirectorScreen JSX and change the string to use the t(...) function (consistent
with other labels in this file) so the label is translated at runtime.
In `@packages/webui/src/client/ui/ClockView/DirectorScreen/DirectorScreenTop.tsx`:
- Around line 53-61: The countdown is inverted because TimeToPlannedEndComponent
is passed now - expectedEnd (negative when expectedEnd > now); update the prop
in DirectorScreenTop's JSX so TimeToPlannedEndComponent receives expectedEnd -
now instead, ensuring a positive remaining-time value while keeping the
surrounding conditional (expectedEnd && expectedEnd > now) and labels unchanged.
🧹 Nitpick comments (2)
packages/webui/src/client/ui/ClockView/DirectorScreen/DirectorScreenTop.tsx (1)
65-70: Inconsistent time source usage.Line 69 uses
getCurrentTime()while other calculations in this component usenow(line 37, 53, 56, 68). This inconsistency could cause subtle timing discrepancies whentimingDurations.currentTimediffers fromgetCurrentTime().♻️ Suggested fix for consistency
<TimeSincePlannedEndComponent value={ rehearsalInProgress ? (partInstanceToCountTimeFrom?.timings?.take || 0) + expectedDuration - now - : getCurrentTime() - (expectedStart + expectedDuration) + : now - (expectedStart + expectedDuration) } />packages/webui/src/client/ui/ClockView/ClockView.tsx (1)
8-8: Consider adding.jsextension for consistency.Other imports in this file use
.jsextensions (e.g., lines 2, 4, 6, 7, 9-16), but this import omits it. While TypeScript/bundler may resolve it correctly, maintaining consistency improves clarity.♻️ Optional fix for consistency
-import { DirectorScreen } from './DirectorScreen/DirectorScreen' +import { DirectorScreen } from './DirectorScreen/DirectorScreen.js'
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/webui/src/client/lib/rundownPlaylistUtil.tspackages/webui/src/client/styles/countdown/director.scsspackages/webui/src/client/styles/counterComponents.scsspackages/webui/src/client/ui/ClockView/ClockView.tsxpackages/webui/src/client/ui/ClockView/DirectorScreen/DirectorScreen.tsxpackages/webui/src/client/ui/ClockView/DirectorScreen/DirectorScreenTop.tsx
💤 Files with no reviewable changes (1)
- packages/webui/src/client/styles/counterComponents.scss
🧰 Additional context used
🧬 Code graph analysis (1)
packages/webui/src/client/lib/rundownPlaylistUtil.ts (2)
packages/webui/src/client/ui/Collections.tsx (1)
UIPartInstances(53-53)packages/job-worker/src/playout/model/implementation/PlayoutModelImpl.ts (1)
playlist(73-75)
🔇 Additional comments (7)
packages/webui/src/client/lib/rundownPlaylistUtil.ts (1)
92-100: LGTM on the API extension.The return type extension to include
partInstanceToCountTimeFromis well-structured and aligns with the PR objectives for rehearsal timing calculations.packages/webui/src/client/ui/ClockView/DirectorScreen/DirectorScreenTop.tsx (1)
20-41: LGTM on component structure.The component is well-structured with clear separation of timing calculations and conditional rendering logic. The use of
useTiminghook anduseTranslationfollows React best practices.packages/webui/src/client/styles/countdown/director.scss (2)
205-267: LGTM on the new timeto-content styling.The styling for the countdown display is comprehensive with proper font-variation-settings for Roboto Flex. The
.overtimemodifier correctly uses$general-late-colorfor visual distinction.
42-48: LGTM on text alignment changes.The centered alignment for
.director-screen__top__time-toand.director-screen__top__planned-toaligns with the UI improvements for the countdown display.packages/webui/src/client/ui/ClockView/DirectorScreen/DirectorScreen.tsx (3)
353-357: LGTM on the subscription addition.Adding the subscription for
firstTakenPartInstanceensures reactive updates for the new timing display feature.
387-395: LGTM on the timing display integration.The logic for determining
isFirstPieceAndNoDurationand the integration ofDirectorScreenTopcorrectly implements the PR objective to show countdown to planned start when initial parts are untimed.
382-382: Remove or capture return value fromuseTiming()call.The
useTiming()hook returnsRundownTimingContext(timing data) that is discarded at line 382. If timing updates are needed, capture and use the return value. If not, this call should be removed as it adds unnecessary event listener overhead.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| <div className="director-screen__body__part__timeto-countdown"> | ||
| <Timediff time={expectedStart - getCurrentTime()} /> | ||
| </div> | ||
| <div className="director-screen__body__part__timeto-name">Time to planned start</div> |
There was a problem hiding this comment.
Missing translation for user-facing string.
The string "Time to planned start" should use the t() translation function for i18n consistency, matching how other labels in this component are translated (e.g., lines 528, 537).
🌐 Suggested fix
-<div className="director-screen__body__part__timeto-name">Time to planned start</div>
+<div className="director-screen__body__part__timeto-name">{t('Time to planned start')}</div>📝 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.
| <div className="director-screen__body__part__timeto-name">Time to planned start</div> | |
| <div className="director-screen__body__part__timeto-name">{t('Time to planned start')}</div> |
🤖 Prompt for AI Agents
In `@packages/webui/src/client/ui/ClockView/DirectorScreen/DirectorScreen.tsx` at
line 491, Replace the hard-coded label "Time to planned start" in the
DirectorScreen component with the i18n translation call used elsewhere; locate
the element with className "director-screen__body__part__timeto-name" inside the
DirectorScreen JSX and change the string to use the t(...) function (consistent
with other labels in this file) so the label is translated at runtime.
packages/webui/src/client/ui/ClockView/DirectorScreen/DirectorScreenTop.tsx
Show resolved
Hide resolved
jstarpl
added
Contribution
jstarpl
changed the title
anteeek
force-pushed
the
feat/SOFIE-252
branch
from
575bb32 to
22f7aa6
Compare
anteeek
force-pushed
the
feat/SOFIE-252
branch
from
22f7aa6 to
9a11560
Compare
3 participants
About the Contributor
This pull request is posted on behalf of the BBC.
Type of Contribution
This is a: Feature
Current Behavior
Countdown to start
The counter in the Director Screen counts to the planned start, and stops counting when the first part is taken, regardless if it's untimed or not.
Rehearsal timing
The counter counts to the planned end which is unhelpful if you're rehearsing an hour before the planned start (or practicing with an old running order)
New Behavior
Countdown to start
When the countdown to planned start is shown in the On-Air part area (before the first take command has occurred) a label has been added to show "Time to planned start"
If the first part(s) are untimed then it retains the countdown to start rather than showing the on-air part.
As the first part(s) are untimed the start time is still very relevant for knowing when to take the first timed part.
Rehearsal timing
After the first timed part is taken, the Time to Planned End countdown in the top portion of the Director Screen shows a Time to Rehearsal End countdown.
It counts towards the end of the program duration (I.E. End = Time of first timed take + program duration)
Testing
Affected areas
Director Screen
Time Frame
Not urgent, but we would like to get this merged into the in-development release.
Other Information
Status