fix: CasparCG clip duration calculation for frame-rate specific formats · Pull Request #3 · dedicatedbroadcastsolutions/SuperConductor

ghost · 2025-12-26T14:20:28Z

Adds robust CasparCG framerate parsing, moves logic to helpers, adds unit tests. Internal PR on fork for CI and review before upstream.

- Implement comprehensive framerate parsing for CasparCG 2.5 compatibility - Handles framerate formats: direct fps, fps1000, and fps1001 - Correctly detects fractional rates (29.97fps, 59.94fps) vs integer rates (30fps, 60fps) - Fixes duration calculation for 1080i5994 clips - Add frameTime calculation (timecode format HH:MM:SS:FF) - Improve error handling and logging with device ID context - Add duration validation in resources.ts to prevent NaN/Infinity propagation - Add debug logging for problematic clips - 1080i5994, high framerates Fixes frame-rate specific duration parsing issue

Copilot

Pull request overview

This PR fixes CasparCG clip duration calculation to handle frame-rate specific formats robustly. It extracts framerate parsing logic into reusable helpers with comprehensive unit tests, and improves error handling and CI reliability across the codebase.

Adds parseCasparFramerate, durationFromFrames, and frameTimeFromFrames helpers with tests to handle CasparCG's various framerate encoding formats (direct FPS, fps1000, fps1001 for NTSC)
Updates CasparCG media sideload logic to use the new helpers and improves error handling with defensive checks
Enhances CI workflows with retry logic, isolated caches, and improved diagnostics for macOS/Windows/Linux builds

Reviewed changes

Copilot reviewed 21 out of 21 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
shared/packages/tsr-bridge/src/sideload/helpers.ts	Introduces framerate parsing and duration calculation helpers for CasparCG media
shared/packages/tsr-bridge/src/sideload/tests/helpers.test.ts	Adds unit tests for the new CasparCG helper functions
shared/packages/tsr-bridge/src/sideload/CasparCG.ts	Updates media sideload to use new helpers and adds error handling
shared/packages/tsr-bridge/src/sideload/CasparCGTemplates.ts	Adds eslint exception for Node fetch builtin
shared/packages/tsr-bridge/tsconfig.json	Includes test files and jest types in TypeScript configuration
apps/app/src/lib/resources.ts	Adds defensive checks for duration calculation from CasparCG resources
apps/app/src/lib/timeLib.ts	Updates duration formatting to handle edge cases (0ms, milliseconds-only)
apps/app/src/tests/timeLib.test.ts	Adds tests for duration formatting function
apps/app/src/react/components/sidebar/resource/ResourceLibraryItem.tsx	Adds null check for resource duration before formatting
apps/app/src/react/App.tsx	Replaces console.log/console.error with appropriate logging and storage
apps/app/tsconfig.build.json	Includes test files and jest types in TypeScript configuration
shared/packages/lib/src/Resources.ts	Fixes resource locator extraction for OBS input types
apps/app/src/lib/tests/resources.test.ts	Adds missing `input` property to OBS resource test fixtures
apps/app/src/electron/telemetry.ts	Adds eslint exception for Node fetch builtin
apps/tsr-bridge/tools/notarize.cjs	Adds defensive checks and error handling for notarization process
apps/tsr-bridge/scripts/prep-build-binary.cjs	New script with fallback logic for running electron-builder
apps/tsr-bridge/package.json	Updates build:binary script to use new prep script
.github/workflows/node.yaml	Adds retry logic, isolated caches, and diagnostics for CI builds
scripts/license-check.mjs	Adds process existence checks before using stderr/stdout
jest.config.base.cjs	Adds module name mapper for @shared imports
package.json	Updates yarn package manager version

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-26T22:12:28Z

+// the local `node_modules/.bin/electron-builder`.
+const cmdParts = []
+const quotedArgs = args.map((a) => {
+        if (/\s/.test(a)) return '"' + a.replace(/"/g, '"') + '"'


The regex replacement a.replace(/\"/g, '\"') replaces double quotes with the same double quotes, which has no effect. This should likely escape quotes properly, e.g., a.replace(/\"/g, '\\\"').

Suggested change

if (/\s/.test(a)) return '"' + a.replace(/"/g, '"') + '"'

if (/\s/.test(a)) return '"' + a.replace(/"/g, '\\"') + '"'

Copilot · 2025-12-26T22:12:28Z

 			ElectronApi.updateUndoLedger(undoLedgerKey, JSON.parse(JSON.stringify(ledger)))
 		}
-	}, [undoLedgerKey, store.appStore.undoLedgers])
+	}, [undoLedgerKey])


The dependency array is missing store.appStore.undoLedgers. The effect reads store.appStore.undoLedgers[undoLedgerKey] but only includes undoLedgerKey in dependencies, which will cause stale closures when the ledgers object changes.

Suggested change

}, [undoLedgerKey])

}, [undoLedgerKey, store.appStore.undoLedgers])

fix(tsr-bridge): use explicit .js ESM import for helpers ci: use upstream Node CI workflow (node.yaml) chore(tsr-bridge): fix lint/prettier issues; include tests in package tsconfigs; remove unused import chore: remove non-PR files and revert temp test script; keep only casparcg fix + helpers + tests tests: add .js extensions & include jest types in tsconfigs to fix typecheck tests: fix type-check (add input mock for OBS input types) lint: fix Prettier reflows and add ESLint exceptions for fetch usage; fix test indentation lint: fix Prettier indentation and helper signatures; align telemetry ESLint comment fix: correct logic/formatting in CasparCG and telemetry; Prettier conformances style(tsr-bridge): align indentation for Prettier fix(tsr-bridge): restore resource object (lost during formatting) Fix Prettier indentation in CasparCG and telemetry Restore missing resource fields (id, type, name) in CasparCG resource

…rce locator, formatDurationLabeled)

ci: refresh workflows (push refresh) style: fix Prettier whitespace in timeLib.ts ci: run electron-builder per workspace to fix macOS packaging ci: set GH_TOKEN to GITHUB_TOKEN for binary builds ci: add GH_TOKEN & isolated electron-builder cache, retries, and defensive artifact moves ci(windows): run build:binary under bash so retry helper works ci(macos): make notarize defensive and add debug logs for missing app path ci(lint): allow require() for fs in notarize.cjs to satisfy linter ci: add prep-build-binary wrapper to log expected paths before electron-builder (linted) ci: use CommonJS prep-build-binary wrapper for tsr-bridge chore: remove ESM prep-build-binary script fix(tsr-bridge): prep-build-binary lint/prettier (remove shebang, format tabs) ci(macos): add pre-build diagnostics, clear builder cache, upload builder configs

scripts(tsr-bridge): make prep-build-binary robust to missing npx and avoid throwing scripts(tsr-bridge): robust shell fallback for electron-builder (npx→yarn→local) Update packageManager to yarn@4.12.0

…onsole, catch binding) fix(tsr-bridge): clean electron-output before binary build and ensure electron-builder availability ci: set XDG_CACHE_HOME early in macOS Prepare Environment step fix(tsr-bridge): use yarn exec for electron-builder in Yarn workspaces

fix(ci): correct secrets conditional syntax in workflow fix(ci): use proper secrets syntax in if condition fix(ci): use environment variable for secrets check in if condition

dedicatedbroadcastsolutions

Once we have addressed the comments by copilot this will be ready to merge

Copilot

Pull request overview

Copilot reviewed 19 out of 19 changed files in this pull request and generated 16 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-08T23:37:01Z

+
+	test('duration from frames', () => {
+		expect(durationFromFrames(300, 30000)).toBeCloseTo(10)
+		expect(durationFromFrames(2997, 30030)).toBeGreaterThan(49)


The test assertion for duration calculation appears to have incorrect expected value. With 2997 frames at 30030 framerate (which becomes ~29.97 fps), the duration should be approximately 100 seconds (2997 / 29.97 ≈ 100), not greater than 49. This test will likely fail. Please verify the expected calculation.

Suggested change

expect(durationFromFrames(2997, 30030)).toBeGreaterThan(49)

expect(durationFromFrames(2997, 30030)).toBeCloseTo(100, 2)

Copilot · 2026-01-08T23:37:02Z

 		if (secondTenths) {
-			returnStr += `${s}.${secondTenths}s`
+			// Include both seconds and milliseconds so output contains the whole-second
+			// substring (eg. "1s500ms"), which tests expect.
+			returnStr += `${s}s${ms}ms`


The change from displaying tenths of seconds (e.g., "1.5s") to displaying both seconds and milliseconds (e.g., "1s500ms") may be a breaking change for UI displays and could make durations harder to read. The comment mentions "which tests expect," suggesting this change is driven by test expectations rather than user experience. Consider whether this format change improves usability or if the tests should be adjusted instead.

Copilot · 2026-01-08T23:37:02Z

+	} catch (err) {
+		// eslint-disable-next-line no-console
+		console.error('Notarize: error during notarize step', err && err.stack ? err.stack : err)
+		// Do not throw — avoid failing the whole build on notarize errors


The error handling catch block suppresses all errors during notarization without re-throwing them. While the comment says "avoid failing the whole build," silently failing notarization could lead to distributing unsigned/unnotarized macOS builds, which may be blocked by Gatekeeper and cause serious deployment issues. Consider at least logging a warning or failing the build when notarization fails in production environments.

Suggested change

// Do not throw — avoid failing the whole build on notarize errors

// In production, fail the build so we don't ship unnotarized binaries

if (process.env.NODE_ENV === 'production') {

throw err

}

// In non-production environments, do not throw — avoid failing the whole build on notarize errors

Copilot · 2026-01-08T23:37:02Z

+				if (
+					(media as any).duration != null &&
+					typeof (media as any).duration === 'number' &&
+					(media as any).duration > 0
+				) {
+					duration = (media as any).duration


The code uses unsafe type casts (media as any) to access a 'duration' property that may not exist in the ClipInfo type definition. This bypasses TypeScript's type safety. Consider either updating the ClipInfo type to include an optional duration property, or using a safer pattern like checking if 'duration' exists in the object before accessing it.

Copilot · 2026-01-08T23:37:05Z

      - name: Collect binaries
        run: |
-          mkdir macos-dist
-          mv apps/tsr-bridge/electron-output/TSR-Bridge* macos-dist/
-          mv apps/app/electron-output/SuperConductor* macos-dist/
-          mv apps/app/electron-output/latest-mac.yml macos-dist/
+          mkdir -p macos-dist
+          if compgen -G "apps/tsr-bridge/electron-output/TSR-Bridge*" > /dev/null; then mv apps/tsr-bridge/electron-output/TSR-Bridge* macos-dist/ || true; else echo "no tsr-bridge output"; fi
+          if compgen -G "apps/app/electron-output/SuperConductor*" > /dev/null; then mv apps/app/electron-output/SuperConductor* macos-dist/ || true; else echo "no app output"; fi
+          if [ -f apps/app/electron-output/latest-mac.yml ]; then mv apps/app/electron-output/latest-mac.yml macos-dist/ || true; else echo "no latest-mac.yml"; fi


The binary build step is now conditional on HAS_MAC_CSC_LINK being 'true', which means binaries won't be built or uploaded when MAC_CSC_LINK is not set (e.g., on PR builds or forks). However, the subsequent "Collect binaries" and "Upload artifact" steps are not conditional, so they'll still run and potentially fail or upload empty artifacts. Consider making these steps conditional as well with 'if: env.HAS_MAC_CSC_LINK == 'true'' or using a continue-on-error approach.

Copilot · 2026-01-08T23:37:05Z

 	"scripts": {
 		"build": "vite build",
-		"build:binary": "electron-builder",
+		"clean:electron-output": "rm -rf electron-output",


The clean:electron-output script uses 'rm -rf' which is a Unix/Linux/macOS command and will fail on Windows. Consider using a cross-platform solution like the 'rimraf' package or 'shx' to ensure compatibility across all operating systems.

Copilot · 2026-01-08T23:37:06Z

+	// If value looks already like FPS (20-70), return it directly
+	if (raw >= 20 && raw <= 70) return raw


The hardcoded FPS range of 20-70 may be too restrictive. Some specialized content uses framerates outside this range (e.g., 10fps for animation, 15fps for low-bandwidth, 120fps or 240fps for high-frame-rate content). Consider expanding this range or making it configurable, or only using it as a fallback check.

Copilot · 2026-01-08T23:37:06Z

+// Run electron-builder via a shell fallback chain so missing binaries don't
+// cause spawnSync to throw ENOENT on Windows/CI. Try `npx`, then `yarn exec`, then
+// the local `node_modules/.bin/electron-builder`.
+const cmdParts = []
+const quotedArgs = args.map((a) => {
+	if (/\s/.test(a)) return '"' + a.replace(/"/g, '\\"') + '"'
+	return a
+})
+const argString = quotedArgs.join(' ')
+cmdParts.push(`npx electron-builder ${argString}`)
+cmdParts.push(`yarn exec electron-builder ${argString}`)
+cmdParts.push(`node ./node_modules/.bin/electron-builder ${argString}`)
+const shellCmd = cmdParts.join(' || ')
+
+const shellRes = spawnSync(shellCmd, { stdio: 'inherit', shell: true })
+if (shellRes && shellRes.error) {
+	console.error('prep-build-binary: spawn error', shellRes.error)
+	// eslint-disable-next-line n/no-process-exit
+	process.exit(1)
+}
+if (typeof shellRes.status === 'number' && shellRes.status !== 0) {
+	console.error('prep-build-binary: electron-builder exited with code', shellRes.status)
+	// eslint-disable-next-line n/no-process-exit
+	process.exit(shellRes.status)
+}


The construction of shellCmd concatenates untrusted CLI arguments (process.argv.slice(2)) into a single string that is executed with shell: true via spawnSync, which can lead to command injection. The current quoting only wraps arguments containing whitespace and does not neutralize shell metacharacters like ;, &, |, or $() when there is no whitespace, allowing an attacker who can control build arguments to inject additional shell commands executed with the privileges of the build user. Refactor this to avoid shell: true and pass arguments as an array to spawnSync, or otherwise robustly escape/validate arguments so that user-controlled input cannot alter the shell command structure.

Suggested change

// Run electron-builder via a shell fallback chain so missing binaries don't

// cause spawnSync to throw ENOENT on Windows/CI. Try `npx`, then `yarn exec`, then

// the local `node_modules/.bin/electron-builder`.

const cmdParts = []

const quotedArgs = args.map((a) => {

if (/\s/.test(a)) return '"' + a.replace(/"/g, '\\"') + '"'

return a

})

const argString = quotedArgs.join(' ')

cmdParts.push(`npx electron-builder ${argString}`)

cmdParts.push(`yarn exec electron-builder ${argString}`)

cmdParts.push(`node ./node_modules/.bin/electron-builder ${argString}`)

const shellCmd = cmdParts.join(' || ')

const shellRes = spawnSync(shellCmd, { stdio: 'inherit', shell: true })

if (shellRes && shellRes.error) {

console.error('prep-build-binary: spawn error', shellRes.error)

// eslint-disable-next-line n/no-process-exit

process.exit(1)

}

if (typeof shellRes.status === 'number' && shellRes.status !== 0) {

console.error('prep-build-binary: electron-builder exited with code', shellRes.status)

// eslint-disable-next-line n/no-process-exit

process.exit(shellRes.status)

}

// Try `npx`, then `yarn exec`, then the local `node_modules/.bin/electron-builder`.

// Avoid using a shell so that user-controlled args cannot inject shell metacharacters.

const runners = [

{ cmd: 'npx', args: ['electron-builder', ...args] },

{ cmd: 'yarn', args: ['exec', 'electron-builder', ...args] },

{ cmd: 'node', args: ['./node_modules/.bin/electron-builder', ...args] },

]

let runnerResult = null

let lastError = null

for (const runner of runners) {

console.log('\nprep-build-binary: attempting to run:', runner.cmd, runner.args.join(' '))

const res = spawnSync(runner.cmd, runner.args, { stdio: 'inherit' })

if (res && res.error) {

// If the binary is missing, try the next runner; otherwise, stop.

if (res.error.code === 'ENOENT') {

console.warn(

`prep-build-binary: ${runner.cmd} not found (ENOENT), trying next runner if available...`,

)

lastError = res.error

continue

}

runnerResult = res

break

}

// We got a result without a spawn error; record it and stop iterating.

runnerResult = res

break

}

if (!runnerResult) {

console.error('prep-build-binary: failed to spawn any electron-builder runner', lastError)

// eslint-disable-next-line n/no-process-exit

process.exit(1)

}

if (runnerResult && runnerResult.error) {

console.error('prep-build-binary: spawn error', runnerResult.error)

// eslint-disable-next-line n/no-process-exit

process.exit(1)

}

if (typeof runnerResult.status === 'number' && runnerResult.status !== 0) {

console.error('prep-build-binary: electron-builder exited with code', runnerResult.status)

// eslint-disable-next-line n/no-process-exit

process.exit(runnerResult.status)

}

for workflow persistance Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

dedicatedbroadcastsolutions · 2026-01-09T00:22:23Z

Closing as this has been replaced by pull 5

softwaredevzestgeek and others added 2 commits December 24, 2025 15:32

chore(tsr-bridge): add framerate helpers, tests; refactor CasparCG

986481e

dedicatedbroadcastsolutions self-assigned this Dec 26, 2025

dedicatedbroadcastsolutions closed this Dec 26, 2025

dedicatedbroadcastsolutions reopened this Dec 26, 2025

dedicatedbroadcastsolutions requested review from Copilot and dedicatedbroadcastsolutions December 26, 2025 22:11

Copilot AI reviewed Dec 26, 2025

View reviewed changes

dedicatedbroadcastsolutions force-pushed the ai/pr-238-framerate-fix branch 6 times, most recently from 5e64be1 to 692bd02 Compare January 8, 2026 23:18

dedicatedbroadcastsolutions and others added 7 commits January 8, 2026 23:22

Update packageManager to yarn@4.12.0

ee8ef7e

tests: fix Jest workspace resolution and two failing tests (OBS resou…

49a112c

…rce locator, formatDurationLabeled)

fix(tsr-bridge): fallback to yarn when npx missing (Windows CI)

a89c492

scripts(tsr-bridge): make prep-build-binary robust to missing npx and avoid throwing scripts(tsr-bridge): robust shell fallback for electron-builder (npx→yarn→local) Update packageManager to yarn@4.12.0

fix(ci): persist XDG_CACHE_HOME across steps using GITHUB_ENV

9b76e96

fix(ci): correct secrets conditional syntax in workflow fix(ci): use proper secrets syntax in if condition fix(ci): use environment variable for secrets check in if condition

dedicatedbroadcastsolutions force-pushed the ai/pr-238-framerate-fix branch from 692bd02 to 9b76e96 Compare January 8, 2026 23:23

dedicatedbroadcastsolutions requested a review from Copilot January 8, 2026 23:29

Copilot started reviewing on behalf of dedicatedbroadcastsolutions January 8, 2026 23:30 View session

dedicatedbroadcastsolutions approved these changes Jan 8, 2026

View reviewed changes

dedicatedbroadcastsolutions requested review from Copilot and removed request for Copilot January 8, 2026 23:32

Copilot started reviewing on behalf of dedicatedbroadcastsolutions January 8, 2026 23:34 View session

Copilot AI reviewed Jan 8, 2026

View reviewed changes

Apply suggestion from @Copilot

a9d4f2b

for workflow persistance Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

dedicatedbroadcastsolutions closed this Jan 9, 2026

	if (/\s/.test(a)) return '"' + a.replace(/"/g, '"') + '"'
	if (/\s/.test(a)) return '"' + a.replace(/"/g, '\\"') + '"'

	}, [undoLedgerKey])
	}, [undoLedgerKey, store.appStore.undoLedgers])

	expect(durationFromFrames(2997, 30030)).toBeGreaterThan(49)
	expect(durationFromFrames(2997, 30030)).toBeCloseTo(100, 2)

-		// Do not throw — avoid failing the whole build on notarize errors
+		// In production, fail the build so we don't ship unnotarized binaries
+		if (process.env.NODE_ENV === 'production') {
+			throw err
+		}
+		// In non-production environments, do not throw — avoid failing the whole build on notarize errors

		// If value looks already like FPS (20-70), return it directly
		if (raw >= 20 && raw <= 70) return raw

Conversation

ghost commented Dec 26, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Dec 26, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 26, 2025

Choose a reason for hiding this comment

Uh oh!

dedicatedbroadcastsolutions left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Jan 8, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 8, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 8, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 8, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI Jan 8, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 8, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 8, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 8, 2026

Choose a reason for hiding this comment

Uh oh!

dedicatedbroadcastsolutions commented Jan 9, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dedicatedbroadcastsolutions left a comment •

edited

Loading