Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: convert action-triggers computation to async #1310

Open
wants to merge 2 commits into
base: release52
Choose a base branch
from

Conversation

Julusian
Copy link
Member

@Julusian Julusian commented Nov 1, 2024

About the Contributor

This pull request is posted on behalf of the BBC

Type of Contribution

This is a: Code improvement

New Behavior

The action-triggers logic is shared between the meteor backend and the webui. This is currently written using sync mongo queries which works in meteor 2, but is not compatible with meteor 3.
This PR reworks it to be using async mongo queries, so that it is ready for meteor 3.

This has become quite a complex task, to figure out how meteor trackers behave when async, and figure out the correct interaction of computations when this happens.
To make this work, the meteor/tracker implementation that was copied into the webui has been updated so that it supports async operation.
This triggers code is very explicit in passing around a computation parameter, partly because async meteor requires this and also because it makes it easier to follow what is reactive and check that it will be using the correct value.the global Tracker.currentComputation will not be correct after an await usage, forcing us to either rebind it Tracker.withComputation(computation, async () => { .... }) or by passing the computation to where it is needed ourselves.

Perhaps a follow up PR should be taking this a step further, and inside of the webui remove the global Tracker object, making it so that we always manually pass around the tracker objects? This would be quite a sprawling change to update every place we are using a tracker the ui.

This PR includes the changes from #1308. It is also dependent on #1309, to ensure that the correct reactivity is visible in the settings.

Testing

  • I have added one or more unit tests for this PR
  • I have updated the relevant unit tests
  • No unit test changes are needed for this PR

I have performed some manual testing by checking that the hotkeys are updating in the shelf while playing out a rundown, with a connected input gateway, and in the settings.
I am not confident that I have sufficiently tested every scenario, as I do not know the area well enough to know what should be tested. Any help or pointers in testing this would be appreciated.

Affected areas

Time Frame

Other Information

Status

  • PR is ready to be reviewed.
  • The functionality has been tested by the author.
  • Relevant unit tests has been added / updated.
  • Relevant documentation (code comments, system documentation) has been added / updated.

@Julusian Julusian added the Contribution from BBC Contributions sponsored by BBC (bbc.co.uk) label Nov 1, 2024
@Julusian Julusian requested a review from a team as a code owner November 1, 2024 15:55
Copy link

codecov bot commented Nov 1, 2024

Codecov Report

Attention: Patch coverage is 44.77766% with 534 lines in your changes missing coverage. Please review.

Project coverage is 60.71%. Comparing base (ea7cece) to head (c2a6522).
Report is 162 commits behind head on release52.

Files with missing lines Patch % Lines
...es/webui/src/client/lib/memoizedIsolatedAutorun.ts 16.47% 71 Missing ⚠️
...eteor/server/api/deviceTriggers/triggersContext.ts 39.47% 69 Missing ⚠️
...eContentStatusUI/rundown/rundownContentObserver.ts 0.00% 62 Missing ⚠️
...eceContentStatusUI/bucket/bucketContentObserver.ts 0.00% 50 Missing ⚠️
...packageManager/expectedPackages/contentObserver.ts 0.00% 42 Missing ⚠️
...erver/api/deviceTriggers/RundownContentObserver.ts 17.14% 29 Missing ⚠️
...ver/publications/partsUI/rundownContentObserver.ts 0.00% 18 Missing ⚠️
meteor/server/api/studio/api.ts 0.00% 17 Missing ⚠️
...ications/partInstancesUI/rundownContentObserver.ts 0.00% 15 Missing ⚠️
meteor/server/api/ingest/rundownInput.ts 43.47% 13 Missing ⚠️
... and 41 more
Additional details and impacted files
@@              Coverage Diff              @@
##           release52    #1310      +/-   ##
=============================================
+ Coverage      57.91%   60.71%   +2.79%     
=============================================
  Files            525      459      -66     
  Lines          84945    79087    -5858     
  Branches        4438     4970     +532     
=============================================
- Hits           49200    48017    -1183     
+ Misses         35689    30927    -4762     
- Partials          56      143      +87     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Julusian Julusian mentioned this pull request Nov 4, 2024
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Contribution from BBC Contributions sponsored by BBC (bbc.co.uk)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant