Skip to content

Commit

Permalink
Fix spurious "File Changed" dialog in JupyterLab 4.1+/Notebook 7.1+ (#…
Browse files Browse the repository at this point in the history
…337)

* Save current hash on the document

* Use public API now that PR 262 in `jupyter_ydoc` is merged

* Use a clean solution which requires a change in JupyterLab

* `jupyter-server-ydoc`: require `jupyter-server` 2.11.1 or newer

As only 2.11.1 added `require_hash` argument
  • Loading branch information
krassowski authored Aug 22, 2024
1 parent 7ea2cd5 commit 1b74c43
Show file tree
Hide file tree
Showing 4 changed files with 66 additions and 6 deletions.
46 changes: 45 additions & 1 deletion packages/docprovider/src/ydrive.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import { PageConfig, URLExt } from '@jupyterlab/coreutils';
import { TranslationBundle } from '@jupyterlab/translation';
import { Contents, Drive, User } from '@jupyterlab/services';
import { ISignal, Signal } from '@lumino/signaling';

import { DocumentChange, ISharedDocument, YDocument } from '@jupyter/ydoc';

Expand Down Expand Up @@ -45,6 +46,10 @@ export class YDrive extends Drive implements ICollaborativeDrive {
this._providers = new Map<string, WebSocketProvider>();

this.sharedModelFactory = new SharedModelFactory(this._onCreate);
super.fileChanged.connect((_, change) => {
// pass through any events from the Drive superclass
this._ydriveFileChanged.emit(change);
});
}

/**
Expand Down Expand Up @@ -84,7 +89,7 @@ export class YDrive extends Drive implements ICollaborativeDrive {
const provider = this._providers.get(key);

if (provider) {
// If the document does't exist, `super.get` will reject with an
// If the document doesn't exist, `super.get` will reject with an
// error and the provider will never be resolved.
// Use `Promise.all` to reject as soon as possible. The Context will
// show a dialog to the user.
Expand Down Expand Up @@ -132,6 +137,13 @@ export class YDrive extends Drive implements ICollaborativeDrive {
return super.save(localPath, options);
}

/**
* A signal emitted when a file operation takes place.
*/
get fileChanged(): ISignal<this, Contents.IChangedArgs> {
return this._ydriveFileChanged;
}

private _onCreate = (
options: Contents.ISharedFactoryOptions,
sharedModel: YDocument<DocumentChange>
Expand Down Expand Up @@ -161,6 +173,37 @@ export class YDrive extends Drive implements ICollaborativeDrive {
const key = `${options.format}:${options.contentType}:${options.path}`;
this._providers.set(key, provider);

sharedModel.changed.connect(async (_, change) => {
if (!change.stateChange) {
return;
}
const hashChanges = change.stateChange.filter(
change => change.name === 'hash'
);
if (hashChanges.length === 0) {
return;
}
if (hashChanges.length > 1) {
console.error(
'Unexpected multiple changes to hash value in a single transaction'
);
}
const hashChange = hashChanges[0];

// A change in hash signifies that a save occurred on the server-side
// (e.g. a collaborator performed the save) - we want to notify the
// observers about this change so that they can store the new hash value.
const model = await this.get(options.path, { content: false });

this._ydriveFileChanged.emit({
type: 'save',
newValue: { ...model, hash: hashChange.newValue },
// we do not have the old model because it was discarded when server made the change,
// we only have the old hash here (which may be empty if the file was newly created!)
oldValue: { hash: hashChange.oldValue }
});
});

sharedModel.disposed.connect(() => {
const provider = this._providers.get(key);
if (provider) {
Expand Down Expand Up @@ -190,6 +233,7 @@ export class YDrive extends Drive implements ICollaborativeDrive {
private _trans: TranslationBundle;
private _providers: Map<string, WebSocketProvider>;
private _globalAwareness: Awareness | null;
private _ydriveFileChanged = new Signal<this, Contents.IChangedArgs>(this);
}

/**
Expand Down
20 changes: 17 additions & 3 deletions projects/jupyter-server-ydoc/jupyter_server_ydoc/loaders.py
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ async def load_content(self, format: str, file_type: str) -> dict[str, Any]:
self.last_modified = model["last_modified"]
return model

async def maybe_save_content(self, model: dict[str, Any]) -> None:
async def maybe_save_content(self, model: dict[str, Any]) -> dict[str, Any] | None:
"""
Save the content of the file.
Expand Down Expand Up @@ -149,20 +149,34 @@ async def maybe_save_content(self, model: dict[str, Any]) -> None:
# otherwise it could corrupt the file
done_saving = asyncio.Event()
task = asyncio.create_task(self._save_content(model, done_saving))
saved_model = None
try:
await asyncio.shield(task)
saved_model = await asyncio.shield(task)
except asyncio.CancelledError:
pass
await done_saving.wait()
return saved_model
else:
# file changed on disk, raise an error
self.last_modified = m["last_modified"]
raise OutOfBandChanges

async def _save_content(self, model: dict[str, Any], done_saving: asyncio.Event) -> None:
async def _save_content(
self, model: dict[str, Any], done_saving: asyncio.Event
) -> dict[str, Any]:
try:
m = await ensure_async(self._contents_manager.save(model, self.path))
self.last_modified = m["last_modified"]
# TODO, get rid of the extra `get` here once upstream issue:
# https://github.com/jupyter-server/jupyter_server/issues/1453 is resolved
model_with_hash = await ensure_async(
self._contents_manager.get(
self.path,
content=False,
require_hash=True,
)
)
return {**m, "hash": model_with_hash["hash"]}
finally:
done_saving.set()

Expand Down
4 changes: 3 additions & 1 deletion projects/jupyter-server-ydoc/jupyter_server_ydoc/rooms.py
Original file line number Diff line number Diff line change
Expand Up @@ -271,7 +271,7 @@ async def _maybe_save_document(self, saving_document: asyncio.Task | None) -> No
await asyncio.sleep(self._save_delay)

self.log.info("Saving the content from room %s", self._room_id)
await self._file.maybe_save_content(
saved_model = await self._file.maybe_save_content(
{
"format": self._file_format,
"type": self._file_type,
Expand All @@ -280,6 +280,8 @@ async def _maybe_save_document(self, saving_document: asyncio.Task | None) -> No
)
async with self._update_lock:
self._document.dirty = False
if saved_model:
self._document.hash = saved_model["hash"]

self._emit(LogLevel.INFO, "save", "Content saved.")

Expand Down
2 changes: 1 addition & 1 deletion projects/jupyter-server-ydoc/pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ authors = [
{ name = "Jupyter Development Team", email = "[email protected]" },
]
dependencies = [
"jupyter_server>=2.4.0,<3.0.0",
"jupyter_server>=2.11.1,<3.0.0",
"jupyter_ydoc>=2.0.0,<4.0.0",
"pycrdt",
"pycrdt-websocket>=0.14.0,<0.15.0",
Expand Down

0 comments on commit 1b74c43

Please sign in to comment.