mirror of
https://github.com/Foundry376/Mailspring.git
synced 2024-09-22 08:16:09 +08:00
[client-app] Don't show non-existent children on folder creation
Summary: Previously, after creating a new folder, the UI would indicate that the new folder had children, even though it didn't. This was caused by duplicate models in our `MutableQueryResultSet` for the user's categories. Basically, we would sync the server version of the folder before the `SyncbackTask` for the new folder returned its `serverId`. Without the `serverId`, the synced version of the folder couldn't yet be tied to the optimistic folder, so a second row was created in the database. This second row is removed when the `syncbackTask` does return the `serverId`, because we persist the optimistic folder with a `REPLACE INTO` query. (This deletes other rows with the same id.) However, since this was done inside a `persist` change with the `serverId` and no `unpersist` was ever recorded for the `clientId`, our `MutableQueryResultSet` never removed the `clientId` model. To address this, this diff adds a check in `updateModel` to see if the `serverId` is being added. If it is, and both the `serverId` and `clientId` exist in the `_ids` list, we remove the `clientId`. The children indicator does still briefly show up while there are still two separate rows for that folder in the database. If we want to get rid of this completely, we would have to ensure that we do not sync the folder before the `syncbackTask` returns the `serverId`. However, this would probably be pretty involved, and for not much gain. This fix is much simpler and reduces most of the issue. Test Plan: manual Reviewers: juan, evan Reviewed By: evan Differential Revision: https://phab.nylas.com/D4228
This commit is contained in:
parent
4492b73507
commit
73e990fc05
|
@ -153,4 +153,65 @@ describe("MutableQueryResultSet", function MutableQueryResultSetSpecs() {
|
|||
});
|
||||
});
|
||||
});
|
||||
|
||||
describe('updateModel', () => {
|
||||
beforeEach(() => {
|
||||
this.mockModel = (clientId, serverId) => {
|
||||
return {
|
||||
id: serverId || clientId,
|
||||
clientId: clientId,
|
||||
serverId: serverId,
|
||||
constructor: {
|
||||
attributes: [],
|
||||
},
|
||||
}
|
||||
}
|
||||
})
|
||||
|
||||
/*
|
||||
Previously, after creating a new folder, the UI would indicate that the new
|
||||
folder had children, even though it didn't. This was caused by duplicate models
|
||||
in our MutableQueryResultSet for the user's categories. Basically, we would
|
||||
sync the server version of the folder before the SyncbackTask for the new
|
||||
folder returned its serverId. Without the serverId, the synced version of
|
||||
the folder couldn't yet be tied to the optimistic folder, so a second row was
|
||||
created in the database. This second row is removed when the syncbackTask
|
||||
does return the serverId, because we persist the optimistic folder with a
|
||||
'REPLACE INTO' query. (This deletes other rows with the same id.) However,
|
||||
since this was done inside a 'persist' change with the serverId and no
|
||||
'unpersist' was ever recorded for the clientId, our MutableQueryResultSet
|
||||
never removed the clientId model. We now detect this case within
|
||||
updateModel and remove the clientId when necessary.
|
||||
*/
|
||||
describe('when the model is an optimistic copy receiving its serverId', () => {
|
||||
it('removes the clientId if both the clientId and serverId are listed', () => {
|
||||
const set = new MutableQueryResultSet({
|
||||
_ids: ['clientId1', 'serverId1', 'serverId2', 'serverId3'],
|
||||
_modelsHash: {
|
||||
'clientId1': this.mockModel('clientId1'),
|
||||
'serverId2': this.mockModel('clientId2', 'serverId2'),
|
||||
'serverId3': this.mockModel('clientId3', 'serverId3'),
|
||||
'serverId1': this.mockModel('clientId4', 'serverId1'),
|
||||
},
|
||||
});
|
||||
|
||||
set.updateModel(this.mockModel('clientId1', 'serverId1'))
|
||||
expect(set.ids().includes('clientId1')).toEqual(false)
|
||||
})
|
||||
|
||||
it('does not remove the clientId if the serverId is not listed', () => {
|
||||
const set = new MutableQueryResultSet({
|
||||
_ids: ['clientId1', 'serverId2', 'serverId3'],
|
||||
_modelsHash: {
|
||||
'clientId1': this.mockModel('clientId1'),
|
||||
'serverId2': this.mockModel('clientId2', 'serverId2'),
|
||||
'serverId3': this.mockModel('clientId3', 'serverId3'),
|
||||
},
|
||||
});
|
||||
|
||||
set.updateModel(this.mockModel('clientId1', 'serverId1'))
|
||||
expect(set.ids().includes('clientId1')).toEqual(true)
|
||||
})
|
||||
})
|
||||
});
|
||||
});
|
||||
|
|
|
@ -91,7 +91,24 @@ export default class MutableQueryResultSet extends QueryResultSet {
|
|||
item[attr.modelKey] = existing[attr.modelKey];
|
||||
}
|
||||
}
|
||||
|
||||
// There can briefly be two rows in the database that are actually the
|
||||
// same item (due to optimistic client-side updates). When these rows are
|
||||
// merged together, _ids is not properly updated because there is never
|
||||
// an 'unpersist' change record. We attempt to catch this scenario here.
|
||||
// First check that the existing item was optimistic (no serverId) and the
|
||||
// new item is not.
|
||||
if (!existing.serverId && item.serverId) {
|
||||
// Now check that _ids does in fact have both IDs.
|
||||
const clientIndex = this._ids.indexOf(existing.clientId)
|
||||
const serverIndex = this._ids.indexOf(item.serverId)
|
||||
if (clientIndex > -1 && serverIndex > -1) {
|
||||
// Remove the clientID
|
||||
this._ids.splice(clientIndex, 1)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
this._modelsHash[item.clientId] = item;
|
||||
this._modelsHash[item.id] = item;
|
||||
this._idToIndexHash = null;
|
||||
|
|
Loading…
Reference in a new issue