Skip to content

Commit

Permalink
[FIX] collaborative: snapshot when leaving spreadsheet
Browse files Browse the repository at this point in the history
Purpose
-------

Let's say a user works a lot on a spreadsheet and performs a lot of heavy
operations generating very large revisions (with lots of commands), such as
copy-pasting a very large zone (there's one UPDATE_CELL command per copy-pasted
cell)

If the user does multiple such very large revision, one after the other,
every thing is fine client side.

The next time the spreadsheet is open though: the server needs to load all
those revisions to send them to the client. The revisions can be so large
it can blow up the server memory limit.
The spreadsheet cannot be open anymore.

Solution
--------

The solution proposed here is to snapshot the spreadsheet when the client
leaves the spreadsheet. Next time the spreadsheet is open, the snapshot will
be loaded, and not the revivisions.

There's a catch: snapshotting a spreadsheet kills the local history (CTRL+Z).
For this reason, we only snapshot if there's no other connected client as it
would kill the other users history.

closes #4334

Task: 3940465
Signed-off-by: Pierre Rousseau (pro) <pro@odoo.com>
  • Loading branch information
LucasLefevre committed Jun 10, 2024
1 parent 1f01394 commit 7bcff34
Show file tree
Hide file tree
Showing 3 changed files with 60 additions and 5 deletions.
5 changes: 4 additions & 1 deletion src/collaborative/session.ts
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,10 @@ export class Session extends EventBus<CollaborativeEvent> {
/**
* Notify the server that the user client left the collaborative session
*/
leave() {
leave(data: WorkbookData) {
if (Object.keys(this.clients).length === 1 && this.processedRevisions.size) {
this.snapshot(data);
}
delete this.clients[this.clientId];
this.transportService.leave(this.clientId);
this.transportService.sendMessage({
Expand Down
2 changes: 1 addition & 1 deletion src/model.ts
Original file line number Diff line number Diff line change
Expand Up @@ -288,7 +288,7 @@ export class Model extends EventBus<any> implements CommandDispatcher {
}

leaveSession() {
this.session.leave();
this.session.leave(this.exportData());
}

private setupUiPlugin(Plugin: UIPluginConstructor) {
Expand Down
58 changes: 55 additions & 3 deletions tests/collaborative/collaborative_session.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { Model } from "../../src";
import { Session } from "../../src/collaborative/session";
import { DEBOUNCE_TIME, MESSAGE_VERSION } from "../../src/constants";
import { buildRevisionLog } from "../../src/history/factory";
import { Client, CommandResult } from "../../src/types";
import { Client, CommandResult, WorkbookData } from "../../src/types";
import { MockTransportService } from "../__mocks__/transport_service";
import { selectCell } from "../test_helpers/commands_helpers";

Expand Down Expand Up @@ -50,7 +50,8 @@ describe("Collaborative session", () => {

test("local client leaves", () => {
const spy = jest.spyOn(transport, "sendMessage");
session.leave();
session.leave({} as WorkbookData);
expect(spy).toHaveBeenCalledTimes(1);
expect(spy).toHaveBeenCalledWith({
type: "CLIENT_LEFT",
version: MESSAGE_VERSION,
Expand All @@ -59,6 +60,57 @@ describe("Collaborative session", () => {
expect(session.getConnectedClients()).toEqual(new Set());
});

test("local client leaves with no other clients and changes", () => {
transport.sendMessage({
type: "REMOTE_REVISION",
version: MESSAGE_VERSION,
nextRevisionId: "42",
clientId: "client_42",
commands: [],
serverRevisionId: transport["serverRevisionId"],
});
const spy = jest.spyOn(transport, "sendMessage");
const data = { sheets: [{}] } as WorkbookData;
session.leave(data);
expect(spy).toHaveBeenCalledWith({
type: "SNAPSHOT",
version: MESSAGE_VERSION,
nextRevisionId: expect.any(String),
serverRevisionId: "42",
data: { ...data, revisionId: expect.any(String) },
});
});

test("local client leaves with other connected clients and changes", () => {
transport.sendMessage({
type: "CLIENT_JOINED",
version: MESSAGE_VERSION,
client: {
id: "bob",
name: "Bob",
position: { sheetId: "sheet1", col: 0, row: 0 },
},
});
expect(session.getConnectedClients().size).toBe(2);
transport.sendMessage({
type: "REMOTE_REVISION",
version: MESSAGE_VERSION,
nextRevisionId: "42",
clientId: "client_42",
commands: [],
serverRevisionId: transport["serverRevisionId"],
});
const spy = jest.spyOn(transport, "sendMessage");
const data = { sheets: [{}] } as WorkbookData;
session.leave(data);
expect(spy).toHaveBeenCalledTimes(1);
expect(spy).toHaveBeenCalledWith({
type: "CLIENT_LEFT",
version: MESSAGE_VERSION,
clientId: client.id,
});
});

test("remote client move", () => {
transport.sendMessage({
type: "CLIENT_MOVED",
Expand Down Expand Up @@ -147,7 +199,7 @@ describe("Collaborative session", () => {

test("Leave the session do not crash", () => {
session.move({ sheetId: "sheetId", col: 1, row: 2 });
session.leave();
session.leave({} as WorkbookData);
jest.advanceTimersByTime(DEBOUNCE_TIME + 100);
});

Expand Down

0 comments on commit 7bcff34

Please sign in to comment.