Tickets/dm 46955

[athornton]

No description provided.

[athornton]

Run on IDF as Experimental Weekly 2024_47_tut_exp

I've left an awful lot of console logging in here that I will delete before I merge, but you may find it helpful if you play with the extension in the RSP, because it lets you trace the construction of the hierarchy and the menu from it.

[athornton]

Now I don't know what to do. jlpm lint:check runs fine locally. I installed on Amazon Linux, and it showed me that some lines were too long. I don't know why MacOS and Linux differ in this regard.

[stvoutsin]

Looks great and also looks like a lot of work!
I've only reviewed the Typescript side, and only have some small suggestions based on my somewhat limited JS/Typescript experience so take with a grain of salt

[stvoutsin]

Suggested change

	"""Rubin query parms."""
	"""Rubin query params."""

[stvoutsin]

Since there is a lot of logging which you may or may not want to display depending on if you are in prod or dev, you could setup a custom logger (or reuse something already out there like Pino).

Example:

function logMessage(level: 'info' | 'warn' | 'error', message: string, ...optionalParams: any[]): void {
  const logLevel = process.env.LOG_LEVEL || 'info';

  const levels = {
    info: 1,
    warn: 2,
    error: 3,
  };

  if (levels[level] >= levels[logLevel]) {
    if (level === 'info') {
      console.info(message, ...optionalParams);
    } else if (level === 'warn') {
      console.warn(message, ...optionalParams);
    } else if (level === 'error') {
      console.error(message, ...optionalParams);
    }
  }
}

Which you could then use as:

logMessage('info', 'Building hierarchy ${name}');

[stvoutsin]

Also just noticed your message that logging would be removed so you may want to skip this, although perhaps you may still want to leave some logging in there that you can turn it off/on via an env var?

May be useful in case things break and you won't have to add logging again from the start.

[athornton]

Yeah, sort of. But I don't have process.env in a useful fashion where the UI is running. But that's why I have the environment extension...but I need to refactor the loading so I can load the environment before initializing the other extensions, and then figure out a way to stash it so they can get to it.

That feels like the big tech-debt refactor I'm putting off, though. For the time being I'll probably get it debugged and then throw away a bunch of the console.log statements.

[stvoutsin]

Suggested change

	function overwriteDialog(
	dest: string,
	manager: IDocumentManager
	): Promise<any> {
	const options = {
	title: 'Target file exists',
	body: `Overwrite file '${dest}' ?`,
	buttons: [Dialog.cancelButton(), Dialog.okButton({ label: 'OVERWRITE' })]
	};
	console.log('Showing overwrite dialog');
	return showDialog(options).then(result => {
	if (!result) {
	console.log('No result from queryDialog');
	return new Promise((res, rej) => {
	/* Nothing */
	});
	}
	console.log('Result from overwriteDialog: ', result);
	if (!result.button) {
	console.log('No result.button from overwriteDialog');
	return new Promise((res, rej) => {
	/* Nothing */
	});
	}
	if (result.button.label === 'OVERWRITE') {
	console.log(
	'Got result ',
	result.button.label,
	' from overwriteDialog: OVERWRITE'
	);
	return Promise.resolve(result.button.label);
	}
	console.log('Did not get overwriteDialog: OVERWRITE');
	return new Promise((res, rej) => {
	/* Nothing */
	});
	});
	}
	interface DialogResult {
	button?: {
	label: string;
	};
	}
	async function overwriteDialog(dest: string, manager: IDocumentManager): Promise<string | void> {
	const dialogOptions = {
	title: 'Target file exists',
	body: `Overwrite file '${dest}' ?`,
	buttons: [
	Dialog.cancelButton(),
	Dialog.okButton({ label: 'OVERWRITE' })
	]
	};
	try {
	console.log('Showing overwrite dialog');
	const result: DialogResult = await showDialog(dialogOptions);
	if (!result) {
	console.log('No result from queryDialog');
	return;
	}
	console.log('Result from overwriteDialog: ', result);
	if (!result.button) {
	console.log('No result.button from overwriteDialog');
	return;
	}
	if (result.button.label === 'OVERWRITE') {
	console.log(
	'Got result ',
	result.button.label,
	' from overwriteDialog: OVERWRITE'
	);
	return result.button.label;
	}
	console.log('Did not get overwriteDialog: OVERWRITE');
	return;
	} catch (error) {
	console.error('Error showing overwrite dialog:', error);
	throw new Error(`Failed to show overwrite dialog: ${error.message}`);
	}
	}

[stvoutsin]

I think simply returning (void) may be better here then promises that don't resolve.
You could also use Promise.resolve() or even a Promise.reject() if you want your function to return a promise, but I don't think it's needed?

Note: I have not tested this

[stvoutsin]

I think debug may be better for these types of messages, which some browser will hide in the console by default

[stvoutsin]

It doesn't look like ABORT is used atm, but may be in future versions in which case it's fine?

[athornton]

Yeah, at the moment there's no notion of a server-side cleanup if the user cancels, but there could be. I think in the current design we don't persist anything into the user's file space unless we have confirmed (or confirmation wasn't needed). But I can see that changing, especially as I figure out the ancillary-file stuff.

[stvoutsin]

Suggested change

	console.log('Created TutorialsHierary from response');
	console.log('Created TutorialsHierarchy from response');

[stvoutsin]

Suggested change

	function apiPostTutorialsEntry(
	settings: ServerConnection.ISettings,
	docManager: IDocumentManager,
	entry: TutorialsEntry
	): void {
	/**
	* Make a request to our endpoint to copy a file into place and open it
	*
	* @param settings - the settings for the current notebook server
	*
	* @param entry - the entry corresponding to the file to work with
	*
	* @returns a Promise resolved with the JSON response
	*/
	/**
	* Make a request to our endpoint to copy a file into place and open it.
	* Handles file overwrite confirmation if needed and opens the file on success.
	*
	* @param settings - the settings for the current notebook server
	* @param docManager - the document manager instance
	* @param entry - the entry corresponding to the file to work with
	*/
	function apiPostTutorialsEntry(
	settings: ServerConnection.ISettings,
	docManager: IDocumentManager,
	entry: TutorialsEntry
	): void {

[stvoutsin]

I think in general the Javascript convention is jsdoc before function

[stvoutsin]

Perhaps a future improvement but buildTutorialsMenu could potentially be broken down to smaller single-responsibility methods?
Example:

• initializeMenu: Menu initialization
• buildSubmenus: Submenu creation
• addMenuEntries: Menu entry creation

[stvoutsin]

Might be personal preference but I'd use methods outside of the constructor like:

  constructor(
    input: ITutorialsHierarchyResponse,
    private readonly name: string = '<unnamed>'
  ) {
    console.log(`Building hierarchy ${this.name}`);
    this.processEntries(input.entries);
    this.processSubhierarchies(input.subhierarchies);
    console.log(`Hierarchy ${this.name} built`);
  }

[stvoutsin]

Object.entries may be useful for iteration, up to you though:

Example:

 private processEntries(entries: Record<string, TutorialsEntry> | null): void {
    if (!entries) return;
    for (const [entryName, entry] of Object.entries(entries)) { 
       ...
       console.log(`Adding entry ${entryName}: ${JSON.stringify(entry, null, 2)}`);
       ...

[stvoutsin]

Tested this out locally and the menu seems to work fine and as expected once I had the right env vars and lab args.

I had a look through the rest of the changes since I only got a chance to review the Typescript side last week. It all looks good to me.

Only a couple quick suggestions/thoughts added here

[stvoutsin]

To be extra safe would it make sense to catch errors loading the JSON file (if it is corrupted or modified for some reason to a non valid json file) and then return None, so the hierarchy is repopulated?

Suggested change

	return Hierarchy.from_primitive(json.loads(stash.read_text()))
	try:
	content = stash.read_text()
	json_data = json.loads(content)
	return Hierarchy.from_primitive(json_data)
	except (OSError, json.JSONDecodeError, HierarchyError):
	return None

[stvoutsin]

Perhaps this could be slightly simplified and you can avoid some extra checks:

    def to_primitive(self) -> dict[str, Any]:
        """Convert to JSON-serializable format."""
        h: dict[str, Any] = {
            "entries": None,
            "subhierarchies": None
        }

        if self.entries:
            h["entries"] = {
                entry_name: entry.to_primitive()
                for entry_name, entry in self.entries.items()
            }

        if self.subhierarchies:
            h["subhierarchies"] = {
                subh_name: subh.to_primitive()
                for subh_name, subh in self.subhierarchies.items()
            }

        return h

[stvoutsin]

I think this is fine but if you did want to leave this work up to Pydantic, I'm wondering if something like this would work?

class Hierarchy(BaseModel):
    """Pydantic validated version of tree structure."""

    entries: dict[str, HierarchyEntry] | None = Field(
        default=None,
        title="Transformable file entries"
    )
    subhierarchies: dict[str, Hierarchy] | None = Field(
        default=None,
        title="Transformable sub-hierarchies"
    )

    class Config:
        json_encoders: ClassVar[dict] = {
            Path: str
        }

And then in your GET handler you can write it out as:

self.write(json.dumps(self.tutorials.model_dump(mode='json')))