Feature: Try/Catch mechanisim

[open-sauced]

Feature: Try/Catch mechanisim

[bdougie]

Are you seeing errors?

It would be preferred to fix this while fixing an actual error, otherwise optimization for sake of optimization is not needed at this time.

[a0m0rajab]

No, I am not seeing any errors, I just noticed that when I started working on #88

[Anush008]

The code refactor and description generation button click handlers do have try/catch blocks implemented to handle errors generated from the API calls.

ai/src/content-scripts/components/AICodeRefactor/ChangeSuggestorButton.ts

Lines 24 to 82 in 805a730

	try {
	if (!(await isLoggedIn())) {
	return window.open(SUPABASE_LOGIN_URL, "_blank");
	}
	const logo = document.getElementById("ai-description-button-logo");
	if (!logo) {
	return;
	}
	const descriptionConfig = await getAIDescriptionConfig();
	if (!descriptionConfig) {
	return;
	}
	if (!descriptionConfig.enabled) {
	return alert("AI PR description is disabled!");
	}
	logo.classList.toggle("animate-spin");
	const selectedLines = document.querySelectorAll(".code-review.selected-line");
	let selectedCode = Array.from(selectedLines).map(line => line.textContent)
	.join("\n");
	// find input with name="position" and get its value
	if (!selectedCode) {
	const positionElement = (commentNode.querySelector("input[name=position]")!);
	const position = positionElement.getAttribute("value")!;
	const codeDiv = document.querySelector(`[data-line-number="${position}"]`)?.nextSibling?.nextSibling as HTMLElement;
	selectedCode = codeDiv.getElementsByClassName("blob-code-inner")[0].textContent!;
	}
	if (isOutOfContextBounds([selectedCode, [] ], descriptionConfig.config.maxInputLength)) {
	logo.classList.toggle("animate-spin");
	return alert(`Max input length exceeded. Try reducing the number of selected lines to refactor.`);
	}
	const token = await getAuthToken();
	const suggestionStream = await generateCodeSuggestion(
	token,
	descriptionConfig.config.language,
	descriptionConfig.config.length,
	descriptionConfig.config.temperature / 10,
	selectedCode,
	);
	logo.classList.toggle("animate-spin");
	if (!suggestionStream) {
	return console.error("No description was generated!");
	}
	const textArea = commentNode.querySelector(GITHUB_PR_SUGGESTION_TEXT_AREA_SELECTOR)!;
	insertTextAtCursor(textArea as HTMLTextAreaElement, suggestionStream);
	} catch (error: unknown) {
	if (error instanceof Error) {
	console.error("Description generation error:", error.message);
	}
	}

ai/src/content-scripts/components/GenerateAIDescription/DescriptionGeneratorButton.ts

Lines 26 to 77 in 805a730

	try {
	if (!(await isLoggedIn())) {
	return window.open(SUPABASE_LOGIN_URL, "_blank");
	}
	const logo = document.getElementById("ai-description-button-logo");
	if (!logo) {
	return;
	}
	const url = getPullRequestAPIURL(window.location.href);
	const descriptionConfig = await getAIDescriptionConfig();
	if (!descriptionConfig) {
	return;
	}
	if (!descriptionConfig.enabled) {
	return alert("AI PR description is disabled!");
	}
	logo.classList.toggle("animate-spin");
	const [diff, commitMessages] = await getDescriptionContext(url, descriptionConfig.config.source);
	if (!diff && !commitMessages) {
	logo.classList.toggle("animate-spin");
	return alert(`No input context was generated.`);
	}
	if (isOutOfContextBounds([diff, commitMessages], descriptionConfig.config.maxInputLength)) {
	logo.classList.toggle("animate-spin");
	return alert(`Max input length exceeded. Try setting the description source to commit-messages.`);
	}
	const token = await getAuthToken();
	const descriptionStream = await generateDescription(
	token,
	descriptionConfig.config.language,
	descriptionConfig.config.length,
	descriptionConfig.config.temperature / 10,
	descriptionConfig.config.tone,
	diff,
	commitMessages,
	);
	logo.classList.toggle("animate-spin");
	if (!descriptionStream) {
	return console.error("No description was generated!");
	}
	const textArea = document.getElementsByName(GITHUB_PR_COMMENT_TEXT_AREA_SELECTOR)[0] as HTMLTextAreaElement;
	insertTextAtCursor(textArea, descriptionStream);
	} catch (error: unknown) {
	if (error instanceof Error) {
	console.error("Description generation error:", error.message);
	}
	}

[a0m0rajab]

The issue with warping the whole function in try/catch is that when I tried to use the functions inside this by itself, like: getDescript, description stream, I had to warp that with a try/cacth again. I think having try catch the base would be more ideal.

what your thoughts? @Anush008

[Anush008]

The idea is to have the caller handle the thrown errors as required.

[diivi]

I think more specific errors could be handled, but we'll just log them all anyway, no other mechanism to deal with any of the for now.

So this can be done later.

[a0m0rajab]

So, should we keep this open until it's done later? So that we will not forget it?

[Anush008]

It's just that when you call functions that throw, you can handle them as required. There's no way around it. It isn't appropriate to handle the error in the function definition since the caller might need a different behaviour on error.

It isn't something that needs to be fixed or improved.

[a0m0rajab]

You can think about it as something similar to API calls, when you got 401 for example it's not throwing an error, but it's returning a response with response.ok state if it's okay then you can move forward.

Here you can check the insights project logic:

https://github.com/open-sauced/insights/blob/168312dd5521746fc28dd05871d7eb0bcfb7f655/lib/hooks/createHighlights.ts#L39-L46

[diivi]

@a0m0rajab can you share an example of an error from our project? That'll help in tackling the issue too, if there is one :)

[a0m0rajab]

@diivi it's more of a I could not use the util functions since that did not provide a try/catch stuff rather than an error I faced.