chore: replace a few class components with hooks#1646
Merged
Julusian merged 4 commits intoSofie-Automation:mainfrom Feb 17, 2026
Merged
chore: replace a few class components with hooks#1646Julusian merged 4 commits intoSofie-Automation:mainfrom
Julusian merged 4 commits intoSofie-Automation:mainfrom
Conversation
Julusian
added
the
Contribution from SuperFly.tv
WalkthroughRefactors many class-based, HOC-wrapped React components into functional components using hooks (useTranslation, useTracker, useTiming), adds a new useTiming hook, replaces several HOC wrappers, introduces small helper components, and updates public component signatures across the web UI (several files modified). Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
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. ✨ 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✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/webui/src/client/ui/RundownView/RundownTiming/withTiming.tsx (1)
204-215:⚠️ Potential issue | 🟠 MajorStale closure over
contextinrefreshComponent.
refreshComponentis memoized with[]deps, so it captures thecontextvalue from the initial render. On line 208,context.durationswill refer to the context object from mount time. If the provider supplies a new context object on updates (the typical React pattern), the filter comparison will use stale data—potentially skipping necessary re-renders or forcing unnecessary ones.The
filterGetterref pattern on lines 201–202 already solves this for the filter function. Apply the same approach tocontext:Proposed fix: store context in a ref
const context = useContext(RundownTimingProviderContext) + const contextRef = useRef(context) + contextRef.current = context ... const refreshComponent = useCallback(() => { if (!filterGetter.current) { setForceUpdate(Date.now()) } else { - const buf = filterGetter.current?.(context.durations || {}) + const buf = filterGetter.current?.(contextRef.current.durations || {}) if (isDirty.current || !_.isEqual(buf, previousValue.current)) { previousValue.current = buf isDirty.current = false setForceUpdate(Date.now()) } } }, [])
🤖 Fix all issues with AI agents
In `@packages/webui/src/client/ui/Header.tsx`:
- Around line 32-44: The updater passed to setIsNotificationCenterOpen currently
mutates NotificationCenter.isOpen (side effect) inside the state updater in
onToggleNotifications; move this side effect out so the updater remains pure:
derive the new filter value inside onToggleNotifications (compute newFilter from
current isNotificationCenterOpen and incoming filter), call
setIsNotificationCenterOpen(newFilter) and then set NotificationCenter.isOpen =
newFilter !== undefined, or preferably add a useEffect that watches
isNotificationCenterOpen and sets NotificationCenter.isOpen =
isNotificationCenterOpen (also simplify the boolean expression to use !==
undefined); update React import to include useEffect if using the preferred
useEffect approach.
In `@packages/webui/src/client/ui/SegmentTimeline/SegmentContextMenu.tsx`:
- Around line 126-134: The first <hr /> outside the enableUserEdits block
creates an extra separator (double hr when enableUserEdits is true and a
trailing hr when false); remove the standalone <hr /> before the conditional so
only the <hr /> inside the enableUserEdits branch remains. Locate the
SegmentContextMenu component, remove the outer hr element that precedes the
conditional rendering of enableUserEdits (the block containing MenuItem and
onEditProps with part.instance.segmentId) so separators render only when the
edit menu is shown.
In `@packages/webui/src/client/ui/Settings/BlueprintSettings.tsx`:
- Around line 90-97: The two near-identical labels use inconsistent casing and
may confuse users; update the label shown for blueprint.blueprintId to match
casing and clarity—either change the LabelActual call currently rendering
"Blueprint Id" to use "Blueprint ID" to match the other label, or better,
disambiguate both by renaming the second label to something like "Internal
Blueprint ID" (or "Blueprint Code ID") where LabelActual is used around
blueprint.blueprintId so the UI reads clearly and consistently (refer to the
LabelActual component and the blueprint.blueprintId usage).
- Line 220: The empty-state text in BlueprintSettings (use of t('This Blueprint
is not compatible with any Studio')) is misleading because the query filters by
blueprintId (assignment) not compatibility; update the translation string used
in BlueprintSettings to say it’s not assigned/being used by any Studio (e.g.,
match the ShowStyles wording "not being used by any Studio" or "not assigned to
any Studio") so the UI reflects assignment rather than compatibility; locate the
text in the BlueprintSettings component where t('This Blueprint is not
compatible with any Studio') is called and replace it with the corrected
phrasing and corresponding i18n key if needed.
- Around line 23-25: Imports DBShowStyleBase, DBStudio and assertNever are using
the source paths from `@sofie-automation/corelib` (src/...) which is inconsistent
with the rest of webui and can break build; update the three imports
(DBShowStyleBase, DBStudio, assertNever) to import from the corresponding
`@sofie-automation/corelib/dist/`... paths so they match other modules and rely on
the compiled package output.
In `@packages/webui/src/client/ui/Settings/ShowStyle/HotkeyLegend.tsx`:
- Line 75: Replace the unstable React key using the user-visible shortcut string
by using the unique identifier instead: in the HotkeyLegend component change the
React.Fragment key from item.key to item._id (or another unique id field on the
hotkey object) so newly added items with empty shortcut strings do not collide;
update any other list rendering in this component that uses item.key as the key
to use item._id consistently.
In `@packages/webui/src/client/ui/Settings/SnapshotsView.tsx`:
- Line 107: Typo in the translation string inside the SnapshotsView component:
replace "throught" with "through" in the call to t('Reads the ingest (NRCS)
data, and pipes it throught the blueprints') so the displayed text reads "Reads
the ingest (NRCS) data, and pipes it through the blueprints"; update the string
literal in SnapshotsView (the JSX/translation invocation) accordingly.
- Line 337: Replace the manual Bootstrap classes on the Button that triggers
takeSystemSnapshot with the react-bootstrap variant prop: remove className="btn
btn-primary" and use variant="primary" (e.g., <Button variant="primary"
onClick={takeSystemSnapshot}>), matching the other Button usages that use
variant="outline-secondary".
- Around line 263-297: The modal dialog text in restoreStoredSnapshot (and
similarly in TakeSystemSnapshotButton and RemoveSnapshotButton) uses hardcoded
English strings passed to doModalDialog; replace those literal strings with
translated strings via the t() function (e.g., t('Restore Snapshot'), t('Do you
really want to restore the snapshot {{name}}?', { name: snapshot.name }),
t('Snapshot restored!'), t('Error: {{error}}', { error: err.toString() })) so
all titles, messages and acceptOnly labels are i18n-aware; locate usages in
restoreStoredSnapshot, TakeSystemSnapshotButton and RemoveSnapshotButton and
wrap each static string with t() or use parameterized keys for dynamic values.
🧹 Nitpick comments (14)
packages/webui/src/client/ui/Settings/DeviceSettings.tsx (2)
141-179:e.persist()is a no-op in React 18.Since this project uses React 18.3.1, SyntheticEvent pooling was removed in React 17. The
e.persist()calls on lines 143 and 193 are harmless but unnecessary dead code.
49-121: Minor: redundant null checks after the early-return guard.Line 56 uses
device?.name(optional chaining) and line 115 usesdevice &&, butdeviceis guaranteed to be defined after the guard on line 43. These are harmless but slightly misleading.Proposed cleanup
- {!device?.name ? ( + {!device.name ? (- {device && - device.type === PeripheralDeviceType.PACKAGE_MANAGER && + {device.type === PeripheralDeviceType.PACKAGE_MANAGER &&packages/webui/src/client/ui/Settings/ShowStyle/HotkeyLegend.tsx (3)
41-51:useCallbackdependency on entireshowStyleBaseobject defeats memoization.Since
showStyleBaseis a new object reference on every reactive update, this callback is recreated every render. Consider destructuring the specific fields needed (_id,name,hotkeyLegend) or accepting the re-creation as intentional.♻️ Suggested narrower dependencies
- const exportHotkeyJSON = useCallback(() => { - const jsonStr = JSON.stringify(showStyleBase.hotkeyLegend, undefined, 4) + const { _id: showStyleBaseId, name: showStyleBaseName, hotkeyLegend } = showStyleBase + + const exportHotkeyJSON = useCallback(() => { + const jsonStr = JSON.stringify(hotkeyLegend, undefined, 4) const element = document.createElement('a') element.href = URL.createObjectURL(new Blob([jsonStr], { type: 'application/json' })) - element.download = `${showStyleBase._id}_${showStyleBase.name.replace(/\W/g, '_')}_hotkeys.json` + element.download = `${showStyleBaseId}_${showStyleBaseName.replace(/\W/g, '_')}_hotkeys.json` document.body.appendChild(element) element.click() document.body.removeChild(element) - }, [showStyleBase]) + }, [hotkeyLegend, showStyleBaseId, showStyleBaseName])
44-50: Blob URL is never revoked — minor memory leak.
URL.createObjectURLallocates a blob URL that persists until the page is unloaded or explicitly revoked. After triggering the download, callURL.revokeObjectURLto free it.♻️ Proposed fix
+ const url = element.href document.body.appendChild(element) element.click() document.body.removeChild(element) + URL.revokeObjectURL(url)
90-90: Unnecessary optional chaining ononDeleteHotkeyLegend.
onDeleteHotkeyLegendis always defined in this component scope — the?.is redundant.♻️ Suggested fix
- <button className="action-btn" onClick={() => onDeleteHotkeyLegend?.(item)}> + <button className="action-btn" onClick={() => onDeleteHotkeyLegend(item)}>packages/webui/src/client/ui/Shelf/PieceCountdownPanel.tsx (1)
37-49:panelis used inside the tracker but missing from the dependency array.The autorun callback references
panel.sourceLayerIds(lines 40, 43) and indirectlypanelto filter pieces, butpanelis not listed in thedepsarray on line 49. Ifpanelever changes identity or itssourceLayerIdsare updated, the tracker will continue using the stale closure value.Proposed fix
- }, [playlist, showStyleBase]) + }, [playlist, showStyleBase, panel])packages/webui/src/client/ui/RundownView/RundownTiming/withTiming.tsx (1)
186-230:Date.now()for force-update can theoretically coalesce.Two rapid
setForceUpdate(Date.now())calls within the same millisecond produce the same value, so React may elide the second update. The standard idiom is a functional updater with a counter:Proposed tweak
- const [_0, setForceUpdate] = useState(0) + const [, setTick] = useState(0) ... - setForceUpdate(Date.now()) + setTick((n) => n + 1) ... - setForceUpdate(Date.now()) + setTick((n) => n + 1)packages/webui/src/client/ui/SegmentTimeline/SmallParts/SegmentTimelineSmallPartFlag.tsx (1)
16-43:timelineWidthprop is declared but never read.
timelineWidthon line 35 is part of the interface but isn't referenced anywhere in the component body. If it's no longer needed after the conversion, remove it to keep the interface accurate.Proposed fix
isLastInSegment: boolean - timelineWidth: number showDurationSourceLayers?: Set<ISourceLayer['_id']>packages/webui/src/client/ui/SegmentTimeline/SegmentContextMenu.tsx (3)
64-67: Remove commented-out dead code.This commented-out
onSetAsNextFromHerehandler (and its counterpart at lines 149–155) are leftovers from the class component. They add noise and should be removed in this cleanup PR.🧹 Proposed cleanup
- // private onSetAsNextFromHere = (part: DBPart, e) => { - // const offset = this.getTimePosition() - // onSetNext(part, e, offset || 0) - // } - const onPlayFromHere = (part: DBPart, e: React.MouseEvent | React.TouchEvent) => {And at lines 149–155:
{startsAt !== undefined && part && enablePlayFromAnywhere ? ( <> - {/* <MenuItem - onClick={(e) => this.onSetAsNextFromHere(part.instance.part, e)} - disabled={isCurrentPart || !!part.instance.orphaned || !canSetAsNext} - > - <span dangerouslySetInnerHTML={{ __html: t('Set <strong>Next</strong> Here') }}></span> ( - {RundownUtils.formatTimeToShortTime(Math.floor((startsAt + timecode) / 1000) * 1000)}) - </MenuItem> */} <MenuItem
102-102: Pre-existingdangerouslySetInnerHTML— consider<Trans>component.Static analysis flags these as XSS risks (CWE-79). The content comes from
t()(i18n-controlled strings), so the practical risk is low. Since you're already touching this component, consider using react-i18next's<Trans>component to render the<strong>tags safely withoutdangerouslySetInnerHTML:<Trans i18nKey="Set segment as <strong>Next</strong>"> Set segment as <strong>Next</strong> </Trans>This is pre-existing and not introduced by this PR, so fine to defer.
Also applies to: 143-143
74-91: Derived state looks correct;playlistredundancy is a nit.Context extraction and derived state are well-structured. Minor nit:
playlistat line 81 is guaranteed to be defined after the guard at line 50, making the&& playlistcheck redundant. The|| undefinedpattern at line 81 producestrue | undefinedrather than a cleanboolean, but this is cosmetic sincedisabledaccepts both.packages/webui/src/client/ui/Header.tsx (2)
16-22:allowDeveloperis declared in the interface but never used.
IPropsHeaderdefinesallowDeveloper?: boolean(Line 19), but the component destructures only{ allowConfigure, allowTesting }(Line 22) and never referencesallowDeveloper. If it's no longer needed after the conversion, consider removing it from the interface to avoid confusion.Proposed cleanup
interface IPropsHeader { allowConfigure?: boolean allowTesting?: boolean - allowDeveloper?: boolean }
93-110: Inconsistent leading whitespace in Nav.Link text.Line 95 has
{t('Rundowns')}with no leading space, but Lines 99, 103, and 107 have a leading space before the{t(...)}call (e.g.,<Nav.Link> {t('Test Tools')}</Nav.Link>). This renders as extra whitespace inside those links.Remove leading spaces for consistency
<LinkContainer to="/testTools" activeClassName="active"> - <Nav.Link> {t('Test Tools')}</Nav.Link> + <Nav.Link>{t('Test Tools')}</Nav.Link> </LinkContainer> ... <LinkContainer to="/status" activeClassName="active"> - <Nav.Link> {t('Status')}</Nav.Link> + <Nav.Link>{t('Status')}</Nav.Link> </LinkContainer> ... <LinkContainer to="/settings" activeClassName="active"> - <Nav.Link> {t('Settings')}</Nav.Link> + <Nav.Link>{t('Settings')}</Nav.Link> </LinkContainer>packages/webui/src/client/ui/Settings/BlueprintSettings.tsx (1)
31-31: Component exported asBlueprintSettings2— leftover transitional name?The
2suffix looks like a temporary name used during migration to avoid conflicts with the old class component. If the old component has been fully removed, consider renaming this toBlueprintSettingsbefore merging to keep the public API clean.
packages/webui/src/client/ui/SegmentTimeline/SegmentContextMenu.tsx
Outdated
Show resolved
Hide resolved
packages/webui/src/client/ui/Settings/ShowStyle/HotkeyLegend.tsx
Outdated
Show resolved
Hide resolved
Julusian
force-pushed
the
feat/convert-some-react-to-hooks
branch
from
4faaad7 to
d6378cf
Compare
jstarpl
approved these changes
Feb 13, 2026
# Conflicts: # packages/webui/src/client/ui/SegmentTimeline/SegmentContextMenu.tsx
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 (2)
packages/webui/src/client/ui/RundownView/RundownTiming/withTiming.tsx (1)
204-215:⚠️ Potential issue | 🟠 Major
refreshComponentrecreated on every context change causes excessive event re-subscriptions.
contextfromuseContextwill be a new object reference whenever the provider value updates (i.e., on every timing tick). SincerefreshComponentdepends on[context], it's recreated each time, which causes theuseEffecton line 217-223 to tear down and re-add the window event listener on every timing update — defeating the purpose of the subscription model.Follow the same ref pattern already used for
filterGetter: store the context (or justcontext.durations) in a ref that's assigned during render, and removecontextfrom the dependency array.Proposed fix
+ const contextRef = useRef(context) + contextRef.current = context const refreshComponent = useCallback(() => { if (!filterGetter.current) { setForceUpdate(Date.now()) } else { - const buf = filterGetter.current?.(context.durations || {}) + const buf = filterGetter.current?.(contextRef.current.durations || {}) if (isDirty.current || !_.isEqual(buf, previousValue.current)) { previousValue.current = buf isDirty.current = false setForceUpdate(Date.now()) } } - }, [context]) + }, [])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/webui/src/client/ui/RundownView/RundownTiming/withTiming.tsx` around lines 204 - 215, refreshComponent is being recreated on every timing tick because it depends on the entire context object; move the value needed (context.durations) into a ref (e.g., durationsRef.current = context.durations) during render (same pattern as filterGetter) and then remove context from refreshComponent's dependency array so refreshComponent uses durationsRef.current instead of context; ensure you still reference isDirty.current, previousValue.current and call setForceUpdate(Date.now()) as before and update previousValue.current from the durationsRef value when changes are detected.packages/webui/src/client/ui/Header.tsx (1)
16-22:⚠️ Potential issue | 🟡 Minor
allowDeveloperis declared in the interface but never used in the component.The prop is defined on line 19 but not destructured on line 22 and not referenced anywhere in the Header component. Although
allowDeveloperis passed from App.tsx (line 173), it's ignored by the Header component. Remove it from the interface and also remove the prop from the App.tsx call site to avoid unused parameters.Changes needed
Header.tsx:
interface IPropsHeader { allowConfigure?: boolean allowTesting?: boolean - allowDeveloper?: boolean }App.tsx:
<Header {...props} allowConfigure={roles.configure} allowTesting={roles.testing} - allowDeveloper={roles.developer} />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/webui/src/client/ui/Header.tsx` around lines 16 - 22, The interface IPropsHeader declares allowDeveloper but the Header component doesn't use it; remove allowDeveloper from IPropsHeader and from the Header component signature (if present) and then remove the allowDeveloper prop being passed from the App.tsx Header(...) call site so there are no unused props; ensure no other code references allowDeveloper on the Header component and run type checks to confirm the prop removal is clean.
🧹 Nitpick comments (8)
packages/documentation/versioned_docs/version-1.50.0/for-developers/device-integrations/intro.md (1)
12-12: Optional: Consider more concise phrasing.The phrase "all of the devices" could be shortened to "all the devices" for slightly better conciseness, though both are correct.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/documentation/versioned_docs/version-1.50.0/for-developers/device-integrations/intro.md` at line 12, The sentence is slightly wordy—replace "all of the devices" with "all the devices" and simplify surrounding phrasing for conciseness; specifically update the paragraph referencing the Conductor class, TSR, timeline, timeline layers, and mappings so it reads more tightly (e.g., "The timeline describes the state of all the devices over time by placing objects on timeline layers, each mapped to a device part via mappings.")packages/webui/src/client/ui/Settings/DeviceSettings.tsx (1)
143-143:e.persist()is a no-op in React 18.React 18 removed SyntheticEvent pooling, so
e.persist()does nothing. Same applies to Line 193 inTroubleshootDeviceButton. Feel free to remove both when convenient.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/webui/src/client/ui/Settings/DeviceSettings.tsx` at line 143, Remove the obsolete e.persist() calls because React 18 no longer pools SyntheticEvents; locate and delete the e.persist() invocation in the DeviceSettings component (the event handler containing e.persist()) and the similar e.persist() in the TroubleshootDeviceButton component (around the handler at the other reported location), leaving the handlers to use the event object directly or copy needed values into locals if they must be used asynchronously.packages/webui/src/client/ui/Shelf/PieceCountdownPanel.tsx (1)
37-49: Dependency array contains object/array references that may cause extra tracker reruns.
playlist,showStyleBase, andpanel.sourceLayerIdsare objects/arrays, so any parent re-render that passes new references (even with identical data) will re-trigger the tracker. This is likely acceptable given that the tracker computation is lightweight and the parent likely doesn't re-render frequently without data changes, but worth keeping in mind if performance concerns arise.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/webui/src/client/ui/Shelf/PieceCountdownPanel.tsx` around lines 37 - 49, The useTracker dependency array uses object/array refs (playlist, showStyleBase, panel.sourceLayerIds) which can cause unnecessary reruns; fix by passing stable, primitive-based dependencies or memoized values instead: derive and pass playlist.currentPartInfo?.partInstanceId and any playlist id/version, a memoized showStyleBase key (or memoize showStyleBase with useMemo/useCallback), and memoize panel.sourceLayerIds (e.g., useMemo or a stable stringified key) so useTracker only re-runs when the actual relevant data changes; update the dependency array and/or wrap panel.sourceLayerIds in a memo to reference the same stable variable inside the useTracker closure (referencing useTracker, getUnfinishedPieceInstancesReactive, livePieceInstance, and panel.sourceLayerIds).packages/webui/src/client/ui/RundownView/RundownTiming/withTiming.tsx (1)
199-199: Minor type mismatch inpreviousValueref.
previousValueis typed asuseRef<RundownTimingContext | null>but it stores the return value offilterGetter.current(...), which isany(perTimingFilterFunction). The deep-equality check still works at runtime, but the type is misleading.Suggested fix
- const previousValue = useRef<RundownTimingContext | null>(null) + const previousValue = useRef<any>(null)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/webui/src/client/ui/RundownView/RundownTiming/withTiming.tsx` at line 199, previousValue is typed as useRef<RundownTimingContext | null> but actually stores the result of filterGetter.current(...), which is typed by TimingFilterFunction (any); update the ref type to match the filter return type—either change the declaration of previousValue to useRef<ReturnType<TimingFilterFunction> | null> (preferred if TimingFilterFunction is a function type) or useRef<any | null> / useRef<unknown | null> if ReturnType is not appropriate—so that previousValue, filterGetter.current(...), and the deep-equality checks are correctly typed; adjust the import/typing references to TimingFilterFunction and RundownTimingContext as needed.packages/webui/src/client/ui/Header.tsx (1)
93-109: Inconsistent leading whitespace in Nav.Link children.Line 95 has
{t('Rundowns')}with no leading space, but Lines 99, 103, and 107 have a space before the{t(...)}call (e.g.,⎵{t('Test Tools')}). This is likely harmless since CSS controls layout, but the inconsistency may produce slightly different text rendering.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/webui/src/client/ui/Header.tsx` around lines 93 - 109, The Nav.Link children have inconsistent leading whitespace (some contain a prefixed space before the {t(...)} call) which can affect rendering; update the LinkContainer/Nav.Link blocks (see Nav.Link elements inside LinkContainer for routes "/rundowns", "/testTools", "/status", "/settings") to remove any leading spaces so every Nav.Link uses the same pattern: <Nav.Link>{t('...')}</Nav.Link>; ensure components referenced by allowTesting and allowConfigure follow the same convention and remove the extra spaces around the {t(...)} expressions.packages/webui/src/client/ui/SegmentTimeline/SegmentContextMenu.tsx (3)
103-103: Consider using<Trans>component instead ofdangerouslySetInnerHTMLfor i18n strings with markup.Lines 103 and 144 use
dangerouslySetInnerHTMLto render translated strings containing<strong>tags. While the content comes from translation keys (not user input), theTranscomponent fromreact-i18nextis the idiomatic and safer approach for embedding markup in translations.This is likely carried over from the old class component, so no urgency, but worth considering in a follow-up.
Example using Trans
-<span dangerouslySetInnerHTML={{ __html: t('Set segment as <strong>Next</strong>') }}></span> +<Trans i18nKey="Set segment as <strong>Next</strong>">Set segment as <strong>Next</strong></Trans>Also applies to: 144-144
🤖 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` at line 103, In SegmentContextMenu replace the two uses of dangerouslySetInnerHTML (e.g., span dangerouslySetInnerHTML={{ __html: t('Set segment as <strong>Next</strong>') }}) with the react-i18next <Trans> component: import { Trans } from 'react-i18next', change the span to <Trans i18nKey="Set segment as <strong>Next</strong>"/> (and the similar "Previous" key at the other occurrence), ensuring any surrounding className or props are preserved; remove the dangerouslySetInnerHTML usage so translations with <strong> markup are rendered via <Trans>.
53-68: Remove commented-out dead code.The commented-out
onSetAsNextFromHeremethod (lines 65–68) is a leftover from the class component. Since it's not used anywhere and the functional equivalentonPlayFromHerereplaces it, this block should be removed to keep the file clean.Proposed cleanup
} - // private onSetAsNextFromHere = (part: DBPart, e) => { - // const offset = this.getTimePosition() - // onSetNext(part, e, offset || 0) - // } - const onPlayFromHere = (part: DBPart, e: React.MouseEvent | React.TouchEvent) => {🤖 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 53 - 68, Remove the dead commented-out method onSetAsNextFromHere and its related commented lines (the block referencing onSetAsNextFromHere, getTimePosition call and onSetNext) from SegmentContextMenu.tsx; keep the functional getTimePosition helper and any active handlers like onPlayFromHere, but delete lines corresponding to the commented legacy method to clean up the file.
81-92:isCurrentPartevaluates toboolean | undefinedinstead ofboolean.Line 82 uses
|| undefinedwhich meansisCurrentPartistrue | undefinedrather than a cleanboolean. While this works correctly in thedisabledprop at line 101 (sinceundefinedis falsy), it's a minor readability issue. Consider using?? falseor wrapping in!!for clarity.Proposed fix
- const isCurrentPart = - (part && playlist && part.instance._id === playlist.currentPartInfo?.partInstanceId) || undefined + const isCurrentPart = + !!part && !!playlist && part.instance._id === playlist.currentPartInfo?.partInstanceId🤖 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 81 - 92, isCurrentPart is being set to true | undefined due to the trailing "|| undefined"; change its initialization so it is a plain boolean (e.g. replace "|| undefined" with "?? false" or wrap the expression with "!!") so isCurrentPart always yields a boolean. Update the assignment for isCurrentPart (the variable initialized from part, playlist and playlist.currentPartInfo?.partInstanceId) to use a boolean coercion method so downstream checks (like disabled props) are clearer and type-consistent.
🤖 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/documentation/versioned_docs/version-1.51.0/for-developers/device-integrations/intro.md`:
- Line 12: Change the typo in the introductory paragraph: replace the phrase
"start of" with "start off" in the sentence that begins "But to start of we will
explain the general structure of the TSR..." so the sentence reads "But to start
off we will explain the general structure of the TSR." Reference the same
paragraph mentioning the Conductor class and TSR to locate the text.
In `@packages/webui/src/client/ui/SegmentTimeline/SegmentContextMenu.tsx`:
- Around line 114-125: The component prop targetName is declared in
UserEditOperationMenuItemsProps but never extracted or used in the
UserEditOperationMenuItems function; update the function signature in
RenderUserEditOperations.tsx to destructure targetName from its props (in
addition to existing props like userEditOperations and isFormEditable) and
incorporate it where appropriate (e.g., use in menu item labels, aria labels or
to build operation payloads) so the passed value from callers (e.g.,
SegmentContextMenu) is honored; alternatively, if the value truly isn’t needed,
remove targetName from UserEditOperationMenuItemsProps and from all call sites
to keep the interface and usages consistent.
In `@packages/webui/src/client/ui/Settings/ShowStyle/HotkeyLegend.tsx`:
- Around line 41-51: The exportHotkeyJSON function should guard against
showStyleBase.hotkeyLegend being undefined by defaulting to an empty
array/object before calling JSON.stringify (e.g. use showStyleBase.hotkeyLegend
|| []), so the produced file contains valid JSON instead of the literal
"undefined"; also capture the object URL returned from URL.createObjectURL into
a variable and call URL.revokeObjectURL(url) after triggering the download (for
example via a short setTimeout or an onload/onfocus handler) to avoid leaking
Blob URLs, while still appending/clicking/removing the anchor element as
currently done.
In `@packages/webui/src/client/ui/Settings/SnapshotsView.tsx`:
- Line 297: The callbacks restoreStoredSnapshot, takeSystemSnapshot, and
removeStoredSnapshot use the translation function t but don't include it in
their useCallback dependency arrays; update each useCallback (the declarations
for restoreStoredSnapshot, takeSystemSnapshot, and removeStoredSnapshot) to
include t (alongside existing dependencies like snapshotId) so the callbacks are
recreated when the active translation changes, preventing stale translations
after a runtime language switch.
- Around line 115-121: Table header labels in SnapshotsView are hardcoded; wrap
the "Type", "Name", and "Comment" strings with the i18n function t() so they are
translatable. Update the JSX inside the <tr> (in the SnapshotsView component) to
call t('Type'), t('Name'), and t('Comment') (or the appropriate translation keys
used elsewhere) for those <th> elements, keeping the conditional rendering of
removeSnapshots unchanged.
- Around line 309-332: The error modal in takeSystemSnapshot incorrectly refers
to restoring a snapshot; update the doModalDialog call (inside the
takeSystemSnapshot callback that calls MeteorCall.snapshot.storeSystemSnapshot)
to use a title and message that reflect taking/storing a snapshot (e.g., use
t('Take Snapshot') and t('Snapshot store failed: {{errorMessage}}', ...)) and
ensure the "Requested by user" literal passed to storeSystemSnapshot is
localized with the t(...) translator instead of the raw string so all
user-facing text is translated.
---
Outside diff comments:
In `@packages/webui/src/client/ui/Header.tsx`:
- Around line 16-22: The interface IPropsHeader declares allowDeveloper but the
Header component doesn't use it; remove allowDeveloper from IPropsHeader and
from the Header component signature (if present) and then remove the
allowDeveloper prop being passed from the App.tsx Header(...) call site so there
are no unused props; ensure no other code references allowDeveloper on the
Header component and run type checks to confirm the prop removal is clean.
In `@packages/webui/src/client/ui/RundownView/RundownTiming/withTiming.tsx`:
- Around line 204-215: refreshComponent is being recreated on every timing tick
because it depends on the entire context object; move the value needed
(context.durations) into a ref (e.g., durationsRef.current = context.durations)
during render (same pattern as filterGetter) and then remove context from
refreshComponent's dependency array so refreshComponent uses
durationsRef.current instead of context; ensure you still reference
isDirty.current, previousValue.current and call setForceUpdate(Date.now()) as
before and update previousValue.current from the durationsRef value when changes
are detected.
---
Duplicate comments:
In `@packages/webui/src/client/ui/Header.tsx`:
- Around line 32-44: The onToggleNotifications state updater currently performs
a side effect by assigning NotificationCenter.isOpen inside the
setIsNotificationCenterOpen callback and uses a redundant ternary; to fix,
compute the new filter value outside the state updater (e.g., derive newFilter
by comparing prev value to incoming filter), call
setIsNotificationCenterOpen(prev => newFilter) and then set
NotificationCenter.isOpen = newFilter !== undefined (no ? true : false) outside
the updater; update references to the onToggleNotifications callback accordingly
so the side effect is no longer inside the state setter.
In `@packages/webui/src/client/ui/SegmentTimeline/SegmentContextMenu.tsx`:
- Around line 126-135: In SegmentContextMenu, remove the standalone trailing <hr
/> rendered after the enableUserEdits block (the lone separator after the
MenuItem that calls onEditProps) so the context menu doesn't end with an empty
separator when timecode === null; rely on the leading <hr /> inside
UserEditOperationMenuItems and the separator already inside the enableUserEdits
branch instead of the extra <hr /> to avoid duplicate/adjacent separators.
In `@packages/webui/src/client/ui/Settings/BlueprintSettings.tsx`:
- Line 220: The empty-state message in BlueprintSettings uses the misleading
text "This Blueprint is not compatible with any Studio" while the query filters
by blueprintId (assignment); update the JSX/i18n string inside BlueprintSettings
where t('This Blueprint is not compatible with any Studio') is used to match
ShowStyle wording — e.g. change to t('This Blueprint is not being used by any
Studio') or the appropriate i18n key so the message reflects "not being used by
any Studio" when the blueprintId filter returns no results.
- Around line 90-97: The two labels are now identical and confusing; change the
label for blueprint.blueprintId in BlueprintSettings (the block rendering
LabelActual and displaying blueprint.blueprintId) to a distinct string such as
"Internal Blueprint ID" (or similar) so users can distinguish it from the other
"Blueprint ID" label that shows the document _id; update the LabelActual call in
the conditional rendering that references blueprint.blueprintId accordingly.
In `@packages/webui/src/client/ui/Settings/SnapshotsView.tsx`:
- Around line 260-304: The reviewer comment is a duplicate noting i18n is fixed
for RestoreStoredSnapshotButton; remove the duplicated review comment or mark it
as resolved so it doesn't block the PR and add a one-line confirmation in the PR
thread that RestoreStoredSnapshotButton’s modal strings (the t(...) calls inside
restoreStoredSnapshot) are correct and no further action is needed.
- Around line 336-342: The review notes that the previous variant issue was
fixed but a dangling duplicate marker remains; remove the stray
"[duplicate_comment]" (or any duplicate review artifact) from the SnapshotsView
component so the JSX return for the Button (using variant="primary" and the
takeSystemSnapshot handler) is clean and contains no leftover review markers or
commented text.
---
Nitpick comments:
In
`@packages/documentation/versioned_docs/version-1.50.0/for-developers/device-integrations/intro.md`:
- Line 12: The sentence is slightly wordy—replace "all of the devices" with "all
the devices" and simplify surrounding phrasing for conciseness; specifically
update the paragraph referencing the Conductor class, TSR, timeline, timeline
layers, and mappings so it reads more tightly (e.g., "The timeline describes the
state of all the devices over time by placing objects on timeline layers, each
mapped to a device part via mappings.")
In `@packages/webui/src/client/ui/Header.tsx`:
- Around line 93-109: The Nav.Link children have inconsistent leading whitespace
(some contain a prefixed space before the {t(...)} call) which can affect
rendering; update the LinkContainer/Nav.Link blocks (see Nav.Link elements
inside LinkContainer for routes "/rundowns", "/testTools", "/status",
"/settings") to remove any leading spaces so every Nav.Link uses the same
pattern: <Nav.Link>{t('...')}</Nav.Link>; ensure components referenced by
allowTesting and allowConfigure follow the same convention and remove the extra
spaces around the {t(...)} expressions.
In `@packages/webui/src/client/ui/RundownView/RundownTiming/withTiming.tsx`:
- Line 199: previousValue is typed as useRef<RundownTimingContext | null> but
actually stores the result of filterGetter.current(...), which is typed by
TimingFilterFunction (any); update the ref type to match the filter return
type—either change the declaration of previousValue to
useRef<ReturnType<TimingFilterFunction> | null> (preferred if
TimingFilterFunction is a function type) or useRef<any | null> / useRef<unknown
| null> if ReturnType is not appropriate—so that previousValue,
filterGetter.current(...), and the deep-equality checks are correctly typed;
adjust the import/typing references to TimingFilterFunction and
RundownTimingContext as needed.
In `@packages/webui/src/client/ui/SegmentTimeline/SegmentContextMenu.tsx`:
- Line 103: In SegmentContextMenu replace the two uses of
dangerouslySetInnerHTML (e.g., span dangerouslySetInnerHTML={{ __html: t('Set
segment as <strong>Next</strong>') }}) with the react-i18next <Trans> component:
import { Trans } from 'react-i18next', change the span to <Trans i18nKey="Set
segment as <strong>Next</strong>"/> (and the similar "Previous" key at the other
occurrence), ensuring any surrounding className or props are preserved; remove
the dangerouslySetInnerHTML usage so translations with <strong> markup are
rendered via <Trans>.
- Around line 53-68: Remove the dead commented-out method onSetAsNextFromHere
and its related commented lines (the block referencing onSetAsNextFromHere,
getTimePosition call and onSetNext) from SegmentContextMenu.tsx; keep the
functional getTimePosition helper and any active handlers like onPlayFromHere,
but delete lines corresponding to the commented legacy method to clean up the
file.
- Around line 81-92: isCurrentPart is being set to true | undefined due to the
trailing "|| undefined"; change its initialization so it is a plain boolean
(e.g. replace "|| undefined" with "?? false" or wrap the expression with "!!")
so isCurrentPart always yields a boolean. Update the assignment for
isCurrentPart (the variable initialized from part, playlist and
playlist.currentPartInfo?.partInstanceId) to use a boolean coercion method so
downstream checks (like disabled props) are clearer and type-consistent.
In `@packages/webui/src/client/ui/Settings/DeviceSettings.tsx`:
- Line 143: Remove the obsolete e.persist() calls because React 18 no longer
pools SyntheticEvents; locate and delete the e.persist() invocation in the
DeviceSettings component (the event handler containing e.persist()) and the
similar e.persist() in the TroubleshootDeviceButton component (around the
handler at the other reported location), leaving the handlers to use the event
object directly or copy needed values into locals if they must be used
asynchronously.
In `@packages/webui/src/client/ui/Shelf/PieceCountdownPanel.tsx`:
- Around line 37-49: The useTracker dependency array uses object/array refs
(playlist, showStyleBase, panel.sourceLayerIds) which can cause unnecessary
reruns; fix by passing stable, primitive-based dependencies or memoized values
instead: derive and pass playlist.currentPartInfo?.partInstanceId and any
playlist id/version, a memoized showStyleBase key (or memoize showStyleBase with
useMemo/useCallback), and memoize panel.sourceLayerIds (e.g., useMemo or a
stable stringified key) so useTracker only re-runs when the actual relevant data
changes; update the dependency array and/or wrap panel.sourceLayerIds in a memo
to reference the same stable variable inside the useTracker closure (referencing
useTracker, getUnfinishedPieceInstancesReactive, livePieceInstance, and
panel.sourceLayerIds).
...ages/documentation/versioned_docs/version-1.51.0/for-developers/device-integrations/intro.md
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/webui/src/client/ui/Settings/ShowStyle/HotkeyLegend.tsx (1)
156-210: ImportHotkeyLegendButton looks good.Proper validation of uploaded JSON, unique
_idgeneration for imported items, and error notification handling are all in place.Nit:
tis listed in the dependency array ofimportHotKeyJSON(Line 182) but isn't used inside the callback body.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/webui/src/client/ui/Settings/ShowStyle/HotkeyLegend.tsx` around lines 156 - 210, In ImportHotkeyLegendButton the importHotKeyJSON useCallback incorrectly includes the translation function t in its dependency array even though t isn't referenced inside that callback; remove t from the dependency array of importHotKeyJSON (leave showStyleBaseId) so the dependency list matches actual captures, or alternatively use t inside the callback if you intend to depend on it—refer to the importHotKeyJSON callback in ImportHotkeyLegendButton to make this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/webui/src/client/ui/Settings/SnapshotsView.tsx`:
- Line 318: The string "Requested by user" passed into storeSystemSnapshot in
SnapshotsView.tsx is not localized; update the call to wrap that literal with
the i18n translator (t) used throughout this file so the user-facing text is
internationalized (e.g., replace the raw string argument to storeSystemSnapshot
with t('Requested by user') or the appropriate translation key), ensuring you
import/use the same t function/context already in this component.
---
Nitpick comments:
In `@packages/webui/src/client/ui/Settings/ShowStyle/HotkeyLegend.tsx`:
- Around line 156-210: In ImportHotkeyLegendButton the importHotKeyJSON
useCallback incorrectly includes the translation function t in its dependency
array even though t isn't referenced inside that callback; remove t from the
dependency array of importHotKeyJSON (leave showStyleBaseId) so the dependency
list matches actual captures, or alternatively use t inside the callback if you
intend to depend on it—refer to the importHotKeyJSON callback in
ImportHotkeyLegendButton to make this change.
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 Superfly
Type of Contribution
This is a: Code improvement
New Behavior
This continues the process of converting the older class based react components into hooks, processing a few more components
Testing
Affected areas
Time Frame
Other Information
Status
Overview
This pull request continues converting React class components to functional components with hooks across the web UI. The change set touches multiple UI files (Header, timing utilities, SegmentTimeline pieces, Settings screens, Shelf/Status components, and some docs), replacing HOC patterns (withTranslation, withTracker, withTiming) with hooks (useTranslation, useTracker, useTiming) and refactoring lifecycle/state logic to hooks.
Key Changes by Component
Header
Timing hook / withTiming
Segment timeline / context menu / small parts
Settings components
Shelf and Status
Minor / Documentation
Implementation patterns
Public API / Export surface changes
Testing / Risk
Summary statistics