Skip to content
20 changes: 15 additions & 5 deletions packages/docprovider-extension/src/filebrowser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -55,12 +55,13 @@ export const rtcContentProvider: JupyterFrontEndPlugin<ICollaborativeContentProv
description: 'The RTC content provider',
provides: ICollaborativeContentProvider,
requires: [ITranslator],
optional: [IGlobalAwareness],
activate: (
optional: [IGlobalAwareness, ISettingRegistry],
activate: async (
app: JupyterFrontEnd,
translator: ITranslator,
globalAwareness: Awareness | null
): ICollaborativeContentProvider => {
globalAwareness: Awareness | null,
settingRegistry: ISettingRegistry | null
): Promise<ICollaborativeContentProvider> => {
const trans = translator.load('jupyter_collaboration');
const defaultDrive = (app.serviceManager.contents as ContentsManager)
.defaultDrive;
Expand All @@ -75,12 +76,21 @@ export const rtcContentProvider: JupyterFrontEndPlugin<ICollaborativeContentProv
'Cannot initialize content provider: no content provider registry.'
);
}
const docmanagerSettings = settingRegistry
? await settingRegistry.load('@jupyterlab/docmanager-extension:plugin')
: null;

// Force autosave to be true by default initially
if (docmanagerSettings) {
void docmanagerSettings.set('autosave', true);
}
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't this change user's autosave setting? I think we could just take the value as-is because autosave is the default in lab and notebook.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, if autosave is set to true by default, then we don't need this.
Let me go ahead and remove it.

const rtcContentProvider = new RtcContentProvider({
apiEndpoint: '/api/contents',
serverSettings: defaultDrive.serverSettings,
user: app.serviceManager.user,
trans,
globalAwareness
globalAwareness,
docmanagerSettings
});
registry.register('rtc', rtcContentProvider);
return rtcContentProvider;
Expand Down
19 changes: 18 additions & 1 deletion packages/docprovider/src/ydrive.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import {
ISharedModelFactory
} from '@jupyter/collaborative-drive';
import { Awareness } from 'y-protocols/awareness';
import { ISettingRegistry } from '@jupyterlab/settingregistry';

const DISABLE_RTC =
PageConfig.getOption('disableRTC') === 'true' ? true : false;
Expand All @@ -42,6 +43,7 @@ namespace RtcContentProvider {
user: User.IManager;
trans: TranslationBundle;
globalAwareness: Awareness | null;
docmanagerSettings: ISettingRegistry.ISettings | null;
}
}

Expand All @@ -57,6 +59,7 @@ export class RtcContentProvider
this._serverSettings = options.serverSettings;
this.sharedModelFactory = new SharedModelFactory(this._onCreate);
this._providers = new Map<string, WebSocketProvider>();
this._docmanagerSettings = options.docmanagerSettings;
}

/**
Expand Down Expand Up @@ -123,7 +126,7 @@ export class RtcContentProvider
const provider = this._providers.get(key);

if (provider) {
// Save is done from the backend
provider.wsProvider?.ws?.send('save_to_disc');
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that we should use a custom message type (see here). Maybe 2 followed by save?
Also, should we wait for a reply indicating that the file has indeed been saved? Otherwise the following get will probably not return the state of the saved file.

Copy link
Member Author

Choose a reason for hiding this comment

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

Otherwise the following get will probably not return the state of the saved file.

True, But since the signal below is fired after each save from server (due to hash change) and the contents model is automatically updated with the new values, it may not be necessary to wait for the reply here to update the contents model.

this._ydriveFileChanged.emit({
type: 'save',
newValue: { ...model, hash: hashChange.newValue },

const fetchOptions: Contents.IFetchOptions = {
type: options.type,
format: options.format,
Expand All @@ -150,6 +153,19 @@ export class RtcContentProvider
if (typeof options.format !== 'string') {
return;
}
// Set initial autosave value, used to determine backend autosave (default: true)
const autosave =
(this._docmanagerSettings?.composite?.['autosave'] as boolean) ?? true;

sharedModel.awareness.setLocalStateField('autosave', autosave);
Copy link
Member

Choose a reason for hiding this comment

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

Should this also include autosaveInterval?

Copy link
Member Author

@Darshan808 Darshan808 May 13, 2025

Choose a reason for hiding this comment

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

Do you think we should also send the autosaveInterval to the backend?

The reason I ask is that currently, JupyterLab (frontend) automatically calls save after every autosaveInterval, which in turn triggers save_to_disc on the backend.
However, the current implementation is that when autosave is enabled, the backend saves the document to disk on every document change, regardless of the configured autosave interval.

If we want to respect the autosave interval properly, I think we could consider removing the _on_document_change function on the backend.

def _on_document_change(self, target: str, event: Any) -> None:
"""
Called when the shared document changes.

Here's why:

  • When autosave is off, only manual saves are possible, so observing document changes for saving might not be necessary.
  • When autosave is on, the client will already call save at the configured interval, which will trigger save_to_disc. Thus, observing document changes on the backend may also be redundant in this case.

One potential caveat is that if multiple clients with different autosaveInterval values are connected to the same document, save_to_disc will still be called for each of them when their individual autosave timers trigger.

Would love to hear your thoughts on this approach and whether you see any concerns or alternatives.

CC: @davidbrochart

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 move the discussion to a new issue and address this in a separate PR?

Copy link
Member Author

@Darshan808 Darshan808 May 13, 2025

Choose a reason for hiding this comment

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

I think that is reasonable. We could create a new issue after this PR gets merged.


// Watch for changes in settings
this._docmanagerSettings?.changed.connect(() => {
const newAutosave =
(this._docmanagerSettings?.composite?.['autosave'] as boolean) ?? true;
sharedModel.awareness.setLocalStateField('autosave', newAutosave);
});

try {
const provider = new WebSocketProvider({
url: URLExt.join(this._serverSettings.wsUrl, DOCUMENT_PROVIDER_URL),
Expand Down Expand Up @@ -235,6 +251,7 @@ export class RtcContentProvider
private _providers: Map<string, WebSocketProvider>;
private _ydriveFileChanged = new Signal<this, Contents.IChangedArgs>(this);
private _serverSettings: ServerConnection.ISettings;
private _docmanagerSettings: ISettingRegistry.ISettings | null;
}

/**
Expand Down
11 changes: 9 additions & 2 deletions projects/jupyter-server-ydoc/jupyter_server_ydoc/handlers.py
Original file line number Diff line number Diff line change
Expand Up @@ -290,8 +290,15 @@ async def on_message(self, message):
"""
On message receive.
"""
message_type = message[0]
if message == "save_to_disc":
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that we should use a custom message type (see here). Maybe 2 followed by save?

try:
self.room._saving_document = asyncio.create_task(
self.room._maybe_save_document(self.room._saving_document)
)
except Exception:
self.log.error("Couldn't save content from room: %s", self._room_id)

message_type = message[0]
if message_type == MessageType.CHAT:
msg = message[2:].decode("utf-8")

Expand Down Expand Up @@ -321,7 +328,7 @@ def on_close(self) -> None:
"""
# stop serving this client
self._message_queue.put_nowait(b"")
if isinstance(self.room, DocumentRoom) and self.room.clients == [self]:
if isinstance(self.room, DocumentRoom) and self.room.clients == {self}:
# no client in this room after we disconnect
# keep the document for a while in case someone reconnects
self.log.info("Cleaning room: %s", self._room_id)
Expand Down
13 changes: 12 additions & 1 deletion projects/jupyter-server-ydoc/jupyter_server_ydoc/rooms.py
Original file line number Diff line number Diff line change
Expand Up @@ -247,6 +247,18 @@ def _on_document_change(self, target: str, event: Any) -> None:
document. This tasks are debounced (60 seconds by default) so we
need to cancel previous tasks before creating a new one.
"""
# Collect autosave values from all clients
autosave_states = [
state.get("autosave", True)
for state in self.awareness.states.values()
if state # skip empty states
]

# Enable autosave if at least one client has it turned on
autosave = any(autosave_states)

if not autosave:
return
if self._update_lock.locked():
return

Expand All @@ -265,7 +277,6 @@ async def _maybe_save_document(self, saving_document: asyncio.Task | None) -> No
"""
if self._save_delay is None:
return

if saving_document is not None and not saving_document.done():
# the document is being saved, cancel that
saving_document.cancel()
Expand Down