Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Chatbot API #37

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Chatbot API #37

wants to merge 2 commits into from

Conversation

dansza1
Copy link
Contributor

@dansza1 dansza1 commented Oct 28, 2024

Add API to the chatbot
Add api command and camera function to fetch screenshots from the cameras.

Comment on lines 174 to 181
// Global error handling
process.on('unhandledRejection', (reason, promise) => {
console.error('Unhandled Rejection at:', promise, 'reason:', reason);
});

process.on('uncaughtException', (error) => {
console.error('Uncaught Exception:', error);
});
Copy link
Member

Choose a reason for hiding this comment

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

Not a fan of adding global stuff into a specific connection file -- this will stop any fatal error across the entire bot from killing the process

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was same as below, to avoid having the API crash the node, but you are correct not good practice to have global stuff in the connection file, will remove.

Comment on lines 190 to 192
} catch (error) {
console.error('Failed to instantiate API:', error.message);
}
Copy link
Member

Choose a reason for hiding this comment

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

Similarly here, not sure this should have a try/catch -- if it fails, it should fail

Copy link
Contributor Author

@dansza1 dansza1 Oct 28, 2024

Choose a reason for hiding this comment

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

I think my goal was to make sure nothing with the API takes down the entire chatbot as the API is not critical to the bots functioning. But if its better practice to remove the try/catch we can do that.

async #getBinary(endpoint) {
try {
const url = `http://${this.#host}${endpoint}`;
console.log(`Fetching binary data from: ${url}`); // Log the request URL
Copy link
Member

Choose a reason for hiding this comment

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

Can this use logger?


// Return the response as an ArrayBuffer
const data = await response.arrayBuffer();
console.log('Received ArrayBuffer of size:', data.byteLength); // Log the size of the ArrayBuffer
Copy link
Member

Choose a reason for hiding this comment

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

Can this use logger?

}
} catch (error) {
this.#logger.error(`Failed to fetch image: ${error.message}`); // Improved error logging
console.error('Error fetching image:', error);
Copy link
Member

Choose a reason for hiding this comment

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

Can this use logger?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes should be logger, will change.

Comment on lines 44 to 47
if (this.controller) {
this.controller.ws = this.#ws; // Ensure controller.ws is set
this.#logger.log('controller.ws assigned');
}
Copy link
Member

Choose a reason for hiding this comment

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

Why are we exposing what is intentionally marked as private in this class? There is already a standard pattern in place for other logic accessing connections from the controller, this violates that pattern?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was for sendAPI and sendBroadcast, but you make a good point, i will move both into the API class.

src/connections/api.js Show resolved Hide resolved
src/connections/api.js Outdated Show resolved Hide resolved
src/connections/api.js Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants