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

Ensure we're handling client responses for window show message #2814

Conversation

vinistock
Copy link
Member

@vinistock vinistock commented Oct 31, 2024

Motivation

When the server makes a request to the client, the response doesn't include the requested method by default. We can make it so that add-ons need to include it as part of the message properties, but normally only the ID can be used to reconcile request and response.

We need to let add-ons include the method name as part of the properties so that we can delegate appropriately.

Implementation

Our regular process_message would just ignore any responses because message[:method] is always nil in a response. We need to extract it from result and define that add-ons must include the method as part of it.

Automated Tests

Adapted the test.

Copy link
Member Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @vinistock and the rest of your teammates on Graphite Graphite

@vinistock vinistock requested review from andyw8 and st0012 October 31, 2024 20:19
@vinistock vinistock added server This pull request should be included in the server gem's release notes bugfix This PR will fix an existing bug labels Oct 31, 2024 — with Graphite App
@vinistock vinistock marked this pull request as ready for review October 31, 2024 20:19
@vinistock vinistock requested a review from a team as a code owner October 31, 2024 20:19
@vinistock vinistock self-assigned this Oct 31, 2024
@vinistock vinistock merged commit 34c38ce into main Nov 1, 2024
38 checks passed
@vinistock vinistock deleted the 10-31-ensure_we_re_handling_client_responses_for_window_show_message branch November 1, 2024 15:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix This PR will fix an existing bug server This pull request should be included in the server gem's release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants