Skip to content
Open
64 changes: 64 additions & 0 deletions app/spec/models/query-subscription-pool-spec.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
import QuerySubscriptionPool from '../../src/flux/models/query-subscription-pool';
import DatabaseStore from '../../src/flux/stores/database-store';
import { Label } from '../../src/flux/models/label';
import { Thread } from '../../src/flux/models/thread';
import { Folder } from '../../src/flux/models/folder';
import { ChangeFolderTask } from '../../src/flux/tasks/change-folder-task';
import { ChangeLabelsTask } from '../../src/flux/tasks/change-labels-task';

describe('QuerySubscriptionPool', function QuerySubscriptionPoolSpecs() {
beforeEach(() => {
Expand Down Expand Up @@ -57,4 +61,64 @@ describe('QuerySubscriptionPool', function QuerySubscriptionPoolSpecs() {
});
});
});

describe('_threadIdsForRemovalTask', () => {
it('should return threadIds for a ChangeFolderTask', () => {
const threads = [
new Thread({ id: 't1', accountId: 'a1', folders: [new Folder({ id: 'f1' })] }),
];
const task = new ChangeFolderTask({
threads,
folder: new Folder({ id: 'trash-folder', role: 'trash', accountId: 'a1' }),
});
const result = QuerySubscriptionPool._threadIdsForRemovalTask(task);
expect(result).toEqual(['t1']);
});

it('should return threadIds for a ChangeLabelsTask with only removals', () => {
const task = new ChangeLabelsTask({
threads: [new Thread({ id: 't2', accountId: 'a1' })],
labelsToRemove: [new Label({ id: 'inbox', role: 'inbox', accountId: 'a1' })],
labelsToAdd: [],
});
const result = QuerySubscriptionPool._threadIdsForRemovalTask(task);
expect(result).toEqual(['t2']);
});

it('should return null for a ChangeLabelsTask with additions', () => {
const task = new ChangeLabelsTask({
threads: [new Thread({ id: 't3', accountId: 'a1' })],
labelsToRemove: [new Label({ id: 'inbox', role: 'inbox', accountId: 'a1' })],
labelsToAdd: [new Label({ id: 'archive', role: 'all', accountId: 'a1' })],
});
const result = QuerySubscriptionPool._threadIdsForRemovalTask(task);
expect(result).toBeNull();
});
});

describe('_optimisticallyRemoveThreads', () => {
it('should call optimisticallyRemoveItemsById on Thread subscriptions', () => {
const threadQuery = DatabaseStore.findAll<Thread>(Thread);
const threadKey = threadQuery.sql();
const callback = jasmine.createSpy('callback');
QuerySubscriptionPool.add(threadQuery, callback);
const subscription = QuerySubscriptionPool._subscriptions[threadKey];
spyOn(subscription, 'optimisticallyRemoveItemsById');

QuerySubscriptionPool._optimisticallyRemoveThreads(['t1', 't2']);
expect(subscription.optimisticallyRemoveItemsById).toHaveBeenCalledWith(['t1', 't2']);
});

it('should not call optimisticallyRemoveItemsById on non-Thread subscriptions', () => {
const labelQuery = DatabaseStore.findAll<Label>(Label);
const labelKey = labelQuery.sql();
const callback = jasmine.createSpy('callback');
QuerySubscriptionPool.add(labelQuery, callback);
const subscription = QuerySubscriptionPool._subscriptions[labelKey];
spyOn(subscription, 'optimisticallyRemoveItemsById');

QuerySubscriptionPool._optimisticallyRemoveThreads(['t1']);
expect(subscription.optimisticallyRemoveItemsById).not.toHaveBeenCalled();
});
});
});
45 changes: 42 additions & 3 deletions app/spec/models/query-subscription-spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ describe('QuerySubscription', function QuerySubscriptionSpecs() {
describe('when initialModels are provided', () =>
it('should apply the models and trigger', () => {
const query = DatabaseStore.findAll<Thread>(Thread);
const threads = [1, 2, 3, 4, 5].map(i => new Thread({ id: i }));
const threads = [1, 2, 3, 4, 5].map((i) => new Thread({ id: i }));
const subscription = new QuerySubscription(query, { initialModels: threads });
expect(subscription._set).not.toBe(null);
}));
Expand Down Expand Up @@ -222,8 +222,8 @@ describe('QuerySubscription', function QuerySubscriptionSpecs() {
jasmine.unspy(Utils, 'generateTempId');

describe('scenarios', () =>
scenarios.forEach(scenario => {
scenario.tests.forEach(test => {
scenarios.forEach((scenario) => {
scenario.tests.forEach((test) => {
it(`with ${scenario.name}, should correctly apply ${test.name}`, () => {
const subscription = new QuerySubscription(scenario.query);
subscription._set = new MutableQueryResultSet();
Expand Down Expand Up @@ -348,4 +348,43 @@ describe('QuerySubscription', function QuerySubscriptionSpecs() {
});
});
});

describe('optimisticallyRemoveItemsById', () => {
it('should remove items from the set and trigger callbacks', () => {
const query = DatabaseStore.findAll<Thread>(Thread);
const threads = [1, 2, 3, 4, 5].map((i) => new Thread({ id: `${i}` }));
const subscription = new QuerySubscription(query, { initialModels: threads });

spyOn(subscription, '_createResultAndTrigger');
subscription.optimisticallyRemoveItemsById(['2', '4']);

expect(subscription._set.offsetOfId('2')).toBe(-1);
expect(subscription._set.offsetOfId('4')).toBe(-1);
expect(subscription._set.ids().length).toBe(3);
expect(subscription._createResultAndTrigger).toHaveBeenCalled();
});

it('should not trigger if no items were in the set', () => {
const query = DatabaseStore.findAll<Thread>(Thread);
const threads = [1, 2, 3].map((i) => new Thread({ id: `${i}` }));
const subscription = new QuerySubscription(query, { initialModels: threads });

spyOn(subscription, '_createResultAndTrigger');
subscription.optimisticallyRemoveItemsById(['99', '100']);

expect(subscription._set.ids().length).toBe(3);
expect(subscription._createResultAndTrigger).not.toHaveBeenCalled();
});

it('should do nothing if _set is null', () => {
spyOn(QuerySubscription.prototype, 'update').andReturn();
const query = DatabaseStore.findAll<Thread>(Thread);
const subscription = new QuerySubscription(query);
subscription._set = null;

expect(() => {
subscription.optimisticallyRemoveItemsById(['1', '2']);
}).not.toThrow();
});
});
});
53 changes: 53 additions & 0 deletions app/src/flux/models/query-subscription-pool.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { QuerySubscription } from './query-subscription';
import { DatabaseChangeRecord } from '../stores/database-change-record';
import ModelQuery from './query';
import { Model } from './model';
import * as Actions from '../actions';
let DatabaseStore = null;

/*
Expand Down Expand Up @@ -105,6 +106,8 @@ class QuerySubscriptionPool {
_setup() {
DatabaseStore = DatabaseStore || require('../stores/database-store').default;
DatabaseStore.listen(this._onChange);
Actions.queueTask.listen(this._onQueueTask);
Actions.queueTasks.listen(this._onQueueTasks);
}

_onChange = (record: DatabaseChangeRecord<Model>) => {
Expand All @@ -113,6 +116,56 @@ class QuerySubscriptionPool {
subscription.applyChangeRecord(record);
}
};

_onQueueTask = (task) => {
const threadIds = this._threadIdsForRemovalTask(task);
if (threadIds && threadIds.length > 0) {
this._optimisticallyRemoveThreads(threadIds);
}
};

_onQueueTasks = (tasks) => {
if (!tasks || !tasks.length) return;
const allIds: string[] = [];
for (const task of tasks) {
const ids = this._threadIdsForRemovalTask(task);
if (ids) {
allIds.push(...ids);
}
}
if (allIds.length > 0) {
this._optimisticallyRemoveThreads(allIds);
}
};

_threadIdsForRemovalTask(task): string[] | null {
Comment thread
indent-staging[bot] marked this conversation as resolved.
Outdated
const ChangeFolderTask = require('../tasks/change-folder-task').ChangeFolderTask;
const ChangeLabelsTask = require('../tasks/change-labels-task').ChangeLabelsTask;

if (task instanceof ChangeFolderTask && task.threadIds && task.threadIds.length > 0) {
Comment thread
indent-staging[bot] marked this conversation as resolved.
Outdated
return task.threadIds;
}
if (
task instanceof ChangeLabelsTask &&
task.threadIds &&
task.threadIds.length > 0 &&
task.labelsToRemove &&
task.labelsToRemove.length > 0 &&
(!task.labelsToAdd || task.labelsToAdd.length === 0)
) {
return task.threadIds;
}
return null;
}

_optimisticallyRemoveThreads(threadIds: string[]) {
Comment thread
indent-staging[bot] marked this conversation as resolved.
Outdated
for (const key of Object.keys(this._subscriptions)) {
const subscription = this._subscriptions[key];
if (subscription._query && subscription._query.objectClass() === 'Thread') {
subscription.optimisticallyRemoveItemsById(threadIds);
}
}
}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This is super interesting - If the issue happens specifically with search, it'd be nice to move this logic down into the SearchQuerySubscription subclass, because we use this QuerySubscription class a lot, and for other model types besides threads. I think that the problem may actually be that the SearchQuerySubscription doesn't refresh like others, but I'm not 100% sure? It's been many years :-)

}

const pool = new QuerySubscriptionPool();
Expand Down
27 changes: 22 additions & 5 deletions app/src/flux/models/query-subscription.ts
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ export class QuerySubscription<T extends Model> {
`QuerySubscription:removeCallback - expects a function, received ${callback}`
);
}
this._callbacks = this._callbacks.filter(c => c !== callback);
this._callbacks = this._callbacks.filter((c) => c !== callback);
if (this.callbackCount() === 0) {
this.onLastCallbackRemoved();
}
Expand Down Expand Up @@ -121,7 +121,7 @@ export class QuerySubscription<T extends Model> {
let knownImpacts = 0;
let unknownImpacts = 0;

this._queuedChangeRecords.forEach(record => {
this._queuedChangeRecords.forEach((record) => {
if (record.type === 'unpersist') {
for (const item of record.objects) {
const offset = this._set.offsetOfId(item.id);
Expand Down Expand Up @@ -248,7 +248,7 @@ export class QuerySubscription<T extends Model> {
rangeQuery.background();
}

DatabaseStore.run<T[] | string[]>(rangeQuery, { format: false }).then(async results => {
DatabaseStore.run<T[] | string[]>(rangeQuery, { format: false }).then(async (results) => {
if (this._queryVersion !== version) {
return;
}
Expand Down Expand Up @@ -279,13 +279,30 @@ export class QuerySubscription<T extends Model> {
}

async _fetchMissingModels() {
const missingIds = this._set.ids().filter(id => !this._set.modelWithId(id));
const missingIds = this._set.ids().filter((id) => !this._set.modelWithId(id));
if (missingIds.length === 0) {
return [];
}
return DatabaseStore.findAll<T>(this._query._klass, { id: missingIds });
}

optimisticallyRemoveItemsById = (ids: string[]) => {
if (!this._set) {
return;
}
let removed = 0;
for (const id of ids) {
const offset = this._set.offsetOfId(id);
if (offset !== -1) {
this._set.removeModelAtOffset({ id } as T, offset);
removed += 1;
}
}
if (removed > 0) {
this._createResultAndTrigger();
}
};

_createResultAndTrigger() {
const allCompleteModels = this._set.isComplete();

Expand Down Expand Up @@ -319,7 +336,7 @@ export class QuerySubscription<T extends Model> {
this._lastResult = this._query.formatResult(models) as QuerySubscriptionResult<T>;
}

this._callbacks.forEach(callback => callback(this._lastResult));
this._callbacks.forEach((callback) => callback(this._lastResult));

// process any additional change records that have arrived
if (this._updateInFlight) {
Expand Down