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

fix: remove session activity updated event as it is not needed #141

Merged
merged 1 commit into from
Jul 25, 2023

Conversation

mohsinht
Copy link
Contributor

Why is this change needed?: https://www.loom.com/share/d6b407d072d54d9b86b20f4830889d79

Problem:

  • SessionActivityUpdated event listens to the changes in Activity. But this has a drawback. The changes to an activity are applied one at a time. For example, when we fill a form, then a sub-activity is added to the Activity document. Which triggers ActivityUpdated event.
  • At this stage when sub-activity is added, the subscription sends a message to client that this Activity has been updated with some new data. Client updates the cache with the new Activity data.
  • Then the ActivityCompleted event is triggered as Activity is marked as 'Done'.

But what happens if ActivityUpdated event happens AFTER ActivityCompleted ? Then the activity is first marked as Done by the ActivityCompleted event and then the activity is marked Active by the ActivityUpdated event. This leaves the Hosted Pages app in a wrong state.

Solution:

  • Remove ActivityUpdated event from the app as it is not required. We only need to listen to statuses of Activities, not their other properties.

@vercel
Copy link

vercel bot commented Jul 24, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
hosted-pages ✅ Ready (Inspect) Visit Preview Jul 24, 2023 9:06pm

@mohsinht mohsinht changed the title fix: remove activity updated subscription as it is not needed fix: remove session activity updated event as it is not needed Jul 24, 2023
Copy link
Contributor

@rahulkeerthi rahulkeerthi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your reasoning makes sense to me. Is this true for all actions, aside from the built in ones? As I understand we don't have any activities today that rely on real-time sync with the backend (e.g. user input only updates the activity state, but does not also complete it). In other words, aside from 'leaving', users can only complete activities once presented with them in our apps -- there is no partial state for an activity. I don't expect that to change for a while yet, though I could see form visibility condition evaluation moving to a subscription to make updates faster. Even in that case, it wouldn't be the activity that is updating

Interested in @FlavioF thoughts here too

@rahulkeerthi
Copy link
Contributor

I'd love to set up a checkly test that can be run X times against a staging (or even preview) build of AHP to test for regressions

@mohsinht mohsinht merged commit 83bc7a2 into main Jul 25, 2023
4 checks passed
@mohsinht mohsinht deleted the fix/remove-activity-updated-subscription branch July 25, 2023 09:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
echoes/intent: unblock Changes needed to solve a bug or unblock a customer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants