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

WIP - Fetch annotations for each frame separately #4193

Closed
wants to merge 1 commit into from

Conversation

robertknight
Copy link
Member

@robertknight robertknight commented Feb 8, 2022

Explore a client-side solution to sending annotations to correct frame
if annotation URL does not exactly match frame URL (see [1]). In the
client-side solution, a separate search request is made for each frame
and the returned annotations are then associated with that frame.

Some issues to resolve:

  • How can we test this locally with the dev server?
  • What if searches for different frames return the same annotation?
    • Allow annotations to be associated with multiple frames by making the $frameId property an array?
    • Preferentially associate the annotation with the main frame?
  • How should the WebSocket be adapted?
    • A separate WebSocket connection per frame?
  • What do about frame IDs for non-sidebar applications (notebook, stream, single annotation page) where annotations are not associated with frames?
    • Treat the annotation these views like the sidebar when there is only one main frame?

[1] #4191

@robertknight robertknight force-pushed the separate-annotation-fetch-per-frame branch from f1f8edc to 46b2984 Compare February 10, 2022 06:33
@robertknight
Copy link
Member Author

robertknight commented Feb 10, 2022

How should the WebSocket be adapted? A separate WebSocket instance per frame?

This is not ideal, for a few reasons:

  • Some WebSocket messages pertain to annotations, but others pertain to objects which are not associated with frames, such as the user's groups
  • WebSocket connections have a per-connection cost on the backend
  • On the client side there will be added complexity for managing multiple WS connections

On the client side the main problem with WS notifications currently is that we don't know which frame(s) a new, but not updated or deleted, annotation should be assigned to. If the backend could provide information about which URIs in the WS configuration the annotation matches, that would solve the problem.

In the backend it looks like it should be straightforward to determine which URL in the WebSocket configuration matched a particular annotation and include that information in the response.

@robertknight
Copy link
Member Author

Related Slack discussion.

@robertknight
Copy link
Member Author

How should the WebSocket be adapted? A separate WebSocket instance per frame?

A possible incremental step is for the WebSocket to initially only receive updates for the main frame, or whichever one connected first and is still connected.

@robertknight robertknight force-pushed the separate-annotation-fetch-per-frame branch from 46b2984 to 1060a57 Compare March 4, 2022 08:46
@robertknight robertknight force-pushed the separate-annotation-fetch-per-frame branch 2 times, most recently from ac15f23 to a326ca2 Compare March 15, 2022 08:20
Explore a client-side solution to sending annotations to correct frame
if annotation URL does not exactly match frame URL (see [1]). In the
client-side solution, a separate search request is made for each frame
and the returned annotations are then associated with that frame.

Some issues to resolve:

 - How can we test this locally with the dev server?
 - What if searches for different frames return the same annotation?
   Make the `frameId` property of `Annotation` an array?
 - How should the WebSocket be adapted? A separate WebSocket instance
   per frame?
 - What do about frame IDs for non-sidebar applications (notebook,
   stream, single annotation page) where annotations are not associated
   with frames?

[1] #4191
@robertknight robertknight force-pushed the separate-annotation-fetch-per-frame branch from a326ca2 to be84627 Compare March 17, 2022 09:27
@github-actions
Copy link

This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.

@github-actions github-actions bot added the stale Used by https://github.com/probot/stale to label stale issues and pull requests before closing them label Apr 16, 2022
@github-actions github-actions bot closed this Apr 21, 2022
@lyzadanger lyzadanger deleted the separate-annotation-fetch-per-frame branch August 2, 2022 13:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale Used by https://github.com/probot/stale to label stale issues and pull requests before closing them
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant