feat(WidgetDriver): Pass Matrix API errors to the widget #4241

toger5 · 2024-11-10T22:31:26Z

Currently the WidgetDriver just returns unspecified error strings to the widget that can be used to display an issue description to the user. It is not helpful to run code like a retry or other error mitigation logic.

Here it is proposed to add standardized errors for issues that every widget driver implementation can run into (all matrix cs api errors): matrix-org/matrix-spec-proposals#2762 (comment)

This PR forwards the errors that occur during the widget processing to the widget in the correct format.

NOTE:
It does not include request Url and http Headers. See also: matrix-org/matrix-spec-proposals#2762 (comment)

Public API changes documented in changelogs (optional)

Signed-off-by:

codecov · 2024-11-13T09:16:19Z

Codecov Report

Attention: Patch coverage is 81.25000% with 9 lines in your changes missing coverage. Please review.

Project coverage is 84.94%. Comparing base (b8a61cf) to head (6b3af4e).
Report is 36 commits behind head on main.

Files with missing lines	Patch %	Lines
crates/matrix-sdk/src/widget/machine/mod.rs	86.20%	4 Missing ⚠️
crates/matrix-sdk/src/widget/matrix.rs	33.33%	2 Missing ⚠️
crates/matrix-sdk/src/widget/mod.rs	60.00%	2 Missing ⚠️
...rates/matrix-sdk/src/widget/machine/from_widget.rs	90.90%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4241      +/-   ##
==========================================
+ Coverage   84.87%   84.94%   +0.06%     
==========================================
  Files         274      274              
  Lines       29719    29735      +16     
==========================================
+ Hits        25225    25257      +32     
+ Misses       4494     4478      -16

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

bnjbvr

Makes sense! But we need many more comments for this, please; let's try to add them as we touch these files.

bnjbvr · 2024-11-14T08:07:13Z

crates/matrix-sdk/src/widget/machine/driver_req.rs

@@ -76,7 +76,7 @@ where

    pub(crate) fn then(
        self,
-        response_handler: impl FnOnce(Result<T, String>, &mut WidgetMachine) -> Vec<Action>
+        response_handler: impl FnOnce(Result<T, crate::Error>, &mut WidgetMachine) -> Vec<Action>


Since you're around, can you add a doc comment to this function please?

bnjbvr · 2024-11-14T08:09:34Z

crates/matrix-sdk/src/widget/machine/from_widget.rs

    }
 }

 #[derive(Serialize)]
 struct FromWidgetError {
    message: String,
+    matrix_api_error: Option<FromWidgetMatrixErrorBody>,


This crate is barely documented, which makes it super hard to understand how it works. Let's change that; of course I wouldn't have you change all of it at once, so let's try to do it incrementally. The doc comments don't have to be long; a single short descriptive line is sufficient to give us a rough idea of why/how it's used.

Please add a doc comment to:

every single struct/enum you're adding fields to (here, FromWidgetError)

every field/variant from those structs/enums (here, message above)

every new struct/enum/field/variant

bnjbvr · 2024-11-14T08:10:08Z

crates/matrix-sdk/src/widget/machine/from_widget.rs

+    http_headers: HashMap<String, String>,
+    url: String,
+    response: Value,
+}


nit: always add a newline after a block closing

Suggested change

}

}

bnjbvr · 2024-11-14T08:10:21Z

crates/matrix-sdk/src/widget/machine/from_widget.rs

+struct FromWidgetMatrixErrorBody {
+    http_status: u32,
+    // TODO: figure out the which type to use here.
+    http_headers: HashMap<String, String>,
+    url: String,
+    response: Value,


I want doc comments everywhere here too please :)

bnjbvr · 2024-11-14T08:22:00Z

crates/matrix-sdk/src/widget/machine/incoming.rs

@@ -38,7 +38,7 @@ pub(crate) enum IncomingMessage {
        request_id: Uuid,

        /// The result of the request: response data or error message.
-        response: Result<MatrixDriverResponse, String>,
+        response: Result<MatrixDriverResponse, crate::Error>,


The doc comment above needs an update^^

bnjbvr · 2024-11-14T08:28:27Z

crates/matrix-sdk/src/widget/matrix.rs

@@ -118,7 +116,7 @@ impl MatrixDriver {
        state_key: Option<String>,
        content: Box<RawJsonValue>,
        delayed_event_parameters: Option<delayed_events::DelayParameters>,
-    ) -> Result<SendEventResponse> {
+    ) -> Result<SendEventResponse, Error> {


bnjbvr · 2024-11-14T08:28:32Z

crates/matrix-sdk/src/widget/matrix.rs

@@ -54,17 +52,17 @@ impl MatrixDriver {
    }

    /// Requests an OpenID token for the current user.
-    pub(crate) async fn get_open_id(&self) -> HttpResult<OpenIdResponse> {
+    pub(crate) async fn get_open_id(&self) -> Result<OpenIdResponse, Error> {


no need to specify the error type here neither

bnjbvr · 2024-11-14T08:28:38Z

crates/matrix-sdk/src/widget/matrix.rs

@@ -167,9 +165,9 @@ impl MatrixDriver {
        &self,
        delay_id: String,
        action: UpdateAction,
-    ) -> HttpResult<delayed_events::update_delayed_event::unstable::Response> {
+    ) -> Result<delayed_events::update_delayed_event::unstable::Response, Error> {


nit: no need to specify the Error here too

bnjbvr · 2024-11-14T08:29:29Z

crates/matrix-sdk/src/widget/mod.rs

@@ -195,7 +195,7 @@ impl<T: CapabilitiesProvider> ProcessingContext<T> {
                self.to_widget_tx.send(msg).await.map_err(|_| ())?;
            }
            Action::MatrixDriverRequest { request_id, data } => {
-                let response = match data {
+                let response: Result<MatrixDriverResponse, Error> = match data {


Is this explicit type declaration needed?

bnjbvr · 2024-11-14T08:29:54Z

crates/matrix-sdk/tests/integration/widget.rs

+    .await;
+
+    Mock::given(method("PUT"))
+        .and(path_regex(r"^/_matrix/client/v3/rooms/.*/send/m.room.message/.*$"))


We have a mock in the MatrixMockServer called .mock_room_send(), can you use that instead?

toger5 force-pushed the toger5/widget-driver-matrix-api-errors branch 2 times, most recently from 1b1a0fb to 0dd249f Compare November 13, 2024 09:01

feat(WidgetDriver): Pass matrix api errors to the widget

cccf7a0

toger5 mentioned this pull request Nov 13, 2024

MSC2762: Allowing widgets to send/receive events matrix-org/matrix-spec-proposals#2762

Open

toger5 force-pushed the toger5/widget-driver-matrix-api-errors branch from 0dd249f to cccf7a0 Compare November 13, 2024 09:18

fix clippy

6b3af4e

toger5 marked this pull request as ready for review November 13, 2024 17:09

toger5 requested a review from a team as a code owner November 13, 2024 17:09

toger5 requested review from bnjbvr and removed request for a team November 13, 2024 17:09

bnjbvr requested changes Nov 14, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(WidgetDriver): Pass Matrix API errors to the widget #4241

feat(WidgetDriver): Pass Matrix API errors to the widget #4241

toger5 commented Nov 10, 2024 •

edited

Loading

codecov bot commented Nov 13, 2024 •

edited

Loading

bnjbvr left a comment

bnjbvr Nov 14, 2024

bnjbvr Nov 14, 2024

bnjbvr Nov 14, 2024

bnjbvr Nov 14, 2024

bnjbvr Nov 14, 2024

bnjbvr Nov 14, 2024

bnjbvr Nov 14, 2024

bnjbvr Nov 14, 2024

bnjbvr Nov 14, 2024

bnjbvr Nov 14, 2024

feat(WidgetDriver): Pass Matrix API errors to the widget #4241

Are you sure you want to change the base?

feat(WidgetDriver): Pass Matrix API errors to the widget #4241

Conversation

toger5 commented Nov 10, 2024 • edited Loading

codecov bot commented Nov 13, 2024 • edited Loading

Codecov Report

bnjbvr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

toger5 commented Nov 10, 2024 •

edited

Loading

codecov bot commented Nov 13, 2024 •

edited

Loading