Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Delayed events (Futures) / MSC4140 for call widget #12714

Merged
merged 1 commit into from
Aug 7, 2024
Merged

Delayed events (Futures) / MSC4140 for call widget #12714

merged 1 commit into from
Aug 7, 2024

Conversation

AndrewFerr
Copy link
Member

@AndrewFerr AndrewFerr commented Jun 28, 2024 •
edited
Loading

Depends on:

Signed-off-by: Andrew Ferrazzutti [email protected]

Checklist

  • Tests written for new code (and old code if feasible).
  • New or updated public/exported symbols have accurate TSDoc documentation.
  • Linter and other CI checks pass.
  • Sign-off given on the changes (see CONTRIBUTING.md).

@AndrewFerr AndrewFerr added the Z-Experiment Experimental PR, primarily up for its Netlify build, high likelihood of never making it beyond here. label Jun 28, 2024
@AndrewFerr AndrewFerr added the T-Enhancement New features, changes in functionality, performance boosts, user-facing improvements label Jul 10, 2024
@AndrewFerr
Copy link
Member Author

I'll take this out of draft once the dependent js-sdk PR is merged.

@AndrewFerr AndrewFerr marked this pull request as ready for review August 1, 2024 16:29
@AndrewFerr AndrewFerr requested review from a team as code owners August 1, 2024 16:29
@AndrewFerr
Copy link
Member Author

Note that the Static Analysis failure is also happening on develop.

toger5
toger5 approved these changes Aug 2, 2024
View reviewed changes
Copy link
Contributor

@toger5 toger5 left a comment

Choose a reason for hiding this comment

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

Oh forgot how easy this is since all the work is done in the widget-api repo.
This is just the matrix part!

I am positive on this, bu Just to get you confirmation: also the capability checks are done in the widget-api right?

src/stores/widgets/StopGapWidgetDriver.ts Show resolved Hide resolved
@AndrewFerr
Copy link
Member Author

capability checks are done in the widget-api right?

Yes they are, in src/ClientWidgetApi.ts

@AndrewFerr AndrewFerr force-pushed the msc4140-widgets branch 2 times, most recently from 57aaf22 to 1b6774f Compare August 6, 2024 12:16
@AndrewFerr
Copy link
Member Author

The latest batch of commits is just a rebase on the develop branch, for resolving the merge conflict on yarn.lock (where the only conflict was which version of the js-sdk to use). All other content is the same as before.

dbkr
dbkr suggested changes Aug 7, 2024
View reviewed changes
src/stores/widgets/StopGapWidgetDriver.ts
stateKey?: string | null,
targetRoomId?: string,
stateKey: string | null = null,
targetRoomId: string | null = null,
Copy link
Member

Choose a reason for hiding this comment

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

why the changes from undefined to null? I think in general intend want to move in the other direction.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's to match the signature of WidgetApi#sendEvent, which uses string | null = null for these arguments.

It also struck me as dangerous to allow stateKey to be undefined or null when there was only a !== null check on it, and felt that restricting both parameters to just null made things simpler.

Copy link
Member

Choose a reason for hiding this comment

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

ok, it would probably be good to change the widget api at some point but that needn't be today

src/stores/widgets/StopGapWidgetDriver.ts
@@ -328,6 +333,93 @@ export class StopGapWidgetDriver extends WidgetDriver {
return { roomId, eventId: r.event_id };
}

/**
* @experimental Part of MSC4140 & MSC4157
*/
Copy link
Member

Choose a reason for hiding this comment

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

Could this have some doc on the params please? For instance, I'm at a loss as to what parentDelayId might be.

Also, what would it mean to pass null to delay here?

Copy link
Member

@dbkr dbkr Aug 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Also, this is quite a lot of params for a function. May want to consider making it an object (although equally I guess we want it consistent with the rest of the API).

Copy link
Member Author

Choose a reason for hiding this comment

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

This implements WidgetDriver#sendDelayedEvent, which is present as of v1.8.0 & is where the documentation is. I'll add a @see here to link to it.

Also, making this take an object would require updating the widget-api. It doesn't take an object for the sake of avoiding adding a new type in case the whole "parent" idea gets dropped.

test/stores/widgets/StopGapWidgetDriver-test.ts
await expect(driver.sendDelayedEvent(2000, null, EventType.RoomMessage, {})).resolves.toEqual({
roomId,
delayId: "id",
});
Copy link
Member

Choose a reason for hiding this comment

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

Could do with an assertion that the client function is actually called in the way you think it should be called, otherwise this isn't testing what it claims to (same for the rest of these).

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch, thanks!

test/stores/widgets/StopGapWidgetDriver-test.ts Outdated

beforeEach(() => {
driver = mkDefaultDriver();
jest.spyOn(SdkContextClass.instance.roomViewStore, "getRoomId").mockReturnValue(roomId);
Copy link
Member

@dbkr dbkr Aug 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Please remember to always call restoreAllMocks() in an afterEach() if you mock or spy on anything, otherwise your mocks will bleed into other tests.

Copy link
Member Author

Choose a reason for hiding this comment

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

It turns out I don't need this after all, so I'll just get rid of it.

@AndrewFerr AndrewFerr requested a review from dbkr August 7, 2024 13:03
dbkr
dbkr approved these changes Aug 7, 2024
View reviewed changes
Copy link
Member

@dbkr dbkr left a comment

Choose a reason for hiding this comment

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

looks good then!

@AndrewFerr
Support delayed events (MSC4140) for call widget
f5fcf55 
The Widget API spec for delayed events is defined by MSC4157.

Also support "parent" delayed events, which were in a previous version
of MSC4140 and may be reintroduced or be part of a new MSC later.
@AndrewFerr
Copy link
Member Author

I've rebased + squashed the commits to hide some of the churn that built up over time, and also wrote a more descriptive commit message.

@robintown robintown added this pull request to the merge queue Aug 7, 2024
Merged via the queue into matrix-org:develop with commit a437c67 Aug 7, 2024
26 checks passed
@AndrewFerr AndrewFerr deleted the msc4140-widgets branch August 7, 2024 15:23
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@toger5 toger5 toger5 approved these changes

@dbkr dbkr dbkr approved these changes

@robintown robintown Awaiting requested review from robintown robintown is a code owner automatically assigned from matrix-org/element-web-reviewers

Assignees
No one assigned
Labels
T-Enhancement New features, changes in functionality, performance boosts, user-facing improvements Z-Experiment Experimental PR, primarily up for its Netlify build, high likelihood of never making it beyond here.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@AndrewFerr @dbkr @toger5 @robintown