Skip to content

Conversation

@singhk97
Copy link
Contributor

@singhk97 singhk97 commented Nov 20, 2025

Linked PR: microsoft/teams.ts#398

This is how the architecture looked like before

image

This is what it looks like now:

image

@heyitsaamir
Copy link
Collaborator

heyitsaamir commented Nov 21, 2025

Now that we aren't using events for responses, I think we should make these fundamental changes:

  1. Remove process_activity from event manager and put it back in app. There is no reason for event manager to call process activity now, since the response isn't coming through event manager anymore.
  2. So pull the above in app. Inject that function in. That function can call event_manager.on_activity if it wants, but that would be a fire and forget event.
  3. The on_activity_response can also be called from the above handler then (instead of from process_message). That would clean up that architecture.

@singhk97 singhk97 marked this pull request as ready for review November 21, 2025 21:06
Copilot AI review requested due to automatic review settings November 21, 2025 21:06
@singhk97
Copy link
Contributor Author

Refactored the activity processor out of the event manager and into the plugin processor.

@singhk97 singhk97 changed the title [WIP] Simplify setting http response after activity processing Simplify setting http response after activity processing Nov 21, 2025
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@singhk97
Copy link
Contributor Author

singhk97 commented Dec 1, 2025

So it turns out that the error event is not even being emitted if an exception is raised by one of the handlers. I've pushed a commit to a fix that. The behavior is now:

image

for

image

(apologies for confusing error message)

Copy link
Collaborator

@heyitsaamir heyitsaamir left a comment

Choose a reason for hiding this comment

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

Tested

@heyitsaamir heyitsaamir merged commit e370a68 into main Dec 4, 2025
6 checks passed
@heyitsaamir heyitsaamir deleted the kavin/simplify-activity-response branch December 4, 2025 21:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants