Getting model from widget first than fallback to documentManager

[nakul-py]

First try to getting model from widget than fallback to documentManager for both files and notebook cells.

file_widget.mp4

cell_widget.mp4

• Closes Get the model from the widget, instead of the documentManager #234

cc @jtpio @brichet

[brichet]

Thanks @nakul-py for working in this.

After a quick test, it works well on my side with cell but with a notebook I have something like

**File: test.ipynb**```json[object Object]```

Can you use the types of objects instead of any ? That would probably help preventing unexpected errors, and also simplify review.

[nakul-py]

@brichet I have used all the needed object types but still i get Untitled1.ipynb, but the content attached seems to be a placeholder [object Object]. from llm. Now how can i tackle this out. I share the updated code below. :) Need your help on this!

Here is the code

 private async _readFileAttachment(
    attachment: IAttachment
  ): Promise<string | null> {
    // Handle both 'file' and 'notebook' types since both have a 'value' path
    if (attachment.type !== 'file' && attachment.type !== 'notebook') {
      return null;
    }

    try {
      // Try reading from an open widget first
      const widget = this.input.documentManager?.findWidget(attachment.value);

      if (widget && widget.context && widget.context.model) {
        const model = widget.context.model as DocumentRegistry.IModel;

        const sharedText = (model as any).sharedModel as
          | ISharedText
          | undefined;
        if (sharedText && typeof sharedText.getSource === 'function') {
          return sharedText.getSource();
        }

        const sharedNb = (model as any).sharedModel as
          | ISharedNotebook
          | undefined;
        if (sharedNb && typeof sharedNb.getCell === 'function') {
          const cells: nbformat.ICell[] = [];
          for (let i = 0; i < sharedNb.cells.length; i++) {
            const cell = sharedNb.getCell(i);
            const cellJSON = cell.toJSON();

            cells.push({
              ...cellJSON,
              outputs: [],
              execution_count: null
            });
          }

          const nb: nbformat.INotebookContent = {
            cells,
            metadata: sharedNb.getMetadata(),
            nbformat: 4,
            nbformat_minor: 5
          };

          return JSON.stringify(nb);
        }
      }

      // If not open, load from disk
      const diskModel = await this.input.documentManager?.services.contents.get(
        attachment.value
      );

      if (!diskModel?.content) {
        return null;
      }

      if (diskModel.type === 'file') {
        // Regular file content
        return diskModel.content;
      }

      if (diskModel.type === 'notebook') {
        const cleaned = {
          ...diskModel,
          cells: diskModel.content.cells.map((cell: any) => ({
            ...cell,
            outputs: [],
            execution_count: null
          }))
        };

        return JSON.stringify(cleaned);
      }
      return null;
    } catch (error) {
      console.warn(`Failed to read file ${attachment.value}:`, error);
      return null;
    }
  }

[nakul-py]

Hey @brichet!

In this commit 5521434, I fixed the Untitled1.ipynb, is [object Object]. error by ensuring that sharedModel.getSource() always returns a string. If it returns an object, I now convert it into JSON using JSON.stringify(). This prevents the LLM from receiving [object Object].

[brichet]

As far as I know, the widget can only be a IDocumentWidget<Notebook, INotebookModel> | undefined.
Casting it would avoid typing the following sharedModel and cells as any.
Did you see use case where the widget is of another type ?

[brichet]

I wonder if we reach this comparison ?
As in the comment above, the widget should only be a IDocumentWidget<Notebook, INotebookModel> | undefined, so the model should be INotebookModel, with sharedModel.getSource always defined (and the previous comparison always return the source).

[brichet]

Thanks @nakul-py for updating the PR 👍

I have some nitpick comments

[brichet]

Suggested change

	let cellData: nbformat.ICell[] | null = null;
	let cellData: nbformat.ICell[];

[brichet]

Suggested change

	cellData = model.content.cells;
	cellData = model.content.cells ?? [];

Related to above comment

[brichet]

Suggested change

	const cell = cellData!.find(c => c.id === cellInfo.id);
	const cell = cellData.find(c => c.id === cellInfo.id);

Related to above comments

[brichet]

Suggested change

	const ymodel = widget?.context?.model?.sharedModel as YNotebook;
	const ymodel = widget?.context.model.sharedModel as YNotebook;

[brichet]

Suggested change

	nb.metadata?.language_info?.name ||
	nb.metadata?.kernelspec?.language ||
	nb.metadata.language_info?.name ||
	nb.metadata.kernelspec?.language ||

[brichet]

Can we reach this comparison ?
The YNotebook always has a getSource() function defined, AFAIK https://github.com/jupyter-server/jupyter_ydoc/blob/fdbf658e072467f49048ddef513a5e569207f25b/javascript/src/ynotebook.ts#L408

[nakul-py]

Yes you are correct Nicolas!. This comparison is unreachable as you say earlier YNotebook always implements getSource(), and getSource() internally calls toJSON().

[nakul-py]

@brichet Now everything looks fine. 👍

[brichet]

Thanks @nakul-py, looks great!