Refactor runtime service #1722
Refactor runtime service #1722
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis change refactors state update logic in the runtime service, enhances and adds new utility functions for more precise update detection, updates type definitions to use new types, and removes the associated test file for rundown service utilities. The update consolidates and simplifies how state changes are detected and broadcast, with stricter equality checks and improved type handling. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
1 similar comment
✅ Actions performedFull review 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: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
packages/types/src/definitions/runtime/CurrentGroupState.type.tsis excluded by none and included by nonepackages/types/src/definitions/runtime/RuntimeStore.type.tsis excluded by none and included by nonepackages/types/src/index.tsis excluded by none and included by none
📒 Files selected for processing (4)
apps/server/src/services/runtime-service/RuntimeService.ts(5 hunks)apps/server/src/services/runtime-service/__tests__/rundownService.utils.test.ts(0 hunks)apps/server/src/services/runtime-service/rundownService.utils.ts(2 hunks)apps/server/src/stores/runtimeState.ts(3 hunks)
💤 Files with no reviewable changes (1)
- apps/server/src/services/runtime-service/tests/rundownService.utils.test.ts
🔇 Additional comments (7)
apps/server/src/stores/runtimeState.ts (2)
2-3: Type refactoring looks goodThe migration from
CurrentGroupStatetoGroupStateandEntryMetaDatatoUpcomingEntryis consistent throughout the type definitions.Also applies to: 39-39, 41-41
807-807: No references to removed nextFlag properties foundSearches did not uncover any uses of
nextFlag.actualStartornextFlag.expectedEndelsewhere in the codebase. The simplified shape—onlyidandexpectedStart—matches its type definition and introduces no breaking dependencies.apps/server/src/services/runtime-service/rundownService.utils.ts (2)
18-24: Well-designed utility for second-level change detectionThe
isNewSecondfunction properly handles timer direction and undefined values, providing a clean abstraction for detecting second transitions.
79-105: Consistent and well-structured update detection logicThe new update detection functions (
getShouldGroupUpdateandgetShouldFlagUpdate) follow a consistent pattern with clear handling of undefined values and optional deep equality checks. The inline comments effectively document which fields are intentionally excluded from comparisons.apps/server/src/services/runtime-service/RuntimeService.ts (3)
16-16: Import updates align with refactored state managementThe migration to
strictDeepEqualand the new utility functions properly support the enhanced state update detection logic.Also applies to: 37-41
685-741: Excellent refactoring of broadcast logicThe refactored broadcast logic is cleaner and more maintainable with consistent use of utility functions and
structuredClonefor state management.Note: Ensure
structuredCloneis available in your Node.js version (requires Node.js 17+).
752-764: Improved event update detectionThe simplified event update logic with
strictDeepEqualprovides more reliable change detection for complex event objects.
|
|
||
| // 4. find if we need to update the timer | ||
| const shouldUpdateTimer = getShouldTimerUpdate(this.lastIntegrationTimerValue, newState.timer.current); | ||
| const shouldUpdateTimer = isNewSecond(this.lastIntegrationTimerValue, newState.timer.current); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify timer update detection is sufficient
The change from getShouldTimerUpdate to isNewSecond narrows the update detection to only second-level changes. This might miss other timer state changes (e.g., playback state, phase changes) that should trigger integrations.
🏁 Script executed:
#!/bin/bash
# Check if there are other places where timer state changes trigger integrations
rg "TimerLifeCycle\.onUpdate" -A 5 -B 5 --type tsLength of output: 1056
To compare the old and new behaviors, let’s inspect the implementations of getShouldTimerUpdate and isNewSecond. Please run:
🏁 Script executed:
#!/bin/bash
# Locate getShouldTimerUpdate definition and context
rg "getShouldTimerUpdate" -A 5 -B 5 --type ts
# Locate isNewSecond implementation
rg "isNewSecond" -A 5 -B 5 --type tsLength of output: 7969
Ensure comprehensive timer update detection
The new code in apps/server/src/services/runtime-service/RuntimeService.ts (around line 147) only calls:
const shouldUpdateTimer = isNewSecond(
this.lastIntegrationTimerValue,
newState.timer.current
);By replacing getShouldTimerUpdate(previousTimer, currentTimer) with a single isNewSecond check, integrations will no longer fire when any of these timer properties change:
secondaryTimeraddedTimedurationphaseplayback
Please restore or extend the logic—e.g. by invoking getShouldTimerUpdate(this.previousState.timer, newState.timer)—so that updates to all relevant timer fields (not just the second tick) trigger the integration.
🤖 Prompt for AI Agents
In apps/server/src/services/runtime-service/RuntimeService.ts at line 147, the
current code only checks for timer updates using isNewSecond, which misses
changes in other timer properties like secondaryTimer, addedTime, duration,
phase, and playback. Replace the isNewSecond call with getShouldTimerUpdate,
passing this.previousState.timer and newState.timer, to ensure all relevant
timer fields are checked and trigger integration updates accordingly.
cpvalente
left a comment
cpvalente
left a comment
There was a problem hiding this comment.
small changes necessary, but good to merge
apps/server/src/services/runtime-service/rundownService.utils.ts
Outdated
Show resolved
Hide resolved
apps/server/src/services/runtime-service/rundownService.utils.ts
Outdated
Show resolved
Hide resolved
apps/server/src/services/runtime-service/rundownService.utils.ts
Outdated
Show resolved
Hide resolved
apps/server/src/services/runtime-service/rundownService.utils.ts
Outdated
Show resolved
Hide resolved
apps/server/src/services/runtime-service/rundownService.utils.ts
Outdated
Show resolved
Hide resolved
alex-Arc
force-pushed
the
refactor-runtime-service
branch
from
b11ab39 to
ebd6a6f
Compare
* refactor: trim type * refactor: update runtime service data push logic * keep using just deep * reword didDependencyUpdate * remove dulicate
No description provided.