Implemented Notification from the mantine functionality for Config.tsx and Urloader.tsx #121

auksesir · 2023-09-04T21:03:50Z

I made the code to empty the text input box and uncheck the checkbox after submitting the URL. I also made selected values stay as placeholders in Config.tsx.

vercel · 2023-09-04T21:03:53Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
aixplora	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Sep 19, 2023 11:13pm

grumpyp · 2023-09-05T10:08:06Z

frontend/src/renderer/pages/file-upload/components/Urlloader.tsx

@@ -4,6 +4,8 @@ import './UrlLoader.css';
 import { useEffect, useState } from 'react';
 import { apiCall } from '../../../utils/api';
 import { useForm } from '@mantine/form';
+import { Notifications } from '@mantine/notifications';
+import { check } from 'prettier';


this seems not to be used?

grumpyp

lgtm awesome work.

Can you also refresh the Uploaded.tsx component, if someone indexed a Website? Basically like you did with the callback function for the Dropzone component.

In the App.tsx there's also some logic which either shows the Config page or the Welcome to AIxplora.

This is currently not working I believe it has to do something with state change, please doublecheck here.

Again: Thanks a lot for working on this!

grumpyp · 2023-09-07T20:26:45Z

I reviewed again, looks good to me but something with the localstorage doesn't work properly. If I refresh my config is gone again. Testing it via home <-> config page

auksesir · 2023-09-07T20:47:10Z

Do you want the App to keep the same configuration after reloading it?
I can implement that.

grumpyp · 2023-09-08T07:17:30Z

Do you want the App to keep the same configuration after reloading it? I can implement that.

Hi, yes, once a user has set a configuration it should always fallback on it.

In the backend it's implemented like this, so it references the last row of the table..

...
self.openai_api_key = Database().get_session().execute(text("SELECT openai_api_key FROM config")).fetchall()[-1]
...

Otherwise a user had to set a new config on every start-up.

grumpyp · 2023-09-10T08:12:53Z

frontend/src/renderer/pages/file-upload/components/Urlloader.tsx

    apiCall('/files/website', 'POST', payload)
      .then((response) => {
-        console.log(response.data);
-        window.location.reload();
+
+        Notifications.show({
+          title: 'URL Upload Successful',
+          message: 'The URL was successfully uploaded.',
+          color: 'green'
+        });
+
+        // passing the website to the Upload.tsx component
+        onFilesUploaded([payload.website]);
      })


we should add a check here, if the status code f.e. is 500 it still will "upload successful".

grumpyp · 2023-09-10T08:15:22Z

frontend/src/renderer/pages/config/Config.tsx

@@ -37,10 +38,9 @@ function saveConfig(OPENAI_API_KEY: string, model: string, embeddingsmodel: stri
 //             console.log('Error fetching config:', error);
 //             return false;
 //         });
-// }
+// }console.log("saveconfig");
    return apiCall('/config', 'POST', payload).then((response) => {


Would you say we should extend the apiCall function with a boolean to deactivate the Notification? As of now a user gets the Endpoint error + the configerror.

grumpyp · 2023-09-10T08:16:59Z

frontend/src/renderer/pages/home/Home.tsx

+    useEffect(() => {
+        // Attempt to retrieve the configuration data from localStorage
+        const savedConfig = JSON.parse(localStorage.getItem('config') || '{}');
+
+        if (Object.keys(savedConfig).length > 0) {
+            // Configuration data exists in localStorage, set it as valid
+            setConfigValid(true);
+        } else {
+            // Configuration data does not exist in localStorage, set it as invalid
+            setConfigValid(false);
+        }
+    }, []);
+
+    if (isConfigValid === undefined) {
+        return <div>Loading...</div>;
+    }
+
+
+    return isConfigValid ? <Start /> : <Config setConfigValid={setConfigValid} />;
+}


Hi there, it works like expected. Very good job!

I would like to have another oppinion on the behavior. So for now, if I delete the database, the API key is still stored.
I mentioned that in this case an API call would make sense every time the Home page is opened initally.

Would you be open to discussing whether to adapt this approach? Should we implement it additionally or go with that approach? So we have 1 single souce of truth basically -> which is the db.

Same for the Config page btw :)

grumpyp

Great work, I left some comments with questions :)

auksesir · 2023-09-10T11:49:32Z

Hi, that makes sense. I'll start working on it :)

grumpyp · 2023-09-12T17:58:31Z

frontend/src/renderer/utils/api.tsx

@@ -14,7 +14,7 @@ const selectBaseUrl = () => {
  }
 };

-export const apiCall = async (endpoint, method, data) => {
+export const apiCall = async (endpoint, method, data, disableNotification = false) => {


the default parameter should either be true or it should be if(disabledNotification), otherwise we won't have any Notifcation.

grumpyp · 2023-09-12T18:39:08Z

frontend/src/renderer/pages/config/Config.tsx

+        // Make an API call to fetch the configuration data only on the first load
+        if (!dataLoaded) {
+            apiCall('/config', 'GET', null, true)
+                .then((response) => {
+                    const fetchedConfig = response.data;
+                    // Set the form values based on the retrieved data
+                    form.setValues({
+                        OPENAI_API_KEY: fetchedConfig.openai_api_key || '',
+                        model: fetchedConfig.model || '',
+                        embeddingsmodel: fetchedConfig.embeddings_model || '',
+                    });
+                    // Save to localStorage for future loads
+                    localStorage.setItem('config', JSON.stringify(fetchedConfig));
+                    // Set the dataLoaded flag to true to prevent future API calls
+                    setDataLoaded(true);
+                })
+                .catch((error) => {
+                    console.log('Error fetching config:', error);
+                    // Handle the error as needed, maybe show a notification or take other actions
+                });
+        }
+


this would make the first load everytime one goes back to Config component - is the if dataLoaded then even needed? as the state will be gone if we change the component.

grumpyp · 2023-09-12T18:43:51Z

frontend/src/renderer/App.tsx

@@ -41,12 +93,11 @@ export default function Hello() {
                    <Router>
                        <HeaderResponsive links={randomLinks}/>
                        <Routes>
-                            <Route path="/" element={<Home />}/>
+                            <Route path="/" element={isConfigValid ? <GettingStarted/> : <Config/>}/>


I am not sure if this will work, if the API returns JSON instead of the true and false which is wanted in the JSX condition.

grumpyp

I think I found some little mistakes, other than that nice work. I like the additional status code implementation you did even without questioning. Really awesome :)

grumpyp · 2023-09-13T18:54:01Z

hi @auksesir please ping me if you checked my review and changed if possible so I'll have another look. thx

auksesir · 2023-09-13T19:46:07Z

Hi @grumpyp, I've changed it. Let me know if it's working correctly.

grumpyp · 2023-09-14T14:20:51Z

Hi, I am sorry but it's not working like expected:

So if there's no Config in the DB, it should render the Config component. If there's one GettingStarted. You can play around by deleting the database.db sqllite db.

auksesir · 2023-09-19T23:18:15Z

Hi @grumpyp. It should work as expected now.

I tried testing it one last time but I started getting this error:

Access to XMLHttpRequest at 'http://localhost:8000/config/' (redirected from 'http://localhost:8000/config') from origin 'http://localhost:1212' has been blocked by CORS policy: No 'Access-Control-Allow-Origin' header is present on the requested resource.

It started occurring even though I had not changed anything. Do you have any idea what could it be? I was trying to figure it out but nothing worked.

grumpyp · 2023-09-20T09:30:54Z

Hi, sorry, but it doesn't work.

Please make also sure to merge the current origin/main, I left a little comment which caught my attention when started the review.

vercel bot deployed to Preview September 4, 2023 21:04 View deployment

grumpyp reviewed Sep 5, 2023

View reviewed changes

grumpyp requested changes Sep 5, 2023

View reviewed changes

vercel bot deployed to Preview September 7, 2023 11:00 View deployment

vercel bot deployed to Preview September 7, 2023 11:05 View deployment

vercel bot deployed to Preview September 9, 2023 14:46 View deployment

grumpyp reviewed Sep 10, 2023

View reviewed changes

grumpyp requested changes Sep 10, 2023

View reviewed changes

vercel bot deployed to Preview September 11, 2023 11:59 View deployment

grumpyp reviewed Sep 12, 2023

View reviewed changes

grumpyp requested changes Sep 12, 2023

View reviewed changes

vercel bot deployed to Preview September 13, 2023 14:04 View deployment

vercel bot deployed to Preview September 19, 2023 23:13 View deployment

auksesir closed this Oct 2, 2023

auksesir force-pushed the main branch from ce90608 to efde240 Compare October 2, 2023 11:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implemented Notification from the mantine functionality for Config.tsx and Urloader.tsx #121

Implemented Notification from the mantine functionality for Config.tsx and Urloader.tsx #121

auksesir commented Sep 4, 2023

vercel bot commented Sep 4, 2023 •

edited

Loading

grumpyp Sep 5, 2023

grumpyp left a comment •

edited

Loading

grumpyp commented Sep 7, 2023

auksesir commented Sep 7, 2023

grumpyp commented Sep 8, 2023

grumpyp Sep 10, 2023

grumpyp Sep 10, 2023

grumpyp Sep 10, 2023

grumpyp left a comment

auksesir commented Sep 10, 2023

grumpyp Sep 12, 2023

grumpyp Sep 12, 2023

grumpyp Sep 12, 2023

grumpyp left a comment

grumpyp commented Sep 13, 2023

auksesir commented Sep 13, 2023

grumpyp commented Sep 14, 2023

auksesir commented Sep 19, 2023

grumpyp commented Sep 20, 2023

Implemented Notification from the mantine functionality for Config.tsx and Urloader.tsx #121

Implemented Notification from the mantine functionality for Config.tsx and Urloader.tsx #121

Conversation

auksesir commented Sep 4, 2023

vercel bot commented Sep 4, 2023 • edited Loading

grumpyp Sep 5, 2023

Choose a reason for hiding this comment

grumpyp left a comment • edited Loading

Choose a reason for hiding this comment

grumpyp commented Sep 7, 2023

auksesir commented Sep 7, 2023

grumpyp commented Sep 8, 2023

grumpyp Sep 10, 2023

Choose a reason for hiding this comment

grumpyp Sep 10, 2023

Choose a reason for hiding this comment

grumpyp Sep 10, 2023

Choose a reason for hiding this comment

grumpyp left a comment

Choose a reason for hiding this comment

auksesir commented Sep 10, 2023

grumpyp Sep 12, 2023

Choose a reason for hiding this comment

grumpyp Sep 12, 2023

Choose a reason for hiding this comment

grumpyp Sep 12, 2023

Choose a reason for hiding this comment

grumpyp left a comment

Choose a reason for hiding this comment

grumpyp commented Sep 13, 2023

auksesir commented Sep 13, 2023

grumpyp commented Sep 14, 2023

auksesir commented Sep 19, 2023

grumpyp commented Sep 20, 2023

vercel bot commented Sep 4, 2023 •

edited

Loading

grumpyp left a comment •

edited

Loading