cheshire-cat-ai / admin-vue

Admin panel of the Cheshire Cat AI
GNU General Public License v3.0
32 stars 28 forks source link

Api client initialization #83

Closed pieroit closed 1 month ago

pieroit commented 1 month ago

I still suggest to have a store with app wide state, and that store will init the api and keep variables that are relevant for the whole app (i.e. apiClient, theme, notifications). Other stores that depend on the api will read on the main store to avoid race conditions.

For example the messages store adds callbacks to the apiClient; it should instead observe a central store that inits apiClient and the callbacks. Page views should not deal with network management.

Also, several API calls are done when not needed (and they also give 403 when a user has no permissions). Those calls should be done only when needed and allowed.

I know this is a lot, but the admin grew a lot and needs some rethinking. What has been done so far is great!!! I am available for a central store design and refactoring!

Thanks for reviewing @zAlweNy26 , if you have no time ping me and I'll merge

zAlweNy26 commented 1 month ago

You are using Vue reactivity system outside of Vue components/composables, this is an highly discouraged thing to do (as the Vue team said that), instead move the API client initialization inside a Pinia store.

pieroit commented 1 month ago

You are using Vue reactivity system outside of Vue components/composables, this is an highly discouraged thing to do (as the Vue team said that), instead move the API client initialization inside a Pinia store.

Yep that is why I'm proposing a central store. I'll try to do it but may need some help

I suggest you merge this, it is way cleaner than updateCredentials

Also, the reconnection bug is finally fixed ( I was riiiiight) 🐱

pieroit commented 1 month ago

You are using Vue reactivity system outside of Vue components/composables, this is an highly discouraged thing to do (as the Vue team said that), instead move the API client initialization inside a Pinia store.

@zAlweNy26 The first step I took was to move apiClient in a composable useApiClient so it reacts to cookie/jwt changes Next step should be to move also the callbacks (now in useMessage.ts) but don't know how to do it. Hope this is better

zAlweNy26 commented 1 month ago

Much better, I'll review it better tomorrow and after some refinements I think it can be merged

zAlweNy26 commented 1 month ago

For reference to some of the comments: https://vuejs.org/guide/reusability/composables.html#usage-restrictions

pieroit commented 1 month ago

closing this PR and restarting a new one!