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

async api handlers #1698

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

async api handlers #1698

wants to merge 1 commit into from

Conversation

scytacki
Copy link
Member

@scytacki scytacki commented Dec 20, 2024

Refactor the handling of api requests from interactives.
The handlers can now be asynchronous. If an array of api requests is sent by the interactive they will be run one after the other with each waiting for the previous one to finish.
Individual api requests will be run when they come in even if there is another api request or array of request waiting to finish.

The api request loop is changed from an autorun based observer to a reaction observer. This way properties accessed by each handler are not observed. We only want to run the loop when a new request comes in, not when a property changes that a handler accessed.

TODO:

  • the cell editing state handling will be broken when an async handler is used. While the async handler is still running incrementInterruptionCount will called. This is because processItems doesn't wait. However this whole approach does not seem async safe because additional requests might come and finish in while one is waiting.

Refactor the handling of api requests from interactives.
The handlers can now be asynchronous. If an array of api requests is sent by the interactive they will be run one after the other which each waiting for the previous one to finish.
Individual api requests will be run when they come in even if there is another api request or array of request waiting to finish.

The api request loop is changed from an autorun based observer to a reaction observer. This way properties accessed by each handler are not observed. We only want to run the loop when a new request comes in, not when a property changes that a handler accessed.
Copy link

cypress bot commented Dec 20, 2024

codap-v3    Run #5600

Run Properties:  status check passed Passed #5600  •  git commit 32fc4536b8: async api handlers
Project codap-v3
Branch Review 188712457-async-api-handlers
Run status status check passed Passed #5600
Run duration 01m 51s
Commit git commit 32fc4536b8: async api handlers
Committer Scott Cytacki
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 0
Tests that did not run due to a developer annotating a test with .skip  Pending 0
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 3
View all changes introduced in this branch ↗︎

Copy link

codecov bot commented Dec 20, 2024

Codecov Report

Attention: Patch coverage is 93.18182% with 3 lines in your changes missing coverage. Please review.

Project coverage is 68.46%. Comparing base (1db08e0) to head (32fc453).
Report is 21 commits behind head on main.

Files with missing lines Patch % Lines
...onents/web-view/use-data-interactive-controller.ts 91.42% 3 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (1db08e0) and HEAD (32fc453). Click for more details.

HEAD has 9 uploads less than BASE
Flag BASE (1db08e0) HEAD (32fc453)
cypress 10 1
Additional details and impacted files
@@             Coverage Diff             @@
##             main    #1698       +/-   ##
===========================================
- Coverage   85.89%   68.46%   -17.43%     
===========================================
  Files         607      607               
  Lines       31108    31112        +4     
  Branches     8643     8643               
===========================================
- Hits        26720    21302     -5418     
- Misses       4233     9655     +5422     
  Partials      155      155               
Flag Coverage Δ
cypress 43.56% <95.23%> (-31.13%) ⬇️
jest 53.92% <9.09%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@tealefristoe
Copy link
Contributor

@scytacki I tested the shared table plugin on this branch and things look like they're working correctly 👍 This is the plugin that required us to add the API request queue, which is blocked when a table is being edited. Here's how to test it if you want to see for yourself:

  1. Visit https://codap3.concord.org/branch/async-api-handlers/?di=https://codap-shared-table-plugin.concord.org/branch/master/ on two different computers.
  2. Create a new shared table on one computer, then join the shared table on the other.
  3. On one computer, start editing the table, but don't finish--leave the cursor blinking in the table cell.
  4. On the other computer, make several changes to the table.
  5. On the first computer, finish editing the table.

On the first computer, you should not see changes made on the second computer while you are actively editing the cell. When you finish editing the cell, all of the changes made on the second computer should appear. This is how the plugin is designed to work.

@kswenson kswenson added the v3 CODAP v3 label Dec 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants