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 sync init when thread unread notif is not supported #2739

Merged
merged 10 commits into from
Oct 7, 2022
62 changes: 62 additions & 0 deletions spec/unit/feature.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
/*
Copyright 2022 The Matrix.org Foundation C.I.C.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

import { buildFeatureSupportMap, Feature } from "../../src/feature";

describe("Feature detection", () => {
it("checks the matrix version", async () => {
const support = await buildFeatureSupportMap({
versions: ["v1.3"],
unstable_features: {},
});

expect(support.get(Feature.Thread)).toBe(true);
expect(support.get(Feature.ThreadUnreadNotifications)).toBe(false);
});

it("checks the matrix msc number", async () => {
const support = await buildFeatureSupportMap({
versions: ["v1.2"],
unstable_features: {
"org.matrix.msc3771": true,
"org.matrix.msc3773": true,
},
});
expect(support.get(Feature.ThreadUnreadNotifications)).toBe(true);
});

it("requires two MSCs to pass", async () => {
const support = await buildFeatureSupportMap({
versions: ["v1.2"],
unstable_features: {
"org.matrix.msc3771": false,
"org.matrix.msc3773": true,
},
});
expect(support.get(Feature.ThreadUnreadNotifications)).toBe(false);
});

it("requires two MSCs OR matrix versions to pass", async () => {
const support = await buildFeatureSupportMap({
versions: ["v1.4"],
unstable_features: {
"org.matrix.msc3771": false,
"org.matrix.msc3773": true,
},
});
expect(support.get(Feature.ThreadUnreadNotifications)).toBe(true);
});
});
8 changes: 7 additions & 1 deletion src/client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,7 @@ import { MAIN_ROOM_TIMELINE } from "./models/read-receipt";
import { IgnoredInvites } from "./models/invites-ignorer";
import { UIARequest, UIAResponse } from "./@types/uia";
import { LocalNotificationSettings } from "./@types/local_notifications";
import { buildFeatureSupportMap, Feature } from "./feature";

export type Store = IStore;

Expand Down Expand Up @@ -528,7 +529,7 @@ export interface ITurnServer {
credential: string;
}

interface IServerVersions {
export interface IServerVersions {
versions: string[];
unstable_features: Record<string, boolean>;
}
Expand Down Expand Up @@ -967,6 +968,8 @@ export class MatrixClient extends TypedEventEmitter<EmittedEvents, ClientEventHa
protected clientWellKnownIntervalID: ReturnType<typeof setInterval>;
protected canResetTimelineCallback: ResetTimelineCallback;

public canSupport = new Map<Feature, boolean>();

// The pushprocessor caches useful things, so keep one and re-use it
protected pushProcessor = new PushProcessor(this);

Expand Down Expand Up @@ -1197,6 +1200,9 @@ export class MatrixClient extends TypedEventEmitter<EmittedEvents, ClientEventHa
this.syncApi.stop();
}

const serverVersions = await this.getVersions();
this.canSupport = await buildFeatureSupportMap(serverVersions);

const { threads, list } = await this.doesServerSupportThread();
Thread.setServerSideSupport(threads);
Thread.setServerSideListSupport(list);
Expand Down
56 changes: 56 additions & 0 deletions src/feature.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
/*
Copyright 2022 The Matrix.org Foundation C.I.C.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

import { IServerVersions } from "./client";

export enum Feature {
Thread = "Thread",
ThreadUnreadNotifications = "ThreadUnreadNotifications",
}

type FeatureSupportCondition = {
unstablePrefixes?: string[];
matrixVersion?: string;
};

const featureSupportResolver: Record<string, FeatureSupportCondition> = {
[Feature.Thread]: {
unstablePrefixes: ["org.matrix.msc3440"],
matrixVersion: "v1.3",
},
[Feature.ThreadUnreadNotifications]: {
unstablePrefixes: ["org.matrix.msc3771", "org.matrix.msc3773"],
matrixVersion: "v1.4",
},
};

export async function buildFeatureSupportMap(versions: IServerVersions): Promise<Map<Feature, boolean>> {
const supportMap = new Map<Feature, boolean>();

for (const [feature, supportCondition] of Object.entries(featureSupportResolver)) {
const supportUnstablePrefixes = supportCondition.unstablePrefixes?.every(unstablePrefix => {
return versions.unstable_features?.[unstablePrefix] === true;
}) ?? false;

const supportMatrixVersion = versions.versions?.includes(supportCondition.matrixVersion || "") ?? false;
supportMap.set(
feature as Feature,
supportUnstablePrefixes || supportMatrixVersion,
);
}

return supportMap;
}
2 changes: 1 addition & 1 deletion src/filter-component.ts
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ export interface IFilterComponent {
* @param {Object} filterJson the definition of this filter JSON, e.g. { 'contains_url': true }
*/
export class FilterComponent {
constructor(private filterJson: IFilterComponent, public readonly userId?: string) {}
constructor(private filterJson: IFilterComponent, public readonly userId?: string | undefined | null) {}

/**
* Checks with the filter component matches the given event
Expand Down
4 changes: 2 additions & 2 deletions src/filter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ export class Filter {
* @param {Object} jsonObj
* @return {Filter}
*/
public static fromJson(userId: string, filterId: string, jsonObj: IFilterDefinition): Filter {
public static fromJson(userId: string | undefined | null, filterId: string, jsonObj: IFilterDefinition): Filter {
const filter = new Filter(userId, filterId);
filter.setDefinition(jsonObj);
return filter;
Expand All @@ -109,7 +109,7 @@ export class Filter {
private roomFilter: FilterComponent;
private roomTimelineFilter: FilterComponent;

constructor(public readonly userId: string, public filterId?: string) {}
constructor(public readonly userId: string | undefined | null, public filterId?: string) {}

/**
* Get the ID of this filter on your homeserver (if known)
Expand Down
2 changes: 1 addition & 1 deletion src/store/memory.ts
Original file line number Diff line number Diff line change
Expand Up @@ -227,7 +227,7 @@ export class MemoryStore implements IStore {
* @param {Filter} filter
*/
public storeFilter(filter: Filter): void {
if (!filter) {
if (!filter || !filter.userId) {
germain-gg marked this conversation as resolved.
Show resolved Hide resolved
return;
}
if (!this.filters[filter.userId]) {
Expand Down
11 changes: 6 additions & 5 deletions src/sync.ts
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ import { BeaconEvent } from "./models/beacon";
import { IEventsResponse } from "./@types/requests";
import { IAbortablePromise } from "./@types/partials";
import { UNREAD_THREAD_NOTIFICATIONS } from "./@types/sync";
import { Feature } from "./feature";

const DEBUG = true;

Expand Down Expand Up @@ -562,7 +563,11 @@ export class SyncApi {
};

private buildDefaultFilter = () => {
return new Filter(this.client.credentials.userId);
const filter = new Filter(this.client.credentials.userId);
if (this.client.canSupport.get(Feature.ThreadUnreadNotifications)) {
filter.setUnreadThreadNotifications(true);
Copy link
Member

Choose a reason for hiding this comment

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

setUnreadThreadNotifications only uses the stable non-prefixed version but this.client.canSupport.get(Feature.ThreadUnreadNotifications) will return true if unstable is supported.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! I've changed the code to use UnstableValue.
I believe I had not done that in the first place after seeing how setProp works... it essentially treats . in property names as a nested level

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should update setProp to take a variadic list of keys rather than a string to avoid this problem

}
return filter;
};

private checkLazyLoadStatus = async () => {
Expand Down Expand Up @@ -706,10 +711,6 @@ export class SyncApi {
const initialFilter = this.buildDefaultFilter();
initialFilter.setDefinition(filter.getDefinition());
initialFilter.setTimelineLimit(this.opts.initialSyncLimit);
const supportsThreadNotifications =
await this.client.doesServerSupportUnstableFeature("org.matrix.msc3773")
|| await this.client.isVersionSupported("v1.4");
initialFilter.setUnreadThreadNotifications(supportsThreadNotifications);
// Use an inline filter, no point uploading it for a single usage
firstSyncFilter = JSON.stringify(initialFilter.getDefinition());
}
Expand Down